[PATCH v2 0/3] trace-cmd: Integrate stack tracer status in 'stat'

[PATCH v2 0/3] trace-cmd: Integrate stack tracer status in 'stat'

[Vladislav Valtchev (VMware)]

This short patch series makes trace-cmd stat aware of the stack tracer: now,
when the stack tracker is ON, the stat command will report that.

Vladislav Valtchev (VMware) (3):
  trace-cmd: Make read_proc() to return int status via OUT arg
  trace-cmd: Remove the die() call from read_proc()
  trace-cmd: Making stat to report when the stack tracer is ON

 trace-cmd.h   |  2 ++
 trace-stack.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 trace-stat.c  |  8 ++++++
 3 files changed, 79 insertions(+), 16 deletions(-)

-- 
2.14.1

[PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg

[Vladislav Valtchev (VMware)]

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(-)

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");
-- 
2.14.1

[PATCH v2 2/3] trace-cmd: Remove the die() call from read_proc()

[Vladislav Valtchev (VMware)]

As trace-stack.c's read_proc() function is going to be used by trace-cmd stat,
we don't want it to make the program die in case something went wrong.
Therefore, this simple patch makes read_proc() to just return -1 in case the
proc file was empty or read() failed with an error, instead of using die().

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-stack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/trace-stack.c b/trace-stack.c
index c1058ca..d55d994 100644
--- a/trace-stack.c
+++ b/trace-stack.c
@@ -79,9 +79,9 @@ static int read_proc(int *status)
 
 	n = read(fd, buf, sizeof(buf));
 
-	/* We assume that the file is never empty we got no errors. */
+	/* The file was empty or read() failed with an error. */
 	if (n <= 0)
-		die("error reading %s", PROC_FILE);
+		return -1;
 
 	/* Does this file have more than 63 characters?? */
 	if (n >= sizeof(buf))
-- 
2.14.1

[PATCH v2 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Vladislav Valtchev (VMware)]

trace-cmd stat is a handy way for users to see what tracing is currently going
on, but currently it does not say anything about the stack tracing. This patch
makes the command to show a message when the stack tracer is ON.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-cmd.h   | 2 ++
 trace-stack.c | 6 ++++++
 trace-stat.c  | 8 ++++++++
 3 files changed, 16 insertions(+)

diff --git a/trace-cmd.h b/trace-cmd.h
index 6fd34d7..9704b2e 100644
--- a/trace-cmd.h
+++ b/trace-cmd.h
@@ -358,6 +358,8 @@ void tracecmd_free_hooks(struct hook_list *hooks);
 /* --- Hack! --- */
 int tracecmd_blk_hack(struct tracecmd_input *handle);
 
+/* --- Stack tracer functions --- */
+int tracecmd_stack_tracer_status(int *status);
 
 /* --- Debugging --- */
 struct kbuffer *tracecmd_record_kbuf(struct tracecmd_input *handle,
diff --git a/trace-stack.c b/trace-stack.c
index d55d994..0028ecc 100644
--- a/trace-stack.c
+++ b/trace-stack.c
@@ -107,6 +107,12 @@ static int read_proc(int *status)
 	return 1; /* full success */
 }
 
+/* Public wrapper of read_proc() */
+int tracecmd_stack_tracer_status(int *status)
+{
+	return read_proc(status);
+}
+
 /* NOTE: this implementation only accepts new_status in the range [0..9]. */
 static void change_stack_tracer_status(int new_status)
 {
diff --git a/trace-stat.c b/trace-stat.c
index fd16354..61dd41f 100644
--- a/trace-stat.c
+++ b/trace-stat.c
@@ -894,6 +894,7 @@ void trace_stat (int argc, char **argv)
 	struct buffer_instance *instance = &top_instance;
 	int topt = 0;
 	int c;
+	int stack_tracer_status;
 
 	for (;;) {
 		c = getopt(argc-1, argv+1, "tB:");
@@ -928,5 +929,12 @@ void trace_stat (int argc, char **argv)
 		stat_instance(instance);
 	}
 
+	if (tracecmd_stack_tracer_status(&stack_tracer_status) >= 0) {
+		if (stack_tracer_status > 0)
+			printf("Stack tracing is enabled\n\n");
+	} else {
+		printf("The status of the stack tracer is indeterminate\n\n");
+	}
+
 	exit(0);
 }
-- 
2.14.1

Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg

[Steven Rostedt]

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");

Re: [PATCH v2 2/3] trace-cmd: Remove the die() call from read_proc()

[Steven Rostedt]

On Thu, 21 Dec 2017 17:25:19 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:

> As trace-stack.c's read_proc() function is going to be used by trace-cmd stat,
> we don't want it to make the program die in case something went wrong.
> Therefore, this simple patch makes read_proc() to just return -1 in case the
> proc file was empty or read() failed with an error, instead of using die().
> 
> Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
> ---
>  trace-stack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/trace-stack.c b/trace-stack.c
> index c1058ca..d55d994 100644
> --- a/trace-stack.c
> +++ b/trace-stack.c
> @@ -79,9 +79,9 @@ static int read_proc(int *status)
>  
>  	n = read(fd, buf, sizeof(buf));
>  
> -	/* We assume that the file is never empty we got no errors. */
> +	/* The file was empty or read() failed with an error. */
>  	if (n <= 0)
> -		die("error reading %s", PROC_FILE);
> +		return -1;
>  
>  	/* Does this file have more than 63 characters?? */
>  	if (n >= sizeof(buf))

But you need to handle the error cases for the users of read_proc().
>From the previous patch:

 static void change_stack_tracer_status(int new_status)
 {
	char buf[1];
	int status;
	int fd;
	int n;
 
	if (read_proc(&status) > 0 && status == new_status)
		return; /* nothing to do */

We should not continue if read_proc() fails. Should move the die here:

	ret = read_proc(&status);
	if (ret < 0)
		die("error reading %s", PROC_FILE);

	if (ret > 0 && status == new_status)
		return; /* nothing to do */

-- Steve

 
 	fd = open(PROC_FILE, O_WRONLY);
 	if (fd < 0)
 		die("writing %s", PROC_FILE);

	buf[0] = new_status + '0';
 	n = write(fd, buf, 1);

Re: [PATCH v2 3/3] trace-cmd: Making stat to report when the stack tracer is ON

[Steven Rostedt]

On Thu, 21 Dec 2017 17:25:20 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:

> trace-cmd stat is a handy way for users to see what tracing is currently going
> on, but currently it does not say anything about the stack tracing. This patch
> makes the command to show a message when the stack tracer is ON.
> 
> Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
> ---
>  trace-cmd.h   | 2 ++
>  trace-stack.c | 6 ++++++
>  trace-stat.c  | 8 ++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/trace-cmd.h b/trace-cmd.h
> index 6fd34d7..9704b2e 100644
> --- a/trace-cmd.h
> +++ b/trace-cmd.h
> @@ -358,6 +358,8 @@ void tracecmd_free_hooks(struct hook_list *hooks);
>  /* --- Hack! --- */
>  int tracecmd_blk_hack(struct tracecmd_input *handle);
>  
> +/* --- Stack tracer functions --- */
> +int tracecmd_stack_tracer_status(int *status);
>  
>  /* --- Debugging --- */
>  struct kbuffer *tracecmd_record_kbuf(struct tracecmd_input *handle,
> diff --git a/trace-stack.c b/trace-stack.c
> index d55d994..0028ecc 100644
> --- a/trace-stack.c
> +++ b/trace-stack.c
> @@ -107,6 +107,12 @@ static int read_proc(int *status)
>  	return 1; /* full success */
>  }
>  
> +/* Public wrapper of read_proc() */
> +int tracecmd_stack_tracer_status(int *status)
> +{
> +	return read_proc(status);
> +}
> +
>  /* NOTE: this implementation only accepts new_status in the range [0..9]. */
>  static void change_stack_tracer_status(int new_status)
>  {
> diff --git a/trace-stat.c b/trace-stat.c
> index fd16354..61dd41f 100644
> --- a/trace-stat.c
> +++ b/trace-stat.c
> @@ -894,6 +894,7 @@ void trace_stat (int argc, char **argv)
>  	struct buffer_instance *instance = &top_instance;
>  	int topt = 0;
>  	int c;
> +	int stack_tracer_status;

Needs to be upside down x-mas tree. (can't you feel the season?) ;-)

Not to mention, it's an awfully verbose variable name. Just call it
"status".

>  
>  	for (;;) {
>  		c = getopt(argc-1, argv+1, "tB:");
> @@ -928,5 +929,12 @@ void trace_stat (int argc, char **argv)
>  		stat_instance(instance);
>  	}
>  
> +	if (tracecmd_stack_tracer_status(&stack_tracer_status) >= 0) {
> +		if (stack_tracer_status > 0)
> +			printf("Stack tracing is enabled\n\n");
> +	} else {
> +		printf("The status of the stack tracer is indeterminate\n\n");

I wonder if we should report saying there was an error?

	printf("Error reading stack tracer status\n");

-- Steve

> +	}
> +
>  	exit(0);
>  }

Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg

[Vladislav Valtchev]

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

Re: [PATCH v2 2/3] trace-cmd: Remove the die() call from read_proc()

[Vladislav Valtchev]

On Fri, 2018-01-12 at 10:43 -0500, Steven Rostedt wrote:
> 
> But you need to handle the error cases for the users of read_proc().
> From the previous patch:
> 
>  static void change_stack_tracer_status(int new_status)
>  {
> 	char buf[1];
> 	int status;
> 	int fd;
> 	int n;
>  
> 	if (read_proc(&status) > 0 && status == new_status)
> 		return; /* nothing to do */
> 
> We should not continue if read_proc() fails. Should move the die here:
> 
> 	ret = read_proc(&status);
> 	if (ret < 0)
> 		die("error reading %s", PROC_FILE);
> 
> 	if (ret > 0 && status == new_status)
> 		return; /* nothing to do */
> 
> -- Steve
> 

You're totally right. I overlooked at this detail.

-- 
Vladislav Valtchev
VMware Open Source Technology Center

Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg

[Steven Rostedt]

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

Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg

[Vladislav Valtchev]

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.



-- 
Vladislav Valtchev
VMware Open Source Technology Center

Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg

[Steven Rostedt]

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