Messages in this thread Patch in this message | | | Date | Fri, 12 Jan 2018 10:13:23 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg |
| |
On Thu, 21 Dec 2017 17:25:18 +0200 "Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:
> -static char read_proc(void) > +/* > + * Returns: > + * -1 - Something went wrong > + * 0 - File does not exist (stack tracer not enabled) > + * 1 - Success > + */ > +static int read_proc(int *status) > { > - char buf[1]; > + struct stat stat_buf; > + char buf[64]; > + long num; > int fd; > int n; > > + if (stat(PROC_FILE, &stat_buf) < 0) { > + /* stack tracer not configured on running kernel */ > + *status = 0; /* not configured means disabled */ > + return 0; > + } > + > fd = open(PROC_FILE, O_RDONLY); > - if (fd < 0) > - die("reading %s", PROC_FILE); > - n = read(fd, buf, 1); > - close(fd); > - if (n != 1) > + > + if (fd < 0) { > + /* we cannot open the file: likely a permission problem. */ > + return -1; > + } > + > + n = read(fd, buf, sizeof(buf)); > + > + /* We assume that the file is never empty we got no errors. */
The above comment does not parse.
> + 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.
We can move the close right after the read up above.
> + > + /* 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.
> + 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?
> + if (!num && (errno == EINVAL || errno == ERANGE)) > + return -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.
> + > + *status = num; > + return 1; /* full success */ > } > > -static void start_stop_trace(char val) > +/* NOTE: this implementation only accepts new_status in the range [0..9]. */ > +static void change_stack_tracer_status(int new_status) > { > char buf[1]; > + int status; > int fd; > int n; > > - buf[0] = read_proc(); > - if (buf[0] == val) > - return; > + if (read_proc(&status) > 0 && status == new_status) > + return; /* nothing to do */ > > fd = open(PROC_FILE, O_WRONLY); > +
Don't add a new line here. It's common to have the error check immediately after the function.
> 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; }
> 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.
int status; int r;
-- Steve
> > - if (read_proc() == '1') > + if (read_proc(&status) <= 0) > + die("Invalid stack tracer state"); > + > + if (status > 0) > printf("(stack tracer running)\n"); > else > printf("(stack tracer not running)\n");
| |