Re: [PATCH 3/4] samples/bpf: Check the error of write() and read()

[Taeung Song]

Hi David Laight,

On 07/02/2018 06:25 PM, David Laight wrote:
> From: Taeung Song
> > Sent: 02 July 2018 10:15
> > test_task_rename() and test_urandom_read()
> > can be failed during write() and read(),
> > So check the result of them.
> >
> > Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
> > ---
> >   samples/bpf/test_overhead_user.c | 20 +++++++++++++++-----
> >   1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/samples/bpf/test_overhead_user.c b/samples/bpf/test_overhead_user.c
> > index 6caf47a..8a88d9c 100644
> > --- a/samples/bpf/test_overhead_user.c
> > +++ b/samples/bpf/test_overhead_user.c
> > @@ -6,6 +6,7 @@
> >    */
> >   #define _GNU_SOURCE
> >   #include <sched.h>
> > +#include <errno.h>
> >   #include <stdio.h>
> >   #include <sys/types.h>
> >   #include <asm/unistd.h>
> > @@ -44,8 +45,12 @@ static void test_task_rename(int cpu)
> >   		exit(1);
> >   	}
> >   	start_time = time_get_ns();
> > -	for (i = 0; i < MAX_CNT; i++)
> > -		write(fd, buf, sizeof(buf));
> > +	for (i = 0; i < MAX_CNT; i++) {
> > +		if (write(fd, buf, sizeof(buf)) < 0) {
> > +			printf("task rename failed: %s\n", strerror(errno));
> > +			break;
>
> I'm not sure 'break' generates sensible output.

OK,
it seems to be better to off on the above error case instead of break;

> > +		}
> > +	}
>
> What about partial writes??
Hum..
do you mean just skipping several errors of the above write() ?

> >   	printf("task_rename:%d: %lld events per sec\n",
> >   	       cpu, MAX_CNT * 1000000000ll / (time_get_ns() - start_time));
> >   	close(fd);
> > @@ -55,7 +60,7 @@ static void test_urandom_read(int cpu)
> >   {
> >   	__u64 start_time;
> >   	char buf[4];
> > -	int i, fd;
> > +	int i, fd, err = 0;
> >
> >   	fd = open("/dev/urandom", O_RDONLY);
> >   	if (fd < 0) {
> > @@ -63,8 +68,13 @@ static void test_urandom_read(int cpu)
> >   		exit(1);
> >   	}
> >   	start_time = time_get_ns();
> > -	for (i = 0; i < MAX_CNT; i++)
> > -		read(fd, buf, sizeof(buf));
> > +	for (i = 0; i < MAX_CNT; i++) {
> > +		err = read(fd, buf, sizeof(buf));
> > +		if (err < 0 || err >= sizeof(buf)) {
>
> Overlong reads indicate that something is seriously awry.
> Short reads are valid - but maybe not expected.
> > +			printf("failed to read from /dev/urandom: %s\n", strerror(errno));
>
>
> strerror() won't give anything sensible unless err == -1;
> You probably want to include the loop count.
>
> > +			break;
>
> The summary print will be gibberish after break.

Ditto, will change the code to do exit(1);
on the err == -1 case.
What do you think about it ?

Thanks,
Taeung

> > +		}
> > +	}
> >   	printf("urandom_read:%d: %lld events per sec\n",
> >   	       cpu, MAX_CNT * 1000000000ll / (time_get_ns() - start_time));
> >   	close(fd);
> > --
> > 2.7.4