* Adrian Hunter <adrian.hun...@intel.com> wrote:
> On 26/11/13 19:53, Ingo Molnar wrote:
> >
> > * Arnaldo Carvalho de Melo <a...@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 <mi...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
