Re: [PATCHv2 bpf-next 2/7] selftests/bpf: Move test_progs helpers to testing_helpers object

Jiri Olsa
2023-01-30T23:17:02Z

On Mon, Jan 30, 2023 at 09:23:16AM -0600, David Vernet wrote:

SNIP

> > diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> > index 9695318e8132..c0eb54bf08b3 100644
> > --- a/tools/testing/selftests/bpf/testing_helpers.c
> > +++ b/tools/testing/selftests/bpf/testing_helpers.c
> > @@ -8,6 +8,7 @@
> > #include <bpf/libbpf.h>
> > #include "test_progs.h"
> > #include "testing_helpers.h"
> > +#include <linux/membarrier.h>
> >
> > int parse_num_list(const char *s, bool **num_set, int *num_set_len)
> > {
> > @@ -229,3 +230,65 @@ int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
> >
> > return bpf_prog_load(type, NULL, license, insns, insns_cnt, &opts);
> > }
> > +
> > +static int finit_module(int fd, const char *param_values, int flags)
> > +{
> > + return syscall(__NR_finit_module, fd, param_values, flags);
> > +}
> > +
> > +static int delete_module(const char *name, int flags)
> > +{
> > + return syscall(__NR_delete_module, name, flags);
> > +}
> > +
> > +void unload_bpf_testmod(FILE *err, bool verbose)
>
> Maybe you should pass a const struct test_env * here and in
> load_bpf_testmod() instead? Technically it also has a FILE *stdout, so
> to be consistent we should probably also pass that to the fprintf()
> calls on the success path.

struct test_env is specific for test_progs and we want to call
un/load_bpf_testmod from test_verifier.. but yes, it looks weird
to pass just 'err' and verbose.. maybe we could pass both out/err

>
> > +{
> > + if (kern_sync_rcu())
> > + fprintf(err, "Failed to trigger kernel-side RCU sync!\n");
> > + if (delete_module("bpf_testmod", 0)) {
> > + if (errno == ENOENT) {
> > + if (verbose)
> > + fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
> > + return;
> > + }
> > + fprintf(err, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> > + return;
> > + }
> > + if (verbose)
> > + fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
> > +}
> > +

SNIP

> > diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
> > index 6ec00bf79cb5..2f80ca5b5f54 100644
> > --- a/tools/testing/selftests/bpf/testing_helpers.h
> > +++ b/tools/testing/selftests/bpf/testing_helpers.h
> > @@ -1,5 +1,9 @@
> > /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > /* Copyright (C) 2020 Facebook, Inc. */
> > +
> > +#ifndef __TRACING_HELPERS_H
> > +#define __TRACING_HELPERS_H
>
> s/__TRACING/__TESTING here and below

right, thanks

jirka