[lttng-dev] [PATCH lttng-tools] Fix: destroy session removes the default config file

[Jérémie Galarneau]

Hi Partha,

CC-ing lttng-dev and inlining your patch to ease the review... Read on.

> diff --git a/src/bin/lttng/commands/destroy.c
b/src/bin/lttng/commands/destroy.c
> index 95343c9..2d1200f 100644
> --- a/src/bin/lttng/commands/destroy.c
> +++ b/src/bin/lttng/commands/destroy.c
> @@ -75,6 +75,7 @@ static void usage(FILE *ofp)
>  static int destroy_session(struct lttng_session *session)
>  {
>   int ret;
> + char *session_name = NULL;
>
>   ret = lttng_destroy_session(session->name);
>   if (ret < 0) {
> @@ -90,7 +91,18 @@ static int destroy_session(struct lttng_session
*session)
>   }
>
>   MSG("Session %s destroyed", session->name);
> - config_destroy_default();
> +
> + session_name = get_session_name();
> + if (session_name == NULL) {
> + ret = CMD_ERROR;
> + goto error;
> + }
> +

This may not be an error.
get_session_name() will return NULL whenever a default session is not
defined. This will happen, for instance, when loading a session
configuration.

The test_load script, under lttng-tools/tests/regression/tools/save-load
will perform:

$ lttng load -i load-42.lttng
$ lttng destroy load-42
Session load-42 destroyed
Error: Can't find valid lttng config /home/jgalar/.lttngrc
Did you create a session? (lttng create <my_session>)
Error: Command error

(make sure you delete ~/.lttngrc beforehand to test this properly)

> + if (strncmp(session->name, session_name, NAME_MAX) == 0) {
> + config_destroy_default();
> + }
> +

We should skip this check if get_session_name() returned NULL with
something like:

if (session_name && strncmp(session->name, session_name, NAME_MAX) == 0) {
...

> + free(session_name);
>
>   if (lttng_opt_mi) {
>   ret = mi_lttng_session(writer, session, 0);
> diff --git a/src/bin/lttng/conf.c b/src/bin/lttng/conf.c
> index 7e6c833..6b8bf51 100644
> --- a/src/bin/lttng/conf.c
> +++ b/src/bin/lttng/conf.c
> @@ -199,8 +199,6 @@ char *config_read_session_name(char *path)
>
>   fp = open_config(path, "r");
>   if (fp == NULL) {
> - ERR("Can't find valid lttng config %s/.lttngrc", path);
> - MSG("Did you create a session? (lttng create <my_session>)");
>   free(session_name);
>   goto error;
>   }
> diff --git a/src/bin/lttng/utils.c b/src/bin/lttng/utils.c
> index ef8d023..d6e854b 100644
> --- a/src/bin/lttng/utils.c
> +++ b/src/bin/lttng/utils.c
> @@ -59,6 +59,8 @@ char *get_session_name(void)
>   /* Get session name from config */
>   session_name = config_read_session_name(path);
>   if (session_name == NULL) {
> + ERR("Can't find valid lttng config %s/.lttngrc", path);
> + MSG("Did you create a session? (lttng create <my_session>)");
>   goto error;
>   }

get_session_name() should not log this when an .lttngrc file can't be
found; it might not be an error.
It should be handled on a case-by-case basis by its callers.

Thanks,
Jérémie

>
> --
> 1.7.9.5
>


On Tue, May 26, 2015 at 1:15 AM, Partha Pratim Mukherjee <
ppm.floss at gmail.com> wrote:

> Jérémie Galarneau <jeremie.galarneau at efficios.com> writes:
>
> > Hi Partha,
> >
> > First, thanks for the patch! Unfortunately, there are a couple of issues
> with this fix. Read on.
> >
> > On Wed, May 20, 2015 at 2:59 PM, Partha Pratim Mukherjee <
> ppm.floss at gmail.com> wrote:
> >
> >     Destroy session command by default removes the default config file
> >     without checking the current session. As a result when we call any
> >     other command which expects a default session by calling
> >     get_session_name() function, it fails.
> >
> >     This patch will fix this by checking that the default config file
> gets
> >     removed only when destroy session is called with the current session.
> >
> >     Fixes: #887
> >
> >     Signed-off-by: Partha Pratim Mukherjee <ppm.floss at gmail.com>
> >     ---
> >      src/bin/lttng/commands/destroy.c |    4 +++-
> >      1 file changed, 3 insertions(+), 1 deletion(-)
> >
> >     diff --git a/src/bin/lttng/commands/destroy.c
> b/src/bin/lttng/commands/destroy.c
> >     index 95343c9..3fcecc2 100644
> >     --- a/src/bin/lttng/commands/destroy.c
> >     +++ b/src/bin/lttng/commands/destroy.c
> >     @@ -90,7 +90,9 @@ static int destroy_session(struct lttng_session
> *session)
> >             }
> >
> >             MSG("Session %s destroyed", session->name);
> >     -       config_destroy_default();
> >     +       if (strncmp(session->name, get_session_name(), NAME_MAX) ==
> 0) {
> >
> > This will leak the string returned by get_session_name().
> >
> > Moreover, config_read_session_name(), which is called
> by get_session_name() will spam the error logs anytime an .lttngrc file
> can't be found;
> > the error logging statement should be moved out to the different callers.
> >
> > Looking forward to a v2!
> > Jérémie
>
> Hi Jérémie,
>
> Thanks for the review comments. I have incorporated your comments in the
> attached patch. Please review.
>
> Thanks,
> Partha
>
>


-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20150527/eb0461a5/attachment.html>