Re: [PATCH] tools/perf/util: Document and clean up readn() a bit

[Ingo Molnar <mingo@kernel.org>]

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> On 26/11/13 19:53, Ingo Molnar wrote:
> > 
> > * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> > 
> >> Em Fri, Nov 22, 2013 at 03:24:26PM +0100, Jiri Olsa escreveu:
> >>>  }
> >>> +
> >>> +ssize_t perf_data_file__write(struct perf_data_file *file,
> >>> +			      void *buf, size_t size)
> >>> +{
> >>> +	ssize_t total = size;
> >>> +
> >>> +	while (size) {
> >>> +		ssize_t ret = write(file->fd, buf, size);
> >>> +
> >>> +		if (ret < 0) {
> >>> +			pr_err("failed to write perf data, error: %m\n");
> >>> +			return -1;
> >>> +		}
> >>> +
> >>> +		size -= ret;
> >>> +		buf  += ret;
> >>> +	}
> >>> +
> >>> +	return total;
> >>
> >> So this is the functional equivalent of "readn", so please move it to
> >> just after "readn" and make this just a simple wrapper.
> > 
> > Btw., would be nice to add a small comment to readn() that describes 
> > its semantics, it looks like a useful helper.
> > 
> > I also added a check for the input parameter 'n', plus I added a 
> > 'left' variable to make the flow clearer, and added a debug check for 
> > the return value - I think returning 'n' is more obvious.
> 
> It would be nicer to match what 'read' does.
> 
> > 
> > Totally untested though.
> > 
> > Thanks,
> > 
> > 	Ingo
> > 
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > 
> > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > index 28a0a89..4789081 100644
> > --- a/tools/perf/util/util.c
> > +++ b/tools/perf/util/util.c
> > @@ -6,6 +6,7 @@
> >  #endif
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > +#include <linux/kernel.h>
> >  
> >  /*
> >   * XXX We need to find a better place for these things...
> > @@ -151,21 +152,29 @@ unsigned long convert_unit(unsigned long value, char *unit)
> >  	return value;
> >  }
> >  
> > -int readn(int fd, void *buf, size_t n)
> > +/*
> > + * Read exactly 'n' bytes or return an error:
> > + */
> > +int readn(int fd, void *buf, ssize_t n)
> 
> Should really be the same prototype as 'read' i.e.
> 
> 	ssize_t readn(int fd, void *buf, size_t n)
> 
> Need to change callers that are using 'int' too.
> 
> >  {
> >  	void *buf_start = buf;
> > +	size_t left = n;
> > +
> > +	BUG_ON(n <= 0);
> 
> 	BUG_ON(n == 0 || n > SSIZE_MAX);
> 
> >  
> > -	while (n) {
> > -		int ret = read(fd, buf, n);
> > +	while (left) {
> > +		int ret = read(fd, buf, left);
> 
> Should use the correct return type:
> 
> 		ssize_t ret = read(fd, buf, left);
> 
> >  
> >  		if (ret <= 0)
> >  			return ret;
> 
> Don't return 0 if not all the bytes were read:
> 
> 		if (ret < 0)
> 			return ret;
> 		if (!ret)
> 			break;

Okay, I thought this was an intentional 'all or nothing' interface - 
but looking at the readn() users they can tolerate partial results 
just fine.

> 
> >  
> > -		n -= ret;
> > +		left -= ret;
> >  		buf += ret;
> >  	}
> >  
> > -	return buf - buf_start;
> > +	BUG_ON(buf-buf_start != n);
> > +
> > +	return n;
> 
> Should return the same value as 'read' i.e the original
> 
> 	return buf - buf_start;

It does, in the original version I patched buf-buf_start == n, see the 
assert that checks for that.

If partial reads are returned then this bit has to change too, yes.

Thanks,

	Ingo