[lttng-dev] [PATCH] A minimal rewrite of lttctl (additional)
Yannick Brosseau
yannick.brosseau at gmail.com
Thu Dec 8 10:51:13 EST 2011
Is it possible for you to use git format-patch and git send-email ?
That would make the patch easier to follow
I've added some comments in the patch bellow
On 2011-12-07 16:02, Thibault, Daniel wrote:
> Just realized the liblttctl.c patch is a little bit off. Here is the correct patch, followed by a recap of what I missed last time.
>
> --------------------
> --- ../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:55:24.000000000 -0500
> @@ -35,7 +35,7 @@
> #include<fcntl.h>
> #include<stdlib.h>
>
> -#define MAX_CHANNEL (256)
> +//#define MAX_CHANNEL (256)
Please don't leave commented out code.
Also, why do you remove it?
>
> static char debugfsmntdir[PATH_MAX];
>
> @@ -107,11 +107,6 @@
> {
> int fd;
>
> - if (!fname) {
> - fprintf(stderr, "%s: args invalid\n", __func__);
> - return 1;
> - }
> -
I don't see why you remove this code.
> 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,16 +786,24 @@
> 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))) {
> + close(fp);
> + return -ENOENT;
> + }
> strcpy(mntdir, mnt_dir);
> + close(fp);
> 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))
Please indent the code correctly
> goto find_again;
> }
> -
> + close(fp);
> return -ENOENT;
> }
> --------------------
>
> To recap, getdebugfsmntdir() was not closing the open FILE* handle fp before returning (three additional lines).
>
> --------------------
> @@ -779,16 +786,24 @@
> 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))) {
You do not explain the 2*NAME_MAX.
Also, why do you add this check?
> + close(fp);
> + return -ENOENT;
> + }
> strcpy(mntdir, mnt_dir);
> + close(fp);
> 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;
Indentation again
> }
> -
> + close(fp);
> return -ENOENT;
> }
> --------------------
>
> 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