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;
>
> - 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;
> }
>
> size_t hex_width(u64 v)
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index c8f362d..bb0a336 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -253,7 +253,7 @@ bool strlazymatch(const char *str, const char *pat);
> int strtailcmp(const char *s1, const char *s2);
> char *strxfrchar(char *s, char from, char to);
> unsigned long convert_unit(unsigned long value, char *unit);
> -int readn(int fd, void *buf, size_t size);
> +int readn(int fd, void *buf, ssize_t size);
>
> struct perf_event_attr;
>
>
>
--
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/
