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 <[email protected]> > 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 > [email protected] > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
