[lttng-dev] [PATCH] A minimal rewrite of lttctl
Raphaël Beamonte
raphael.beamonte at polymtl.ca
Thu Dec 8 12:25:43 EST 2011
I think it will not compile either, there is a right parenthesis missing,
hehe
2011/12/8 David Goulet <david.goulet at polymtl.ca>
> 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 <compudj at krystal.dyndns.org>
> >>>>
> >>>> 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
> >>> lttng-dev at lists.lttng.org
> >>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> >> _______________________________________________
> >> lttng-dev mailing list
> >> lttng-dev at lists.lttng.org
> >> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> >
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev at lists.lttng.org
> > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> >
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20111208/8fc48de5/attachment-0001.htm>
More information about the lttng-dev
mailing list