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@xxxxxxxxx> 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