[PATCH 1/2] selftests/netfilter: use socklen_t, not a signed int, for len

[PATCH 1/2] selftests/netfilter: use socklen_t, not a signed int, for len

[John Hubbard]

When building with clang, via:

    make LLVM=1 -C tools/testing/selftest

...clang warns about using "int *" interchangeably with "socklen_t *".

clang is correct, so fix this by declaring len as a socklen_t, instead
of as an int.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/netfilter/sctp_collision.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/netfilter/sctp_collision.c b/tools/testing/selftests/netfilter/sctp_collision.c
index 21bb1cfd8a85..91df996367e9 100644
--- a/tools/testing/selftests/netfilter/sctp_collision.c
+++ b/tools/testing/selftests/netfilter/sctp_collision.c
@@ -9,7 +9,8 @@
 int main(int argc, char *argv[])
 {
 	struct sockaddr_in saddr = {}, daddr = {};
-	int sd, ret, len = sizeof(daddr);
+	int sd, ret;
+	socklen_t len = sizeof(daddr);
 	struct timeval tv = {25, 0};
 	char buf[] = "hello";
 

base-commit: f462ae0edd3703edd6f22fe41d336369c38b884b
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
-- 
2.45.0

[PATCH 2/2] selftests/netfilter: return a value for several "int" functions

[John Hubbard]

When building with clang, via:

    make LLVM=1 -C tools/testing/selftests

...clang warns, correctly, that several functions declared with an "int"
return type are not always returning values in all cases (or at least,
clang cannot prove that they always return a value).

Fix this by returning 0 for each function. (For these functions,
non-zero values indicate failure.)

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/netfilter/conntrack_dump_flush.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/netfilter/conntrack_dump_flush.c b/tools/testing/selftests/netfilter/conntrack_dump_flush.c
index b11ea8ee6719..2513221ae5e6 100644
--- a/tools/testing/selftests/netfilter/conntrack_dump_flush.c
+++ b/tools/testing/selftests/netfilter/conntrack_dump_flush.c
@@ -43,6 +43,7 @@ static int build_cta_tuple_v4(struct nlmsghdr *nlh, int type,
 	mnl_attr_nest_end(nlh, nest_proto);
 
 	mnl_attr_nest_end(nlh, nest);
+	return 0;
 }
 
 static int build_cta_tuple_v6(struct nlmsghdr *nlh, int type,
@@ -71,6 +72,7 @@ static int build_cta_tuple_v6(struct nlmsghdr *nlh, int type,
 	mnl_attr_nest_end(nlh, nest_proto);
 
 	mnl_attr_nest_end(nlh, nest);
+	return 0;
 }
 
 static int build_cta_proto(struct nlmsghdr *nlh)
@@ -90,6 +92,7 @@ static int build_cta_proto(struct nlmsghdr *nlh)
 	mnl_attr_nest_end(nlh, nest_proto);
 
 	mnl_attr_nest_end(nlh, nest);
+	return 0;
 }
 
 static int conntrack_data_insert(struct mnl_socket *sock, struct nlmsghdr *nlh,
@@ -207,6 +210,7 @@ static int conntrack_data_generate_v6(struct mnl_socket *sock,
 static int count_entries(const struct nlmsghdr *nlh, void *data)
 {
 	reply_counter++;
+	return 0;
 }
 
 static int conntracK_count_zone(struct mnl_socket *sock, uint16_t zone)
-- 
2.45.0

Re: [PATCH 2/2] selftests/netfilter: return a value for several "int" functions

[John Hubbard]

On 5/6/24 7:41 AM, Felix Huettner wrote:
> On Sun, May 05, 2024 at 02:47:16PM -0700, John Hubbard wrote:
...
>  > @@ -207,6 +210,7 @@ static int conntrack_data_generate_v6(struct 
> mnl_socket *sock,
>  >  static int count_entries(const struct nlmsghdr *nlh, void *data)
>  >  {
>  >         reply_counter++;
>  > +       return 0;
> 
> Hi John,
> 
> This will need to return MNL_CB_OK.
> Otherwise mnl_cb_run below will abort early and the connection count
> will be wrong.
> 

Thanks for catching that, I'm sending a v2 with that fix.

I was thinking about it, and expected that the pre-existing code
appeared to work because the return value was some non-zero garbage
value scrounged off of the stack (or %rax, for example on x86).

However, just a quick test showed that *any* value (O, 1==MNL_CB_OK,
or no value at all) allows the test to report success...oh, I see,
it's reporting PASSED when it really ought to say SKIPPED:

$ ./conntrack_dump_flush
TAP version 13
1..3
# Starting 3 tests from 1 test cases.
#  RUN           conntrack_dump_flush.test_dump_by_zone ...
mnl_socket_open: Protocol not supported
#            OK  conntrack_dump_flush.test_dump_by_zone
ok 1 conntrack_dump_flush.test_dump_by_zone
#  RUN           conntrack_dump_flush.test_flush_by_zone ...
mnl_socket_open: Protocol not supported
#            OK  conntrack_dump_flush.test_flush_by_zone
ok 2 conntrack_dump_flush.test_flush_by_zone
#  RUN           conntrack_dump_flush.test_flush_by_zone_default ...
mnl_socket_open: Protocol not supported
#            OK  conntrack_dump_flush.test_flush_by_zone_default
ok 3 conntrack_dump_flush.test_flush_by_zone_default
# PASSED: 3 / 3 tests passed.
# Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0

As long as we are looking at this, what do you think about
this:

diff --git a/tools/testing/selftests/netfilter/conntrack_dump_flush.c 
b/tools/testing/selftests/netfilter/conntrack_dump_flush.c
index e9df4ae14e16..4a73afad4de4 100644
--- a/tools/testing/selftests/netfilter/conntrack_dump_flush.c
+++ b/tools/testing/selftests/netfilter/conntrack_dump_flush.c
@@ -317,12 +317,12 @@ FIXTURE_SETUP(conntrack_dump_flush)
         self->sock = mnl_socket_open(NETLINK_NETFILTER);
         if (!self->sock) {
                 perror("mnl_socket_open");
-               exit(EXIT_FAILURE);
+               SKIP(exit(EXIT_FAILURE), "mnl_socket_open() failed");
         }

         if (mnl_socket_bind(self->sock, 0, MNL_SOCKET_AUTOPID) < 0) {
                 perror("mnl_socket_bind");
-               exit(EXIT_FAILURE);
+               SKIP(exit(EXIT_FAILURE), "mnl_socket_bind() failed");
         }

         ret = conntracK_count_zone(self->sock, TEST_ZONE_ID);

...which changes the above output, to:

$  ./conntrack_dump_flush
TAP version 13
1..3
# Starting 3 tests from 1 test cases.
#  RUN           conntrack_dump_flush.test_dump_by_zone ...
mnl_socket_open: Protocol not supported
#      SKIP      mnl_socket_open() failed
#            OK  conntrack_dump_flush.test_dump_by_zone
ok 1 conntrack_dump_flush.test_dump_by_zone # SKIP mnl_socket_open() failed
#  RUN           conntrack_dump_flush.test_flush_by_zone ...
mnl_socket_open: Protocol not supported
#      SKIP      mnl_socket_open() failed
#            OK  conntrack_dump_flush.test_flush_by_zone
ok 2 conntrack_dump_flush.test_flush_by_zone # SKIP mnl_socket_open() failed
#  RUN           conntrack_dump_flush.test_flush_by_zone_default ...
mnl_socket_open: Protocol not supported
#      SKIP      mnl_socket_open() failed
#            OK  conntrack_dump_flush.test_flush_by_zone_default
ok 3 conntrack_dump_flush.test_flush_by_zone_default # SKIP 
mnl_socket_open() failed
# PASSED: 3 / 3 tests passed.
# Totals: pass:0 fail:0 xfail:0 xpass:0 skip:3 error:0

?

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 2/2] selftests/netfilter: return a value for several "int" functions

[John Hubbard]

On 5/7/24 1:36 AM, Felix Huettner wrote:
>  >
>  > As long as we are looking at this, what do you think about
>  > this:
>  >
>  > diff --git a/tools/testing/selftests/netfilter/conntrack_dump_flush.c
>  > b/tools/testing/selftests/netfilter/conntrack_dump_flush.c
>  > index e9df4ae14e16..4a73afad4de4 100644
>  > --- a/tools/testing/selftests/netfilter/conntrack_dump_flush.c
>  > +++ b/tools/testing/selftests/netfilter/conntrack_dump_flush.c
>  > @@ -317,12 +317,12 @@ FIXTURE_SETUP(conntrack_dump_flush)
>  >         self->sock = mnl_socket_open(NETLINK_NETFILTER);
>  >         if (!self->sock) {
>  >                 perror("mnl_socket_open");
>  > -               exit(EXIT_FAILURE);
>  > +               SKIP(exit(EXIT_FAILURE), "mnl_socket_open() failed");
>  >         }
>  >
>  >         if (mnl_socket_bind(self->sock, 0, MNL_SOCKET_AUTOPID) < 0) {
>  >                 perror("mnl_socket_bind");
>  > -               exit(EXIT_FAILURE);
>  > +               SKIP(exit(EXIT_FAILURE), "mnl_socket_bind() failed");
>  >         }
>  >
>  >         ret = conntracK_count_zone(self->sock, TEST_ZONE_ID);
>  >
>  > ...which changes the above output, to:
>  >
>  > $  ./conntrack_dump_flush
>  > TAP version 13
>  > 1..3
>  > # Starting 3 tests from 1 test cases.
>  > #  RUN           conntrack_dump_flush.test_dump_by_zone ...
>  > mnl_socket_open: Protocol not supported
>  > #      SKIP      mnl_socket_open() failed
>  > #            OK  conntrack_dump_flush.test_dump_by_zone
>  > ok 1 conntrack_dump_flush.test_dump_by_zone # SKIP mnl_socket_open() 
> failed
>  > #  RUN           conntrack_dump_flush.test_flush_by_zone ...
>  > mnl_socket_open: Protocol not supported
>  > #      SKIP      mnl_socket_open() failed
>  > #            OK  conntrack_dump_flush.test_flush_by_zone
>  > ok 2 conntrack_dump_flush.test_flush_by_zone # SKIP mnl_socket_open() 
> failed
>  > #  RUN           conntrack_dump_flush.test_flush_by_zone_default ...
>  > mnl_socket_open: Protocol not supported
>  > #      SKIP      mnl_socket_open() failed
>  > #            OK  conntrack_dump_flush.test_flush_by_zone_default
>  > ok 3 conntrack_dump_flush.test_flush_by_zone_default # SKIP
>  > mnl_socket_open() failed
>  > # PASSED: 3 / 3 tests passed.
>  > # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:3 error:0
>  >
>  > ?
> 
> lgtm
> 

OK, I'll send that out, appreciate your looking at it.

thanks,
-- 
John Hubbard
NVIDIA