[lttng-dev] [PATCH] A minimal rewrite of lttctl
Thibault, Daniel
Daniel.Thibault at drdc-rddc.gc.ca
Wed Dec 7 15:27:56 EST 2011
------------------------------
> 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/>
More information about the lttng-dev
mailing list