I think it will not compile either, there is a right parenthesis missing, hehe<br>
<br><br><div class="gmail_quote">2011/12/8 David Goulet <span dir="ltr"><<a href="mailto:david.goulet@polymtl.ca">david.goulet@polymtl.ca</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
You do realize that the code below will return ENOMEM if the malloc *succeeds*... :P<br>
<br>
Funny boy!<br>
<font color="#888888"><br>
David<br>
</font><br>
P.S : It might be me, I did not had my second coffee.<br>
<div><div></div><div class="h5"><br>
On 11-12-08 11:53 AM, Matthew Khouzam wrote:<br>
> wow! sorry about that last post<br>
><br>
> if (opt_head = (struct lttctl_option *)malloc(sizeof(struct lttctl_option))<br>
> {<br>
> return enomem;<br>
> }<br>
><br>
> I would be lost without a compiler.<br>
><br>
><br>
> On 11-12-08 10:29 AM, Matthew Khouzam wrote:<br>
>> Please correct me if I'm wrong, but shouldn't the failure to malloc<br>
>> return an enomem like :<br>
>> if opt_head = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));<br>
>> {<br>
>> return enomem;<br>
>> }<br>
>> because it is hiding a larger more system destabilizing problem.<br>
>><br>
>> please others comment.<br>
>> On 11-12-07 03:27 PM, Thibault, Daniel wrote:<br>
>>> ------------------------------<br>
>>>> Date: Tue, 6 Dec 2011 12:04:26 -0500<br>
>>>> From: Mathieu Desnoyers <<a href="mailto:compudj@krystal.dyndns.org">compudj@krystal.dyndns.org</a>><br>
>>>><br>
>>>> If you can split out the obvious bugfixes from the refactoring/rewrite,<br>
>>>> I would gladly accept the bugfixes.<br>
>>> ------------------------------<br>
>>><br>
>>> All right, here is a minimal patch for lttctl.c:<br>
>>><br>
>>> ------------------------------<br>
>>> --- ../ltt-control-0.89-792f03c/lttctl/lttctl.c 2011-05-12 08:44:27.000000000 -0400<br>
>>> +++ ../ltt-control-0.89-792f03c.minimalchanges/lttctl/lttctl.c 2011-12-07 15:18:28.000000000 -0500<br>
>>> @@ -64,7 +64,7 @@<br>
>>> struct lttctl_option *next;<br>
>>> };<br>
>>><br>
>>> -struct lttctl_option *opt_head, *last_opt;<br>
>>> +static struct lttctl_option *opt_head, *last_opt;<br>
>>><br>
>>> static int opt_create;<br>
>>> static int opt_destroy;<br>
>>> @@ -236,9 +236,11 @@<br>
>>><br>
>>> if (!opt_head) {<br>
>>> opt_head = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));<br>
>>> + if (opthead) {<br>
>>> init_channel_opt(&opt_head->opt_mode.chan_opt, opt_name);<br>
>>> opt_head->type = CHANNEL;<br>
>>> opt_head->next = NULL;<br>
>>> + }<br>
>>> last_opt = opt_head;<br>
>>> return opt_head;<br>
>>> }<br>
>>> @@ -251,15 +253,17 @@<br>
>>> }<br>
>>><br>
>>> new_opt = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));<br>
>>> + if (new_opt) {<br>
>>> init_channel_opt(&new_opt->opt_mode.chan_opt, opt_name);<br>
>>> new_opt->type = CHANNEL;<br>
>>> new_opt->next = NULL;<br>
>>> last_opt->next = new_opt;<br>
>>> last_opt = new_opt;<br>
>>> + }<br>
>>> return new_opt;<br>
>>> }<br>
>>><br>
>>> -int set_channel_opt(struct channel_option *opt, char *opt_name, char *opt_valstr)<br>
>>> +static int set_channel_opt(struct channel_option *opt, char *opt_name, char *opt_valstr)<br>
>>> {<br>
>>> int opt_val, ret;<br>
>>> ------------------------------<br>
>>><br>
>>> It makes opt_head and last_opt private.<br>
>>> 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.<br>
>>> 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).<br>
>>><br>
>>> Here is a minimal patch for liblttctl.c:<br>
>>><br>
>>> ------------------------------<br>
>>> --- ../ltt-control-0.89-792f03c/liblttctl/liblttctl.c 2011-05-12 08:44:27.000000000 -0400<br>
>>> +++ ../ltt-control-0.89-792f03c.minimalchanges/liblttctl/liblttctl.c 2011-12-07 15:03:31.000000000 -0500<br>
>>> @@ -35,7 +35,7 @@<br>
>>> #include <fcntl.h><br>
>>> #include <stdlib.h><br>
>>><br>
>>> -#define MAX_CHANNEL (256)<br>
>>> +//#define MAX_CHANNEL (256)<br>
>>><br>
>>> static char debugfsmntdir[PATH_MAX];<br>
>>><br>
>>> @@ -107,11 +107,6 @@<br>
>>> {<br>
>>> int fd;<br>
>>><br>
>>> - if (!fname) {<br>
>>> - fprintf(stderr, "%s: args invalid\n", __func__);<br>
>>> - return 1;<br>
>>> - }<br>
>>> -<br>
>>> fd = open(fname, O_WRONLY);<br>
>>> if (fd == -1) {<br>
>>> fprintf(stderr, "%s: open %s failed: %s\n", __func__, fname,<br>
>>> @@ -227,6 +222,10 @@<br>
>>> continue;<br>
>>> old_list = list;<br>
>>> list = malloc(sizeof(char *) * ++nr_chan);<br>
>>> + if (!list) {<br>
>>> + nr_chan = -ENOMEM;<br>
>>> + break;<br>
>>> + }<br>
>>> memcpy(list, old_list, sizeof(*list) * (nr_chan - 1));<br>
>>> free(old_list);<br>
>>> list[nr_chan - 1] = strdup(dirent->d_name);<br>
>>> @@ -405,7 +404,7 @@<br>
>>> int ret;<br>
>>> char ctlfname[PATH_MAX];<br>
>>><br>
>>> - if (!name) {<br>
>>> + if (!name || !trans) {<br>
>>> fprintf(stderr, "%s: args invalid\n", __func__);<br>
>>> ret = -EINVAL;<br>
>>> goto arg_error;<br>
>>> @@ -477,9 +476,10 @@<br>
>>> goto op_err;<br>
>>> }<br>
>>><br>
>>> - for (; n_channel > 0; n_channel--) {<br>
>>> + int ch = 0;<br>
>>> + for ( ; ch < n_channel; ch++) {<br>
>>> ret = __lttctl_set_channel_enable(name,<br>
>>> - channellist[n_channel - 1], enable);<br>
>>> + channellist[ch], enable);<br>
>>> if (ret)<br>
>>> goto op_err_clean;<br>
>>> }<br>
>>> @@ -541,9 +541,10 @@<br>
>>> goto op_err;<br>
>>> }<br>
>>><br>
>>> - for (; n_channel > 0; n_channel--) {<br>
>>> + int ch = 0;<br>
>>> + for ( ; ch < n_channel; ch++) {<br>
>>> ret = __lttctl_set_channel_overwrite(name,<br>
>>> - channellist[n_channel - 1], overwrite);<br>
>>> + channellist[ch], overwrite);<br>
>>> if (ret)<br>
>>> goto op_err_clean;<br>
>>> }<br>
>>> @@ -609,9 +610,10 @@<br>
>>> goto op_err;<br>
>>> }<br>
>>><br>
>>> - for (; n_channel > 0; n_channel--) {<br>
>>> + int ch = 0;<br>
>>> + for ( ; ch < n_channel; ch++) {<br>
>>> ret = __lttctl_set_channel_subbuf_num(name,<br>
>>> - channellist[n_channel - 1], subbuf_num);<br>
>>> + channellist[ch], subbuf_num);<br>
>>> if (ret)<br>
>>> goto op_err_clean;<br>
>>> }<br>
>>> @@ -677,9 +679,10 @@<br>
>>> goto op_err;<br>
>>> }<br>
>>><br>
>>> - for (; n_channel > 0; n_channel--) {<br>
>>> + int ch = 0;<br>
>>> + for ( ; ch < n_channel; ch++) {<br>
>>> ret = __lttctl_set_channel_subbuf_size(name,<br>
>>> - channellist[n_channel - 1], subbuf_size);<br>
>>> + channellist[ch], subbuf_size);<br>
>>> if (ret)<br>
>>> goto op_err_clean;<br>
>>> }<br>
>>> @@ -745,9 +748,10 @@<br>
>>> goto op_err;<br>
>>> }<br>
>>><br>
>>> - for (; n_channel > 0; n_channel--) {<br>
>>> + int ch = 0;<br>
>>> + for ( ; ch < n_channel; ch++) {<br>
>>> ret = __lttctl_set_channel_switch_timer(name,<br>
>>> - channellist[n_channel - 1], switch_timer);<br>
>>> + channellist[ch], switch_timer);<br>
>>> if (ret)<br>
>>> goto op_err_clean;<br>
>>> }<br>
>>> @@ -769,6 +773,9 @@<br>
>>> char mnt_type[PATH_MAX];<br>
>>> int trymount_done = 0;<br>
>>><br>
>>> + if (!mntdir)<br>
>>> + return -EINVAL;<br>
>>> +<br>
>>> FILE *fp = fopen("/proc/mounts", "r");<br>
>>> if (!fp)<br>
>>> return -EINVAL;<br>
>>> @@ -779,14 +786,19 @@<br>
>>> break;<br>
>>><br>
>>> if (!strcmp(mnt_type, "debugfs")) {<br>
>>> + // 4 for the LTT_PATH "/ltt", 9 for the LTT_CONTROL_PATH "/control/"<br>
>>> + // 9 for the LTT_CHANNEL_PATH "/channel/", 13 for the LTT_BUFFERS_TIMER_PATH "/switch_timer"<br>
>>> + // NAME_MAX for the trace name and the channel name<br>
>>> + if (strlen(mnt_dir) >= (PATH_MAX - (4 + 9 + 9 + 13 + 2*NAME_MAX)))<br>
>>> + return -ENOENT;<br>
>>> strcpy(mntdir, mnt_dir);<br>
>>> return 0;<br>
>>> }<br>
>>> }<br>
>>><br>
>>> if (!trymount_done) {<br>
>>> - mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, NULL);<br>
>>> trymount_done = 1;<br>
>>> + if (!mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, NULL))<br>
>>> goto find_again;<br>
>>> }<br>
>>> ------------------------------<br>
>>><br>
>>> It comments out the MAX_CHANNEL define, which is unused.<br>
>>> It removes the lttctl_sendop() parameter check, which is unnecessary (this private function will always get valid arguments).<br>
>>> It handles malloc() failure in lttctl_get_channellist(); previously, if malloc() failed while growing the channellist, one immediately got a segmentation fault.<br>
>>> It fixes the missing parameter check in lttctl_set_trans().<br>
>>> 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.<br>
>>> It fixes the missing parameter check in getdebugfsmntdir().<br>
>>> 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.<br>
>>> Finally, it prevents getdebugfsmntdir() from scanning /proc/mounts uselessly when the mount command fails.<br>
>>><br>
>>> I've also written (but omitted from this patch) the following:<br>
>>><br>
>>> * 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.<br>
>>><br>
>>> * A version of lttctl_get_channellist that uses readdir_r for thread safety. Is this worthwhile?<br>
>>><br>
>>> Daniel U. Thibault<br>
>>> R & D pour la défense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)<br>
>>> Système de systèmes (SdS) / System of Systems (SoS)<br>
>>> Solutions informatiques et expérimentations (SIE) / Computing Solutions and Experimentations (CSE)<br>
>>> 2459 Boul. Pie XI Nord<br>
>>> Québec, QC G3J 1X5<br>
>>> CANADA<br>
>>> Vox : (418) 844-4000 x4245<br>
>>> Fax : (418) 844-4538<br>
>>> NAC: 918V QSDJ<br>
>>> Gouvernement du Canada / Government of Canada<br>
>>> <<a href="http://www.valcartier.drdc-rddc.gc.ca/" target="_blank">http://www.valcartier.drdc-rddc.gc.ca/</a>><br>
>>><br>
>>> _______________________________________________<br>
>>> lttng-dev mailing list<br>
>>> <a href="mailto:lttng-dev@lists.lttng.org">lttng-dev@lists.lttng.org</a><br>
>>> <a href="http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev" target="_blank">http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev</a><br>
>> _______________________________________________<br>
>> lttng-dev mailing list<br>
>> <a href="mailto:lttng-dev@lists.lttng.org">lttng-dev@lists.lttng.org</a><br>
>> <a href="http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev" target="_blank">http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev</a><br>
><br>
> _______________________________________________<br>
> lttng-dev mailing list<br>
> <a href="mailto:lttng-dev@lists.lttng.org">lttng-dev@lists.lttng.org</a><br>
> <a href="http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev" target="_blank">http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev</a><br>
><br>
<br>
_______________________________________________<br>
lttng-dev mailing list<br>
<a href="mailto:lttng-dev@lists.lttng.org">lttng-dev@lists.lttng.org</a><br>
<a href="http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev" target="_blank">http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev</a><br>
</div></div></blockquote></div><br>