[lttng-dev] [PATCH] ltt-control "Minimal changes" update (resubmission)
Yannick Brosseau
yannick.brosseau at gmail.com
Thu Dec 15 14:18:23 EST 2011
Hi Daniel,
The format is better, thank you. Would it be possible for you to split
the patch in several git commit and thus into several different patches.
(One "logical" change/feature per patch). This would make the review
much easier, and allow us to comment on specific changes.
Yannick
On 2011-12-14 16:31, Thibault, Daniel wrote:
> Using Eclipse's EGit plug-in (a standard feature since Eclipse 3.7, it turns out), I generated the patch which follows (as requested by Yannick on 8 December). I think it is produced directly in the git send-email format; correct me if I'm wrong.
>
> As I wrote in my local repository commit message (but had to trim here because it makes the Subject line way too long), this "Minimal changes" update does the following:
>
> * lttctl
> ** Makes opt_head, last_opt and set_channel_opt() private
> ** Fixes missing malloc failure detection in find_insert_channel_opt()
> ** Cosmetic re-ordering of the #includes
> ** Minor space vs. tab formatting fixes
>
> * liblttctl
> ** Comments out the unused #define MAX_CHANNEL
> ** Removes unnecessary lttctl_sendop() parameter check
> ** Fixes missing malloc failure detection in lttctl_get_channellist()
> ** Fixes missing parameter check in lttctl_set_trans() and getdebugfsmntdir()
> ** Fixes memory leak in lttctl_set_channel_*() functions
> ** Fixes missing fclose(FILE *fp) in getdebugfsmntdir()
> ** Prevents potential string buffer overruns by limiting the length of the debugfs mounting point path
> ** Prevents getdebugfsmntdir() from re-scanning /proc/mounts/ uselessly when the mount command fails
> ** Minor space vs. tab formatting fixes
>
> ----------------------------------------------------------------------
> From 6266f28621246f1dda00c2e0cbd391e0169acdcd Wed, 14 Dec 2011 15:44:16 -0500
> From: Daniel U. Thibault <daniel.thibault at drdc-rddc.gc.ca>
> Date: Wed, 14 Dec 2011 13:33:45 -0500
> Subject: [PATCH] "Minimal changes" update
>
> diff --git a/liblttctl/liblttctl.c b/liblttctl/liblttctl.c
> index 1032f15..9d4cc62 100644
> --- a/liblttctl/liblttctl.c
> +++ b/liblttctl/liblttctl.c
> @@ -35,7 +35,7 @@
> #include <fcntl.h>
> #include <stdlib.h>
>
> -#define MAX_CHANNEL (256)
> +//#define MAX_CHANNEL (256)
>
> static char debugfsmntdir[PATH_MAX];
>
> @@ -106,11 +106,6 @@
> static int lttctl_sendop(const char *fname, const char *op)
> {
> int fd;
> -
> - if (!fname) {
> - fprintf(stderr, "%s: args invalid\n", __func__);
> - return 1;
> - }
>
> fd = open(fname, O_WRONLY);
> if (fd == -1) {
> @@ -227,6 +222,10 @@
> continue;
> old_list = list;
> list = malloc(sizeof(char *) * ++nr_chan);
> + if (!list) {
> + nr_chan = -ENOMEM;
> + break;
> + }
> memcpy(list, old_list, sizeof(*list) * (nr_chan - 1));
> free(old_list);
> list[nr_chan - 1] = strdup(dirent->d_name);
> @@ -405,7 +404,7 @@
> int ret;
> char ctlfname[PATH_MAX];
>
> - if (!name) {
> + if (!name || !trans) {
> fprintf(stderr, "%s: args invalid\n", __func__);
> ret = -EINVAL;
> goto arg_error;
> @@ -477,9 +476,10 @@
> goto op_err;
> }
>
> - for (; n_channel > 0; n_channel--) {
> + int ch = 0;
> + for ( ; ch < n_channel; ch++) {
> ret = __lttctl_set_channel_enable(name,
> - channellist[n_channel - 1], enable);
> + channellist[ch], enable);
> if (ret)
> goto op_err_clean;
> }
> @@ -541,9 +541,10 @@
> goto op_err;
> }
>
> - for (; n_channel > 0; n_channel--) {
> + int ch = 0;
> + for ( ; ch < n_channel; ch++) {
> ret = __lttctl_set_channel_overwrite(name,
> - channellist[n_channel - 1], overwrite);
> + channellist[ch], overwrite);
> if (ret)
> goto op_err_clean;
> }
> @@ -609,9 +610,10 @@
> goto op_err;
> }
>
> - for (; n_channel > 0; n_channel--) {
> + int ch = 0;
> + for ( ; ch < n_channel; ch++) {
> ret = __lttctl_set_channel_subbuf_num(name,
> - channellist[n_channel - 1], subbuf_num);
> + channellist[ch], subbuf_num);
> if (ret)
> goto op_err_clean;
> }
> @@ -677,9 +679,10 @@
> goto op_err;
> }
>
> - for (; n_channel > 0; n_channel--) {
> + int ch = 0;
> + for ( ; ch < n_channel; ch++) {
> ret = __lttctl_set_channel_subbuf_size(name,
> - channellist[n_channel - 1], subbuf_size);
> + channellist[ch], subbuf_size);
> if (ret)
> goto op_err_clean;
> }
> @@ -745,9 +748,10 @@
> goto op_err;
> }
>
> - for (; n_channel > 0; n_channel--) {
> + int ch = 0;
> + for ( ; ch < n_channel; ch++) {
> ret = __lttctl_set_channel_switch_timer(name,
> - channellist[n_channel - 1], switch_timer);
> + channellist[ch], switch_timer);
> if (ret)
> goto op_err_clean;
> }
> @@ -769,6 +773,9 @@
> char mnt_type[PATH_MAX];
> int trymount_done = 0;
>
> + if (!mntdir)
> + return -EINVAL;
> +
> FILE *fp = fopen("/proc/mounts", "r");
> if (!fp)
> return -EINVAL;
> @@ -779,16 +786,25 @@
> break;
>
> if (!strcmp(mnt_type, "debugfs")) {
> + // 4 for the LTT_PATH "/ltt", 9 for the LTT_CONTROL_PATH "/control/"
> + // 9 for the LTT_CHANNEL_PATH "/channel/", 13 for the LTT_BUFFERS_TIMER_PATH "/switch_timer"
> + // NAME_MAX for the trace name and also for the channel name
> + // This check ensures sprintf of a debugfs path will never overrun its target char x[PATH_MAX]
> + if (strlen(mnt_dir) >= (PATH_MAX - (4 + 9 + 9 + 13 + 2*NAME_MAX))) {
> + fclose(fp);
> + return -ENOENT;
> + }
> strcpy(mntdir, mnt_dir);
> + fclose(fp);
> return 0;
> }
> }
>
> if (!trymount_done) {
> - mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, NULL);
> trymount_done = 1;
> - goto find_again;
> + if (!mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, NULL))
> + goto find_again;
> }
> -
> + fclose(fp);
> return -ENOENT;
> }
> diff --git a/lttctl/lttctl.c b/lttctl/lttctl.c
> index e77b7f3..5aadcd0 100644
> --- a/lttctl/lttctl.c
> +++ b/lttctl/lttctl.c
> @@ -28,16 +28,16 @@
> #include <config.h>
> #endif
>
> -#include <liblttctl/lttctl.h>
> -#include <errno.h>
> +#define _GNU_SOURCE
> #include <stdio.h>
> #include <stdlib.h>
> -#include <sys/wait.h>
> -#include <unistd.h>
> +#include <errno.h>
> #include <string.h>
> +#include <unistd.h>
> #include <limits.h>
> -#define _GNU_SOURCE
> #include <getopt.h>
> +#include <sys/wait.h>
> +#include <liblttctl/lttctl.h>
>
> #define OPT_MAX (1024)
> #define OPT_NAMELEN (256)
> @@ -64,7 +64,7 @@
> struct lttctl_option *next;
> };
>
> -struct lttctl_option *opt_head, *last_opt;
> +static struct lttctl_option *opt_head, *last_opt;
>
> static int opt_create;
> static int opt_destroy;
> @@ -135,10 +135,10 @@
> printf(" channel.<channelname>.overwrite=\n");
> printf(" channel.<channelname>.bufnum=\n");
> printf(" channel.<channelname>.bufsize= (in bytes, rounded to "
> - "next power of 2)\n");
> + "next power of 2)\n");
> printf(" <channelname> can be set to all for all channels\n");
> printf(" channel.<channelname>.switch_timer= (timer interval in "
> - "ms)\n");
> + "ms)\n");
> printf("\n");
> printf(" Integration options:\n");
> printf(" -C, --create_start\n");
> @@ -236,10 +236,12 @@
>
> if (!opt_head) {
> opt_head = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));
> - init_channel_opt(&opt_head->opt_mode.chan_opt, opt_name);
> - opt_head->type = CHANNEL;
> - opt_head->next = NULL;
> - last_opt = opt_head;
> + if (opt_head) {
> + init_channel_opt(&opt_head->opt_mode.chan_opt, opt_name);
> + opt_head->type = CHANNEL;
> + opt_head->next = NULL;
> + last_opt = opt_head;
> + }
> return opt_head;
> }
>
> @@ -251,15 +253,17 @@
> }
>
> new_opt = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));
> - init_channel_opt(&new_opt->opt_mode.chan_opt, opt_name);
> - new_opt->type = CHANNEL;
> - new_opt->next = NULL;
> - last_opt->next = new_opt;
> - last_opt = new_opt;
> + if (new_opt) {
> + init_channel_opt(&new_opt->opt_mode.chan_opt, opt_name);
> + new_opt->type = CHANNEL;
> + new_opt->next = NULL;
> + last_opt->next = new_opt;
> + last_opt = new_opt;
> + }
> return new_opt;
> }
>
> -int set_channel_opt(struct channel_option *opt, char *opt_name, char *opt_valstr)
> +static int set_channel_opt(struct channel_option *opt, char *opt_name, char *opt_valstr)
> {
> int opt_val, ret;
>
> @@ -268,10 +272,10 @@
> return -EINVAL;
> }
> if (opt_valstr[0] == 'Y' || opt_valstr[0] == 'y'
> - || opt_valstr[0] == '1')
> + || opt_valstr[0] == '1')
> opt_val = 1;
> else if (opt_valstr[0] == 'N' || opt_valstr[0] == 'n'
> - || opt_valstr[0] == '0')
> + || opt_valstr[0] == '0')
> opt_val = 0;
> else {
> return -EINVAL;
> @@ -284,10 +288,10 @@
> return -EINVAL;
> }
> if (opt_valstr[0] == 'Y' || opt_valstr[0] == 'y'
> - || opt_valstr[0] == '1')
> + || opt_valstr[0] == '1')
> opt_val = 1;
> else if (opt_valstr[0] == 'N' || opt_valstr[0] == 'n'
> - || opt_valstr[0] == '0')
> + || opt_valstr[0] == '0')
> opt_val = 0;
> else {
> return -EINVAL;
> @@ -377,8 +381,9 @@
>
> if (!strcmp("channel", name1)) {
> opt = find_insert_channel_opt(name2);
> + if (!opt) return -ENOMEM; //malloc failure
> if ((ret = set_channel_opt(&opt->opt_mode.chan_opt,
> - name3, opt_valstr) != 0)) {
> + name3, opt_valstr) != 0)) {
> fprintf(stderr, "Option name error2: %s\n", optarg);
> return ret;
> }
> @@ -402,20 +407,20 @@
> int ret = 0;
>
> static struct option longopts[] = {
> - {"create", no_argument, NULL, 'c'},
> - {"destroy", no_argument, NULL, 'd'},
> - {"start", no_argument, NULL, 's'},
> - {"pause", no_argument, NULL, 'p'},
> - {"help", no_argument, NULL, 'h'},
> + {"create", no_argument, NULL, 'c'},
> + {"destroy", no_argument, NULL, 'd'},
> + {"start", no_argument, NULL, 's'},
> + {"pause", no_argument, NULL, 'p'},
> + {"help", no_argument, NULL, 'h'},
> {"transport", required_argument, NULL, 2},
> - {"option", required_argument, NULL, 'o'},
> + {"option", required_argument, NULL, 'o'},
> {"create_start", no_argument, NULL, 'C'},
> {"pause_destroy", no_argument, NULL, 'D'},
> - {"write", required_argument, NULL, 'w'},
> - {"append", no_argument, NULL, 'a'},
> + {"write", required_argument, NULL, 'w'},
> + {"append", no_argument, NULL, 'a'},
> {"dump_threads", required_argument, NULL, 'n'},
> {"channel_root", required_argument, NULL, 3},
> - { NULL, 0, NULL, 0 },
> + { NULL, 0, NULL, 0 },
> };
>
> /*
> @@ -629,8 +634,8 @@
>
> if (opt->enable != -1) {
> if ((ret = lttctl_set_channel_enable(opt_tracename,
> - opt->chan_name,
> - opt->enable)) != 0)
> + opt->chan_name,
> + opt->enable)) != 0)
> return ret;
> }
> if (opt->overwrite != -1) {
> @@ -641,19 +646,20 @@
> }
> if (opt->bufnum != -1) {
> if ((ret = lttctl_set_channel_subbuf_num(opt_tracename,
> - opt->chan_name,
> - opt->bufnum)) != 0)
> + opt->chan_name,
> + opt->bufnum)) != 0)
> return ret;
> }
> if (opt->bufsize != -1) {
> if ((ret = lttctl_set_channel_subbuf_size(opt_tracename,
> - opt->chan_name,
> - opt->bufsize)) != 0)
> + opt->chan_name,
> + opt->bufsize)) != 0)
> return ret;
> }
> if (opt->switch_timer != -1) {
> if ((ret = lttctl_set_channel_switch_timer(opt_tracename,
> - opt->chan_name, opt->switch_timer)) != 0)
> + opt->chan_name,
> + opt->switch_timer)) != 0)
> return ret;
> }
> ----------------------------------------------------------------------
>
> 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
More information about the lttng-dev
mailing list