| Message ID | 20171221152520.25867-2-vladislav.valtchev@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | trace-cmd: Integrate stack tracer status in 'stat' | expand |
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");
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.
On Tue, 16 Jan 2018 16:47:33 +0200 Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote: > > > > + 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. :-) That was totally lost in translation. :-) No, I didn't mean to have a comment literally saying "why would strtol return zero and this not be an error", I meant for the comment to explain it. Actually, looking at the man page which states: ==== RETURN VALUE The strtol() function returns the result of the conversion, unless the value would underflow or overflow. If an underflow occurs, strtol() returns LONG_MIN. If an overflow occurs, strtol() returns LONG_MAX. In both cases, errno is set to ERANGE. Precisely the same holds for strtoll() (with LLONG_MIN and LLONG_MAX instead of LONG_MIN and LONG_MAX). ==== Which means that zero isn't enough to check. It also shows the following example: ==== errno = 0; /* To distinguish success/failure after call */ val = strtol(str, &endptr, base); /* Check for various possible errors */ if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) || (errno != 0 && val == 0)) { perror("strtol"); exit(EXIT_FAILURE); } ==== I say we simply remove the comment. Or say what the man page example says: /* Check for various possible errors */ and leave it at that. > > > > > 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; > } Sure it could be negative. The point was, you don't want it to be if you do: buf[0] = new_status + '0'; As that will break if new_status is negative or greater than 9. Also, whether you use unsigned, or do the above, they both have the same result. A negative produces a warning. Which is fine. As long as it doesn't kill the program. It's only an implementation detail. That is, using unsigned char as new_status, and checking if (new_status > 9) Is no different than using int and checking if (new_status < 0 || new_status > 9) except that you use more instructions to accomplish the same thing. -- Steve
On Tue, 2018-01-16 at 11:27 -0500, Steven Rostedt wrote: > > > :-) That was totally lost in translation. :-) > > No, I didn't mean to have a comment literally saying "why would strtol > return zero and this not be an error", I meant for the comment to > explain it. > > Actually, looking at the man page which states: > Yep, I got it. Sometimes I interpret words too literally. My fault :-) > I say we simply remove the comment. Or say what the man page example > says: > > /* Check for various possible errors */ > > and leave it at that. Sure, "Check for various possible errors" sounds good to me. > > Sure it could be negative. The point was, you don't want it to be if > you do: > > buf[0] = new_status + '0'; > > As that will break if new_status is negative or greater than 9. > > Also, whether you use unsigned, or do the above, they both have the > same result. A negative produces a warning. Which is fine. As long as > it doesn't kill the program. It's only an implementation detail. > > That is, using unsigned char as new_status, and checking > > if (new_status > 9) > > Is no different than using int and checking > > if (new_status < 0 || new_status > 9) > > except that you use more instructions to accomplish the same thing. > Sure, using two checks with 'int' is less efficient then using the 'unsigned trick', but my point is that such a function (at interface level) should accept exactly the same type 'returned' (via OUT param) by read_proc(). It should be symmetric, as if instead of 'int/unsigned' we used an opaque type 'value_t' for which we cannot make assumptions. Clearly, the implementation may in practice accept a subset of the values allowed by the parameter type. What about accepting 'int' but doing the check this way: if ((unsigned)new_status > 9) { warning(...); return; } This way, we'll keep the interface symmetric (with read_proc()) but, at the same time, we use a more efficient check.
On Tue, 16 Jan 2018 20:49:22 +0200 Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote: > Sure, using two checks with 'int' is less efficient then using the 'unsigned trick', > but my point is that such a function (at interface level) should accept exactly > the same type 'returned' (via OUT param) by read_proc(). It should be symmetric, > as if instead of 'int/unsigned' we used an opaque type 'value_t' for which we cannot > make assumptions. Clearly, the implementation may in practice accept a subset of the values > allowed by the parameter type. > > What about accepting 'int' but doing the check this way: > > if ((unsigned)new_status > 9) { > warning(...); > return; > } > > This way, we'll keep the interface symmetric (with read_proc()) but, at the same time, > we use a more efficient check. The thing is, since we only accept new_status to be 0-9, it can never be negative. Thus we should make it unsigned. Yes read_proc() is signed, but that's because it can be negative. We are keeping the variables the same as the values they represent, nothing more. One variable shouldn't affect what the other variable is, as the ranges are indeed different. -- Steve
diff --git a/trace-stack.c b/trace-stack.c index aa79ae3..c1058ca 100644 --- a/trace-stack.c +++ b/trace-stack.c @@ -20,6 +20,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <limits.h> #include <getopt.h> #include <sys/types.h> #include <sys/stat.h> @@ -49,37 +50,79 @@ static void test_available(void) die("stack tracer not configured on running kernel"); } -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. */ + 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; + + /* 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. */ + num = strtol(buf, NULL, 10); + + /* strtol() returned 0: we have to check for errors */ + 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' */ + + *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); + if (fd < 0) die("writing %s", PROC_FILE); - buf[0] = val; + buf[0] = new_status + '0'; 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; - 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");
This patch changes both the implementation and the interface of read_proc() in trace-stack.c. First, it makes the function to read a string from the proc file and then parse it as an integer using strtol(). Then, it makes the function to return the integer read from the proc file using the int *status OUT parameter, in order to make possible its return value to be used by the caller to check if the operation succeeded. This new implementation relaxes the external contraints the function relies on, making it possible to be used by trace stat. The point is that 'stat' should not fail in case there is something wrong with the stack tracer. Only the call to die() in case the file is empty has been left in this patch: it will be removed as well in a separate commit. Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com> --- trace-stack.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 16 deletions(-)