<div dir="ltr">Hi Partha,<div><br></div><div>CC-ing lttng-dev and inlining your patch to ease the review... Read on.</div><div><br></div><div><div>> diff --git a/src/bin/lttng/commands/destroy.c b/src/bin/lttng/commands/destroy.c</div><div>> index 95343c9..2d1200f 100644</div><div>> --- a/src/bin/lttng/commands/destroy.c</div><div>> +++ b/src/bin/lttng/commands/destroy.c</div><div>> @@ -75,6 +75,7 @@ static void usage(FILE *ofp)</div><div>> static int destroy_session(struct lttng_session *session)</div><div>> {</div><div>> <span class="" style="white-space:pre"> </span>int ret;</div><div>> +<span class="" style="white-space:pre"> </span>char *session_name = NULL;</div><div>> </div><div>> <span class="" style="white-space:pre"> </span>ret = lttng_destroy_session(session->name);</div><div>> <span class="" style="white-space:pre"> </span>if (ret < 0) {</div><div>> @@ -90,7 +91,18 @@ static int destroy_session(struct lttng_session *session)</div><div>> <span class="" style="white-space:pre"> </span>}</div><div>> </div><div>> <span class="" style="white-space:pre"> </span>MSG("Session %s destroyed", session->name);</div><div>> -<span class="" style="white-space:pre"> </span>config_destroy_default();</div><div>> +</div><div>> +<span class="" style="white-space:pre"> </span>session_name = get_session_name();</div><div>> +<span class="" style="white-space:pre"> </span>if (session_name == NULL) {</div><div>> +<span class="" style="white-space:pre"> </span>ret = CMD_ERROR;</div><div>> +<span class="" style="white-space:pre"> </span>goto error;</div><div>> +<span class="" style="white-space:pre"> </span>}</div><div>> +</div><div><br></div><div>This may not be an error.</div><div>get_session_name() will return NULL whenever a default session is not defined. This will happen, for instance, when loading a session configuration.</div><div><br></div><div>The test_load script, under lttng-tools/tests/regression/tools/save-load will perform:</div><div><br></div><div>$ lttng load -i load-42.lttng</div><div><div>$ lttng destroy load-42</div><div>Session load-42 destroyed</div><div>Error: Can't find valid lttng config /home/jgalar/.lttngrc</div><div>Did you create a session? (lttng create <my_session>)</div><div>Error: Command error</div></div><div><br></div><div>(make sure you delete ~/.lttngrc beforehand to test this properly)</div><div><br></div><div>> +<span class="" style="white-space:pre"> </span>if (strncmp(session->name, session_name, NAME_MAX) == 0) {</div><div>> +<span class="" style="white-space:pre"> </span>config_destroy_default();</div><div>> +<span class="" style="white-space:pre"> </span>}</div><div>> +</div><div><br></div><div>We should skip this check if get_session_name() returned NULL with something like:</div><div><br></div><div>if (session_name && strncmp(session->name, session_name, NAME_MAX) == 0) { ...</div><div><br></div><div>> +<span class="" style="white-space:pre"> </span>free(session_name);</div><div>> </div><div>> <span class="" style="white-space:pre"> </span>if (lttng_opt_mi) {</div><div>> <span class="" style="white-space:pre"> </span>ret = mi_lttng_session(writer, session, 0);</div><div>> diff --git a/src/bin/lttng/conf.c b/src/bin/lttng/conf.c</div><div>> index 7e6c833..6b8bf51 100644</div><div>> --- a/src/bin/lttng/conf.c</div><div>> +++ b/src/bin/lttng/conf.c</div><div>> @@ -199,8 +199,6 @@ char *config_read_session_name(char *path)</div><div>> </div><div>> <span class="" style="white-space:pre"> </span>fp = open_config(path, "r");</div><div>> <span class="" style="white-space:pre"> </span>if (fp == NULL) {</div><div>> -<span class="" style="white-space:pre"> </span>ERR("Can't find valid lttng config %s/.lttngrc", path);</div><div>> -<span class="" style="white-space:pre"> </span>MSG("Did you create a session? (lttng create <my_session>)");</div><div>> <span class="" style="white-space:pre"> </span>free(session_name);</div><div>> <span class="" style="white-space:pre"> </span>goto error;</div><div>> <span class="" style="white-space:pre"> </span>}</div><div>> diff --git a/src/bin/lttng/utils.c b/src/bin/lttng/utils.c</div><div>> index ef8d023..d6e854b 100644</div><div>> --- a/src/bin/lttng/utils.c</div><div>> +++ b/src/bin/lttng/utils.c</div><div>> @@ -59,6 +59,8 @@ char *get_session_name(void)</div><div>> <span class="" style="white-space:pre"> </span>/* Get session name from config */</div><div>> <span class="" style="white-space:pre"> </span>session_name = config_read_session_name(path);</div><div>> <span class="" style="white-space:pre"> </span>if (session_name == NULL) {</div><div>> +<span class="" style="white-space:pre"> </span>ERR("Can't find valid lttng config %s/.lttngrc", path);</div><div>> +<span class="" style="white-space:pre"> </span>MSG("Did you create a session? (lttng create <my_session>)");</div><div>> <span class="" style="white-space:pre"> </span>goto error;</div><div>> <span class="" style="white-space:pre"> </span>}</div><div><br></div><div>get_session_name() should not log this when an .lttngrc file can't be found; it might not be an error.</div><div>It should be handled on a case-by-case basis by its callers.</div><div><br></div><div>Thanks,</div><div>Jérémie</div><div><br></div><div>> </div><div>> -- </div><div>> 1.7.9.5</div><div>> </div></div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 26, 2015 at 1:15 AM, Partha Pratim Mukherjee <span dir="ltr"><<a href="mailto:ppm.floss@gmail.com" target="_blank">ppm.floss@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">Jérémie Galarneau <<a href="mailto:jeremie.galarneau@efficios.com">jeremie.galarneau@efficios.com</a>> writes:<br>
<br>
> Hi Partha,<br>
><br>
> First, thanks for the patch! Unfortunately, there are a couple of issues with this fix. Read on.<br>
><br>
> On Wed, May 20, 2015 at 2:59 PM, Partha Pratim Mukherjee <<a href="mailto:ppm.floss@gmail.com">ppm.floss@gmail.com</a>> wrote:<br>
><br>
> Destroy session command by default removes the default config file<br>
> without checking the current session. As a result when we call any<br>
> other command which expects a default session by calling<br>
> get_session_name() function, it fails.<br>
><br>
> This patch will fix this by checking that the default config file gets<br>
> removed only when destroy session is called with the current session.<br>
><br>
> Fixes: #887<br>
><br>
> Signed-off-by: Partha Pratim Mukherjee <<a href="mailto:ppm.floss@gmail.com">ppm.floss@gmail.com</a>><br>
> ---<br>
> src/bin/lttng/commands/destroy.c | 4 +++-<br>
> 1 file changed, 3 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/src/bin/lttng/commands/destroy.c b/src/bin/lttng/commands/destroy.c<br>
> index 95343c9..3fcecc2 100644<br>
> --- a/src/bin/lttng/commands/destroy.c<br>
> +++ b/src/bin/lttng/commands/destroy.c<br>
> @@ -90,7 +90,9 @@ static int destroy_session(struct lttng_session *session)<br>
> }<br>
><br>
> MSG("Session %s destroyed", session->name);<br>
> - config_destroy_default();<br>
> + if (strncmp(session->name, get_session_name(), NAME_MAX) == 0) {<br>
><br>
> This will leak the string returned by get_session_name().<br>
><br>
> 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;<br>
> the error logging statement should be moved out to the different callers.<br>
><br>
> Looking forward to a v2!<br>
> Jérémie<br>
<br>
</div></div>Hi Jérémie,<br>
<br>
Thanks for the review comments. I have incorporated your comments in the<br>
attached patch. Please review.<br>
<br>
Thanks,<br>
Partha<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Jérémie Galarneau<br>EfficiOS Inc.<br><a href="http://www.efficios.com" target="_blank">http://www.efficios.com</a></div>
</div></div>