[lttng-dev] [PATCH] A minimal rewrite of lttctl

[Thibault, Daniel]

> Date: Thu, 8 Dec 2011 10:29:48 -0500
> From: Matthew Khouzam <matthew.khouzam at ericsson.com>
>
> 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:
> @@ -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;
>  	}

   First, there's a typo in the patch (how did that get in there?):
+		if (opthead) {
   should be:
+		if (opt_head) {

   Second, find_insert_channel_opt() returns a NULL in case of failure, not an error code.  ENOMEM will be found in errno in this case.  It is up to parst_opt(), the only function that calls find_insert_channel_opt(), to return -ENOMEM in this case.  Which makes me realize I missed that particular check while paring down the patch.  I'll add it in the resubmit.

> ------------------------------
> Message: 3
> Date: Thu, 08 Dec 2011 10:51:13 -0500
> From: Yannick Brosseau <yannick.brosseau at gmail.com>
>
> Is it possible for you to use git format-patch and git send-email ?
> That would make the patch easier to follow

   I'll try to get that set up.  Thanks for the pointer, I did not know what to use besides diff.

> > -#define MAX_CHANNEL	(256)
> > +//#define MAX_CHANNEL	(256)
> Please don't leave commented out code.
> Also, why do you remove it?

   As I said, it appears unused.  I left it in to make backtracking easier should someone point out why that define was in there to start with (although I suspect it's just a holdover from earlier versions of lttctl).

> > @@ -107,11 +107,6 @@
> > -	if (!fname) {
> > -		fprintf(stderr, "%s: args invalid\n", __func__);
> > -		return 1;
> > -	}
> I don't see why you remove this code.

   Because this particular case cannot occur: all execution paths that reach lttctl_sendop() ensure fname will be non-null.

> > @@ -779,16 +786,24 @@
> >   	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;
> >   	}
> Please indent the code correctly

   It was indented correctly; something must have eaten a tab.  I'll check more carefully before resubmitting.

> > @@ -779,16 +786,24 @@
> > +			// 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?

   Yes, I do explain it in the comment: NAME_MAX for the trace name, and another NAME_MAX for the channel name.  The longest debugfs sub-paths are of the form "/ltt" + "/control/" + tracename + "/channel/" + channelname + "/switch_timer".  The check on debugfsmntdir's length makes sure that a PATH_MAX buffer will be able to hold any possible path (with a terminating zero at its end, hence the >= instead of just >).  Otherwise, if the debugfs mounting point and trace name (and maybe a user-defined channel name as well) are outrageously long, you can get a buffer overrun.  (Granted, this is an unlikely scenario)

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/>