On Fri, 2018-01-12 at 10:13 -0500, Steven Rostedt wrote:
> > +
> > + /* We assume that the file is never empty we got no errors. */
>
> The above comment does not parse.
OK, I just removed it.
>
> > + if (n <= 0)
> > die("error reading %s", PROC_FILE);
> >
> > - return buf[0];
> > + /* Does this file have more than 63 characters?? */
> > + if (n >= sizeof(buf))
> > + return -1;
>
> We need to close fd before returning, otherwise we leak a file
> descriptor.
Oops, you're totally right.
>
> We can move the close right after the read up above.
>
Yep.
> > +
> > + /* n is guaranteed to be in the range [1, sizeof(buf)-1]. */
> > + buf[n] = 0;
> > + close(fd);
> > +
> > + errno = 0;
> > +
> > + /* Read an integer from buf ignoring any non-digit trailing characters.
> > */
>
> We don't really need to comment what strtol() does ;-) That's what man
> pages are for.
>
Alright.
> > + num = strtol(buf, NULL, 10);
> > +
> > + /* strtol() returned 0: we have to check for errors */
>
> Actually, a better comment is, why would strtol return zero and this
> not be an error?
I don't understand: I'm checking exactly the case when strtol() returned 0
and that might be an error. It's not sure that there's an error, but there
might be.
It would be strange for me to read:
"why would strtol return zero and this not be an error?"
and see an IF statement which in the true-path returns -1.
> > + if (num > INT_MAX || num < INT_MIN)
> > + return -1; /* the number is good but does not fit in 'int' */
>
> Don't need the comment after the above return. The INT_MAX and INT_MIN
> are self describing.
OK
>
> Don't add a new line here. It's common to have the error check
> immediately after the function.
OK
>
> > if (fd < 0)
> > die("writing %s", PROC_FILE);
>
> If you want a new line, you can add it here.
>
> > - buf[0] = val;
> > + buf[0] = new_status + '0';
>
> If you are paranoid, we can make new_status unsigned int, or even
> unsigned char, and add at the beginning of the function:
>
> if (new_status > 9) {
> warning("invalid status %d\n", new_status);
> return;
> }
The problem with that is that we agreed the value in the proc file
might also be negative. That's why new_status should be an int.
So, what a check like that:
if (new_status < 0 || new_status > 9) {
warning("invalid status %d\n", new_status);
return;
}
>
> > n = write(fd, buf, 1);
> > if (n < 0)
> > die("writing into %s", PROC_FILE);
> > @@ -88,12 +131,12 @@ static void start_stop_trace(char val)
> >
> > static void start_trace(void)
> > {
> > - start_stop_trace('1');
> > + change_stack_tracer_status(1);
> > }
> >
> > static void stop_trace(void)
> > {
> > - start_stop_trace('0');
> > + change_stack_tracer_status(0);
> > }
> >
> > static void reset_trace(void)
> > @@ -123,8 +166,12 @@ static void read_trace(void)
> > char *buf = NULL;
> > size_t n;
> > int r;
> > + int status;
>
> Remember, upside down x-mas trees.
Sure.
--
Vladislav Valtchev
VMware Open Source Technology Center
