[bpf-next,v2,2/6] selftests/bpf: Skip ENOTSUPP in ASSERT_OK

Commit Message

Geliang Tang July 5, 2024, 2:38 a.m. UTC

From: Geliang Tang <[e-mail]>

Just like handling ENOTSUPP in test_lsm_cgroup_functional(), this patch
adds a new helper test_progs_get_error() to check whether the input error
is ENOTSUPP (524) or ENOTSUP (95). If it is, invoke test__skip() to skip
the test instead of using test__fail().

Use this helper in ASSERT_OK() before invoking CHECK() macro.

Signed-off-by: Geliang Tang <[e-mail]>
---
 .../selftests/bpf/prog_tests/lsm_cgroup.c     |  6 +----
 tools/testing/selftests/bpf/test_progs.h      | 23 +++++++++++++++++--
 2 files changed, 22 insertions(+), 7 deletions(-)

Comments

Andrii Nakryiko July 8, 2024, 6:54 p.m. UTC | #1

On Thu, Jul 4, 2024 at 7:38 PM Geliang Tang <[e-mail]> wrote:
>
> From: Geliang Tang <[e-mail]>
>
> Just like handling ENOTSUPP in test_lsm_cgroup_functional(), this patch
> adds a new helper test_progs_get_error() to check whether the input error
> is ENOTSUPP (524) or ENOTSUP (95). If it is, invoke test__skip() to skip
> the test instead of using test__fail().
>
> Use this helper in ASSERT_OK() before invoking CHECK() macro.
>
> Signed-off-by: Geliang Tang <[e-mail]>
> ---
>  .../selftests/bpf/prog_tests/lsm_cgroup.c     |  6 +----
>  tools/testing/selftests/bpf/test_progs.h      | 23 +++++++++++++++++--
>  2 files changed, 22 insertions(+), 7 deletions(-)
>

I haven't followed these patch sets, but no, let's not add magical
special error codes handling into ASSERT_xxx() macros.

> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> index 6df25de8f080..6511f5f4a00f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> @@ -102,12 +102,8 @@ static void test_lsm_cgroup_functional(void)
>         ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count");
>         ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count");
>         err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
> -       if (err == -ENOTSUPP) {
> -               test__skip();
> -               goto close_cgroup;
> -       }
>         if (!ASSERT_OK(err, "attach alloc_prog_fd"))
> -               goto detach_cgroup;
> +               goto close_cgroup;
>         ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 1, "prog count");
>         ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 1, "total prog count");
>
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 930a4181dbd9..d1d77785b165 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -176,6 +176,23 @@ void test__skip(void);
>  void test__fail(void);
>  int test__join_cgroup(const char *path);
>
> +static inline bool test_progs_check_errno(int error, int check)
> +{
> +       return error == -check ||
> +              (error && errno == check);
> +}
> +
> +static inline int test_progs_get_error(int error)
> +{
> +       if (test_progs_check_errno(error, ENOTSUP) ||
> +           test_progs_check_errno(error, ENOTSUPP)) {
> +               test__skip();
> +               return 0;
> +       } else {
> +               return error;
> +       }
> +}
> +
>  #define PRINT_FAIL(format...)                                                  \
>         ({                                                                     \
>                 test__fail();                                                  \
> @@ -338,8 +355,10 @@ int test__join_cgroup(const char *path);
>         static int duration = 0;                                        \
>         long long ___res = (res);                                       \
>         bool ___ok = ___res == 0;                                       \
> -       CHECK(!___ok, (name), "unexpected error: %lld (errno %d)\n",    \
> -             ___res, errno);                                           \
> +       if (test_progs_get_error(___res))                               \
> +               CHECK(!___ok, (name),                                   \
> +                     "unexpected error: %lld (errno %d)\n",            \
> +                     ___res, errno);                                   \
>         ___ok;                                                          \
>  })
>
> --
> 2.43.0
>

Eduard Zingerman July 8, 2024, 7:47 p.m. UTC | #2

On Mon, 2024-07-08 at 11:54 -0700, Andrii Nakryiko wrote:
> On Thu, Jul 4, 2024 at 7:38 PM Geliang Tang <[e-mail]> wrote:
> > 
> > From: Geliang Tang <[e-mail]>
> > 
> > Just like handling ENOTSUPP in test_lsm_cgroup_functional(), this patch
> > adds a new helper test_progs_get_error() to check whether the input error
> > is ENOTSUPP (524) or ENOTSUP (95). If it is, invoke test__skip() to skip
> > the test instead of using test__fail().
> > 
> > Use this helper in ASSERT_OK() before invoking CHECK() macro.
> > 
> > Signed-off-by: Geliang Tang <[e-mail]>
> > ---
> >  .../selftests/bpf/prog_tests/lsm_cgroup.c     |  6 +----
> >  tools/testing/selftests/bpf/test_progs.h      | 23 +++++++++++++++++--
> >  2 files changed, 22 insertions(+), 7 deletions(-)
> > 
> 
> I haven't followed these patch sets, but no, let's not add magical
> special error codes handling into ASSERT_xxx() macros.

I agree with Andrii here.
You might use -d (denylist) option for test_progs to filter out test
cases you know are not supported.

[...]

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
index 6df25de8f080..6511f5f4a00f 100644
--- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
@@ -102,12 +102,8 @@  static void test_lsm_cgroup_functional(void)
 	ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count");
 	ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count");
 	err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
-	if (err == -ENOTSUPP) {
-		test__skip();
-		goto close_cgroup;
-	}
 	if (!ASSERT_OK(err, "attach alloc_prog_fd"))
-		goto detach_cgroup;
+		goto close_cgroup;
 	ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 1, "prog count");
 	ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 1, "total prog count");
 
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 930a4181dbd9..d1d77785b165 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -176,6 +176,23 @@  void test__skip(void);
 void test__fail(void);
 int test__join_cgroup(const char *path);
 
+static inline bool test_progs_check_errno(int error, int check)
+{
+	return error == -check ||
+	       (error && errno == check);
+}
+
+static inline int test_progs_get_error(int error)
+{
+	if (test_progs_check_errno(error, ENOTSUP) ||
+	    test_progs_check_errno(error, ENOTSUPP)) {
+		test__skip();
+		return 0;
+	} else {
+		return error;
+	}
+}
+
 #define PRINT_FAIL(format...)                                                  \
 	({                                                                     \
 		test__fail();                                                  \
@@ -338,8 +355,10 @@  int test__join_cgroup(const char *path);
 	static int duration = 0;					\
 	long long ___res = (res);					\
 	bool ___ok = ___res == 0;					\
-	CHECK(!___ok, (name), "unexpected error: %lld (errno %d)\n",	\
-	      ___res, errno);						\
+	if (test_progs_get_error(___res))				\
+		CHECK(!___ok, (name),					\
+		      "unexpected error: %lld (errno %d)\n",		\
+		      ___res, errno);					\
 	___ok;								\
 })