I think it will not compile either, there is a right parenthesis missing, hehe
2011/12/8 David Goulet <[email protected]> > You do realize that the code below will return ENOMEM if the malloc > *succeeds*... :P > > Funny boy! > > David > > P.S : It might be me, I did not had my second coffee. > > On 11-12-08 11:53 AM, Matthew Khouzam wrote: > > wow! sorry about that last post > > > > if (opt_head = (struct lttctl_option *)malloc(sizeof(struct > lttctl_option)) > > { > > return enomem; > > } > > > > I would be lost without a compiler. > > > > > > On 11-12-08 10:29 AM, Matthew Khouzam wrote: > >> Please correct me if I'm wrong, but shouldn't the failure to malloc > >> return an enomem like : > >> if opt_head = (struct lttctl_option *)malloc(sizeof(struct > lttctl_option)); > >> { > >> return enomem; > >> } > >> because it is hiding a larger more system destabilizing problem. > >> > >> please others comment. > >> On 11-12-07 03:27 PM, Thibault, Daniel wrote: > >>> ------------------------------ > >>>> Date: Tue, 6 Dec 2011 12:04:26 -0500 > >>>> From: Mathieu Desnoyers <[email protected]> > >>>> > >>>> If you can split out the obvious bugfixes from the > refactoring/rewrite, > >>>> I would gladly accept the bugfixes. > >>> ------------------------------ > >>> > >>> All right, here is a minimal patch for lttctl.c: > >>> > >>> ------------------------------ > >>> --- ../ltt-control-0.89-792f03c/lttctl/lttctl.c 2011-05-12 > 08:44:27.000000000 -0400 > >>> +++ ../ltt-control-0.89-792f03c.minimalchanges/lttctl/lttctl.c > 2011-12-07 15:18:28.000000000 -0500 > >>> @@ -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; > >>> @@ -236,9 +236,11 @@ > >>> > >>> if (!opt_head) { > >>> opt_head = (struct lttctl_option *)malloc(sizeof(struct > lttctl_option)); > >>> + if (opthead) { > >>> 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)); > >>> + 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; > >>> ------------------------------ > >>> > >>> It makes opt_head and last_opt private. > >>> In find_insert_channel_opt, it handles malloc() failure; > previously, if malloc() failed while creating a new lttctl_option list > node, one immediately got a segmentation fault. > >>> It makes set_channel_opt() private (there is no reason why it > should be exportable, and all the other functions in this unit are private). > >>> > >>> Here is a minimal patch for liblttctl.c: > >>> > >>> ------------------------------ > >>> --- ../ltt-control-0.89-792f03c/liblttctl/liblttctl.c 2011-05-12 > 08:44:27.000000000 -0400 > >>> +++ ../ltt-control-0.89-792f03c.minimalchanges/liblttctl/liblttctl.c > 2011-12-07 15:03:31.000000000 -0500 > >>> @@ -35,7 +35,7 @@ > >>> #include <fcntl.h> > >>> #include <stdlib.h> > >>> > >>> -#define MAX_CHANNEL (256) > >>> +//#define MAX_CHANNEL (256) > >>> > >>> static char debugfsmntdir[PATH_MAX]; > >>> > >>> @@ -107,11 +107,6 @@ > >>> { > >>> int fd; > >>> > >>> - if (!fname) { > >>> - fprintf(stderr, "%s: args invalid\n", __func__); > >>> - return 1; > >>> - } > >>> - > >>> fd = open(fname, O_WRONLY); > >>> if (fd == -1) { > >>> fprintf(stderr, "%s: open %s failed: %s\n", __func__, > fname, > >>> @@ -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,14 +786,19 @@ > >>> 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 the channel name > >>> + if (strlen(mnt_dir) >= (PATH_MAX - (4 + 9 + 9 + 13 > + 2*NAME_MAX))) > >>> + return -ENOENT; > >>> strcpy(mntdir, mnt_dir); > >>> return 0; > >>> } > >>> } > >>> > >>> if (!trymount_done) { > >>> - mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, NULL); > >>> trymount_done = 1; > >>> + if (!mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, > NULL)) > >>> goto find_again; > >>> } > >>> ------------------------------ > >>> > >>> It comments out the MAX_CHANNEL define, which is unused. > >>> It removes the lttctl_sendop() parameter check, which is > unnecessary (this private function will always get valid arguments). > >>> It handles malloc() failure in lttctl_get_channellist(); > previously, if malloc() failed while growing the channellist, one > immediately got a segmentation fault. > >>> It fixes the missing parameter check in lttctl_set_trans(). > >>> In each of the lttctl_set_channel_*() functions, it fixes a memory > leak: the original code would count n_channel down to zero before passing > it to lttctl_free_channellist(), so that none of the strings stored in the > channellist array were freed. > >>> It fixes the missing parameter check in getdebugfsmntdir(). > >>> It limits the length of the debugfs mounting point path, based on > the longest sub-path possible (generously using PATH_MAX for the channel > name, which in practice is limited to 13 characters). Otherwise, whenever > sprintf is used to build a debugfs path string, the string buffer could be > overrun. > >>> Finally, it prevents getdebugfsmntdir() from scanning /proc/mounts > uselessly when the mount command fails. > >>> > >>> I've also written (but omitted from this patch) the following: > >>> > >>> * A version of lttctl_sendop that handles the case where the write() > is incomplete. This can potentially occur, but is so rare (I think it > happens only if a signal interrupts the write) that it may not be > worthwhile to fix it. > >>> > >>> * A version of lttctl_get_channellist that uses readdir_r for thread > safety. Is this worthwhile? > >>> > >>> 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 > > > > _______________________________________________ > > 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 >
_______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
