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