2020-01-23 17:16:39

by Lorenz Bauer

Subject: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

Use a proper temporary file for sendpage tests. This means that running
the tests doesn't clutter the working directory, and allows running the
test on read-only filesystems.

Signed-off-by: Lorenz Bauer <[email protected]>
---
tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 4a851513c842..779e11da979c 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
FILE *file;
int i, fp;

- file = fopen(".sendpage_tst.tmp", "w+");
+ file = tmpfile();
if (!file) {
perror("create file for sendpage");
return 1;
@@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
fwrite(&k, sizeof(char), 1, file);
fflush(file);
fseek(file, 0, SEEK_SET);
- fclose(file);

- fp = open(".sendpage_tst.tmp", O_RDONLY);
- if (fp < 0) {
- perror("reopen file for sendpage");
- return 1;
- }
+ fp = fileno(file);

clock_gettime(CLOCK_MONOTONIC, &s->start);
for (i = 0; i < cnt; i++) {
@@ -354,11 +349,11 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,

if (!drop && sent < 0) {
perror("send loop error");
- close(fp);
+ fclose(file);
return sent;
} else if (drop && sent >= 0) {
printf("sendpage loop error expected: %i\n", sent);
- close(fp);
+ fclose(file);
return -EIO;
}

@@ -366,7 +361,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
s->bytes_sent += sent;
}
clock_gettime(CLOCK_MONOTONIC, &s->end);
- close(fp);
+ fclose(file);
return 0;
}

--
2.20.1

2020-01-23 18:36:02

by Martin KaFai Lau

Subject: Re: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

On Thu, Jan 23, 2020 at 04:59:30PM +0000, Lorenz Bauer wrote:
> Use a proper temporary file for sendpage tests. This means that running
> the tests doesn't clutter the working directory, and allows running the
> test on read-only filesystems.
>
> Signed-off-by: Lorenz Bauer <[email protected]>
> ---
> tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index 4a851513c842..779e11da979c 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
> FILE *file;
> int i, fp;
>
> - file = fopen(".sendpage_tst.tmp", "w+");
> + file = tmpfile();
> if (!file) {
> perror("create file for sendpage");
> return 1;
> @@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
> fwrite(&k, sizeof(char), 1, file);
> fflush(file);
> fseek(file, 0, SEEK_SET);
> - fclose(file);
>
> - fp = open(".sendpage_tst.tmp", O_RDONLY);
> - if (fp < 0) {
> - perror("reopen file for sendpage");
> - return 1;
> - }
> + fp = fileno(file);
It may be better to keep fp == -1 check here.
It is not clear to me the original intention of reopen.
I would defer to John for comment.

>
> clock_gettime(CLOCK_MONOTONIC, &s->start);
> for (i = 0; i < cnt; i++) {

2020-01-24 09:21:30

by Jakub Sitnicki

Subject: Re: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

On Thu, Jan 23, 2020 at 05:59 PM CET, Lorenz Bauer wrote:
> Use a proper temporary file for sendpage tests. This means that running
> the tests doesn't clutter the working directory, and allows running the
> test on read-only filesystems.
>
> Signed-off-by: Lorenz Bauer <[email protected]>
> ---
> tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index 4a851513c842..779e11da979c 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
> FILE *file;
> int i, fp;
>
> - file = fopen(".sendpage_tst.tmp", "w+");
> + file = tmpfile();
> if (!file) {
> perror("create file for sendpage");
> return 1;

memfd_create() would be an alternative that doesn't clutter anything.

[...]

2020-01-24 21:02:30

by John Fastabend

Subject: RE: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

Lorenz Bauer wrote:
> Use a proper temporary file for sendpage tests. This means that running
> the tests doesn't clutter the working directory, and allows running the
> test on read-only filesystems.
>
> Signed-off-by: Lorenz Bauer <[email protected]>
> ---
> tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>

Agree nice bit of cleanup. We should merge this in bpf-next though
considering its late in the bpf tree.

Acked-by: John Fastabend <[email protected]>

2020-01-24 21:28:16

by John Fastabend

Subject: Re: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

Martin Lau wrote:
> On Thu, Jan 23, 2020 at 04:59:30PM +0000, Lorenz Bauer wrote:
> > Use a proper temporary file for sendpage tests. This means that running
> > the tests doesn't clutter the working directory, and allows running the
> > test on read-only filesystems.
> >
> > Signed-off-by: Lorenz Bauer <[email protected]>
> > ---
> > tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
> > 1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> > index 4a851513c842..779e11da979c 100644
> > --- a/tools/testing/selftests/bpf/test_sockmap.c
> > +++ b/tools/testing/selftests/bpf/test_sockmap.c
> > @@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
> > FILE *file;
> > int i, fp;
> >
> > - file = fopen(".sendpage_tst.tmp", "w+");
> > + file = tmpfile();
> > if (!file) {
> > perror("create file for sendpage");
> > return 1;
> > @@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
> > fwrite(&k, sizeof(char), 1, file);
> > fflush(file);
> > fseek(file, 0, SEEK_SET);
> > - fclose(file);
> >
> > - fp = open(".sendpage_tst.tmp", O_RDONLY);
> > - if (fp < 0) {
> > - perror("reopen file for sendpage");
> > - return 1;
> > - }
> > + fp = fileno(file);
> It may be better to keep fp == -1 check here.
> It is not clear to me the original intention of reopen.
> I would defer to John for comment.
>

Seeing fileno shouldn't fail seems OK to me.

> >
> > clock_gettime(CLOCK_MONOTONIC, &s->start);
> > for (i = 0; i < cnt; i++) {