Hi Leo, On Mon, 14 Oct 2024 at 22:37, Leo Stone <leocstone@xxxxxxxxx> wrote: > > Add tests that check if getsockopt(TCP_AO_GET_KEYS) returns the right > keys when using different filters. > > Sample output: > > > # ok 114 filter keys: by sndid, rcvid, address > > # ok 115 filter keys: by sndid, rcvid > > # ok 116 filter keys: by is_current > > # ok 117 filter keys: by is_rnext > > Signed-off-by: Leo Stone <leocstone@xxxxxxxxx> > --- > This patch is meant to address the TODO in setsockopt-closed.c: > > /* > > * TODO: check getsockopt(TCP_AO_GET_KEYS) with different filters > > * returning proper nr & keys; > > */ > > Is this a reasonable way to do these tests? If so, what cases should I > add? Your change does look reasonable to me. I think you could add one more test here for passing (FILTER_TEST_NKEYS/2) to getsockopt() as tcp_ao_getsockopt::nkeys with get_all = 1, and check that the value in tcp_ao_getsockopt::nkeys after getsockopt() reflects the number of matched keys (FILTER_TEST_NKEYS). See also minor nits inline. [..] > +static void filter_keys_checked(int sk, struct tcp_ao_getsockopt *filter, > + struct tcp_ao_getsockopt *expected, > + unsigned int nexpected, const char *tst) > +{ > + struct tcp_ao_getsockopt all_keys[FILTER_TEST_NKEYS] = {}; > + struct tcp_ao_getsockopt filtered_keys[FILTER_TEST_NKEYS] = {}; > + socklen_t len = sizeof(struct tcp_ao_getsockopt); > + > + fetch_all_keys(sk, all_keys); > + memcpy(&filtered_keys[0], filter, sizeof(struct tcp_ao_getsockopt)); > + filtered_keys[0].nkeys = FILTER_TEST_NKEYS; > + if (getsockopt(sk, IPPROTO_TCP, TCP_AO_GET_KEYS, filtered_keys, &len)) > + test_error("getsockopt"); I think the following two checks would be better s/test_error/test_fail/. The difference between _error() and _fail() is that for the later exit() is not called, which allows the person running the test to gather all "not okay" cases. So, in tcp_ao selftests I used _error() only for failures where nothing meaningful could be done afterwards, i.e. memory allocation or socket() creation. > + if (filtered_keys[0].nkeys != nexpected) > + test_error("wrong nr of keys, expected %u got %u", nexpected, > + filtered_keys[0].nkeys); > + if (compare_mkts(expected, nexpected, filtered_keys, filtered_keys[0].nkeys)) > + test_error("got wrong keys back"); ^ in those two it seems to be better to do : test_fail("...") : goto out_close; which would allow to go through other "filter" and "duplicate" selftests even if one of the "filter" tests has failed. [..] > static void *client_fn(void *arg) > { > if (inet_pton(TEST_FAMILY, __TEST_CLIENT_IP(2), &tcp_md5_client) != 1) > test_error("Can't convert ip address"); > extend_tests(); > einval_tests(); > + filter_tests(); > duplicate_tests(); > /* > * TODO: check getsockopt(TCP_AO_GET_KEYS) with different filters ^ please, remove the related TODO comment, I think you just fixed it :-) Thank you for the patch, Dmitry