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