-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Daniel,
I will not merge this before rc1. This changes the destroy command and unless there is a bug or important behavior fix, I'll wait after the stable release. Comments below. On 12-01-31 10:17 AM, Thibault, Daniel wrote: > destroy.c : > * Fix usage() output alignment > * Document and enforce return values of cmd_destroy() and destroy_session() > * Output --help to stdout > * Fix destroy_session() so it removes the config when the current session is > explicitly specified > conf.c : > * Fix config_destroy() description to match behaviour > ------------------------------ > From 41f3600d9f24fb082ab2e60e61f50960f0ef35eb Tue, 31 Jan 2012 10:12:32 -0500 > From: Daniel U. Thibault <[email protected]> > Date: Tue, 31 Jan 2012 10:12:19 -0500 > Subject: [PATCH] lttng-tools conf.c : Fix config_destroy description to match > behaviour; destroy.c : Fix usage output alignment, document and enforce > return values, output --help to stdout, fix destroy_session so it removes the > config when the current session is explicitly specified > > Signed-off-by: Daniel U. Thibault <[email protected]> > > diff --git a/src/bin/lttng/commands/destroy.c > b/src/bin/lttng/commands/destroy.c > index 39b4e9a..441b858 100644 > --- a/src/bin/lttng/commands/destroy.c > +++ b/src/bin/lttng/commands/destroy.c > @@ -52,31 +52,43 @@ > fprintf(ofp, "get it from the configuration directory (.lttng).\n"); > fprintf(ofp, "\n"); > fprintf(ofp, " -h, --help Show this help\n"); > - fprintf(ofp, " --list-options Simple listing of options\n"); > + fprintf(ofp, " --list-options Simple listing of options\n"); > fprintf(ofp, "\n"); > } > > /* > - * Destroy a session removing the config directory and unregistering to the > - * session daemon. > + * Destroys a session, unregistering it from the session daemon. > + * If the session is unspecified, the current one is destroyed. > + * If the current session is destroyed, so is its config file. > + * > + * Returns one of the CMD_* result values. > */ > static int destroy_session() > { > int ret; > - char *session_name, *path; > + int destroying_current_session; > + char *session_name, *cur_session_name, *path; > > - if (opt_session_name == NULL) { > - session_name = get_session_name(); > - if (session_name == NULL) { > - ret = CMD_ERROR; > - goto error; > - } > - } else { > - session_name = opt_session_name; > + cur_session_name = get_session_name(); > + if ((cur_session_name == NULL) && (opt_session_name == NULL)) { > + ret = CMD_ERROR; > + goto error; Extra parentheses are useless here. (Just for "homogeneity" across the code base). > } > + /* At least one of cur_session_name and opt_session_name is not NULL */ > + session_name = (opt_session_name ? opt_session_name : cur_session_name); > + /* > + * We're destroying the current session in two cases: > + * If no explicit session was supplied, or > + * the explicit session name matches the current session name's (if it > exists) 80 carac. > + */ > + destroying_current_session = > + (opt_session_name == NULL) || > + (cur_session_name ? > + (strcmp(cur_session_name, > opt_session_name)==0) : 0); You seem to like compact "if..else" ;). I have no problem with some... "obvious" case but here I'll rather prefer basic if...else for code clarity and avoid confusion in the future. Finally, small detail which can save me time merging you patch. The code is standardize on 80 characters per line (kernel standard). Most of the time you are OK but comments are often unaligned. I don't know if Eclipse can allow you that but it will be very helpful. Thanks a lot! David > > ret = lttng_destroy_session(session_name); > if (ret < 0) { > + ret = CMD_ERROR; > goto free_name; > } > > @@ -86,7 +98,7 @@ > goto free_name; > } > > - if (opt_session_name == NULL) { > + if (destroying_current_session) { > config_destroy(path); > MSG("Session %s destroyed at %s", session_name, path); > } else { > @@ -96,8 +108,8 @@ > ret = CMD_SUCCESS; > > free_name: > - if (opt_session_name == NULL) { > - free(session_name); > + if (cur_session_name != NULL) { > + free(cur_session_name); > } > error: > return ret; > @@ -105,6 +117,8 @@ > > /* > * The 'destroy <options>' first level command > + * > + * Returns one of the CMD_* result values. > */ > int cmd_destroy(int argc, const char **argv) > { > @@ -117,11 +131,10 @@ > while ((opt = poptGetNextOpt(pc)) != -1) { > switch (opt) { > case OPT_HELP: > - usage(stderr); > + usage(stdout); > goto end; > case OPT_LIST_OPTIONS: > list_cmd_options(stdout, long_options); > - ret = CMD_SUCCESS; > goto end; > default: > usage(stderr); > diff --git a/src/bin/lttng/conf.c b/src/bin/lttng/conf.c > index 00991b0..0ff5bf1 100644 > --- a/src/bin/lttng/conf.c > +++ b/src/bin/lttng/conf.c > @@ -143,7 +143,7 @@ > /* > * config_destroy > * > - * Destroys directory config and file config. > + * Removes the config file from the config directory. > */ > void config_destroy(char *path) > { > ------------------------------ > > Daniel U. Thibault > R & D pour la défense Canada - Valcartier (RDDC Valcartier) / Defence R&D > Canada - Valcartier (DRDC Valcartier) > Système de systèmes (SdS) / System of Systems (SoS) > Solutions informatiques et expérimentations (SIE) / Computing Solutions and > Experimentations (CSE) > 2459 Boul. Pie XI Nord > Québec, QC G3J 1X5 > CANADA > Vox : (418) 844-4000 x4245 > Fax : (418) 844-4538 > NAC: 918V QSDJ > Gouvernement du Canada / Government of Canada > <http://www.valcartier.drdc-rddc.gc.ca/> > > _______________________________________________ > lttng-dev mailing list > [email protected] > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) iQEcBAEBAgAGBQJPKXYjAAoJEELoaioR9I02NgUH/RXXZy/D1CkhG6oT1+fU9JN1 UgQAD+eIsOBKXtYxma673qvmtcORC9c4xhbUIRQUTcHuAEdttahwDN+4NxBq34Gt 870kdpbpR8ZqWS9mI676zShGHVK7wvc0pNB2+yrWoFKRoJ3hOCU+pLzmnQa/TeHo 7l3/PwzkI4GpdE60sjZhPxVkd7Jh2w47pEOCIAyUvyUlGQqtIWmVZ2q8G83SZRbE 8VYSxv5qytbHG3l4uFXsplGUbccqeDXK6V4cghyud8jpQd7ul3kYFmLDXOCIQuiL oHZUL0NGMEqHD3+aZqJFOig5FiZd+lKxMLLbXyHO3xXjrf9sKkq6yVMZuLQpybQ= =Sucm -----END PGP SIGNATURE----- _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
