[lttng-dev] [PATCH] lttng-tools conf.c and destroy.c
David Goulet
david.goulet at polymtl.ca
Wed Feb 1 12:28:03 EST 2012
-----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 <daniel.thibault at drdc-rddc.gc.ca>
> 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 <daniel.thibault at drdc-rddc.gc.ca>
>
> 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
> lttng-dev at lists.lttng.org
> 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-----
More information about the lttng-dev
mailing list