On Thu, 21 Dec 2017 17:25:18 +0200
"Vladislav Valtchev (VMware)" <[email protected]> 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");
