lkml.org 
[lkml]   [2013]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] tools/perf/util: Document and clean up readn() a bit

* 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


\
 
 \ /
  Last update: 2013-11-27 13:21    [W:0.104 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site