Re: [PATCH v2] selftest/tcp-ao: Add filter tests

[Simon Horman]

On Tue, Oct 15, 2024 at 10:51:52PM -0700, Leo Stone 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 is_current
> > # ok 116 filter keys: by is_rnext
> > # ok 117 filter keys: by sndid, rcvid
> > # ok 118 filter keys: correct nkeys when in.nkeys < matched_keys
> 
> Signed-off-by: Leo Stone <leocstone@xxxxxxxxx>
> ---
> Changes in v2:
> - Changed 2 unnecessary test_error calls to test_fail
> - Added another test to make sure getsockopt returns the right nkeys
>   value when the input nkeys is smaller than the number of matching keys
> - Removed the TODO that this patch addresses
> 
> Thank you for your feedback.
> ---
>  .../selftests/net/tcp_ao/setsockopt-closed.c  | 180 +++++++++++++++++-
>  1 file changed, 175 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c b/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c
> index 084db4ecdff6..4bfa76c28e4e 100644
> --- a/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c
> +++ b/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c
> @@ -6,6 +6,8 @@
>  
>  static union tcp_addr tcp_md5_client;
>  
> +#define FILTER_TEST_NKEYS 16
> +
>  static int test_port = 7788;
>  static void make_listen(int sk)
>  {
> @@ -813,23 +815,191 @@ static void duplicate_tests(void)
>  	setsockopt_checked(sk, TCP_AO_ADD_KEY, &ao, EEXIST, "duplicate: SendID differs");
>  }
>  
> +
> +static void fetch_all_keys(int sk, struct tcp_ao_getsockopt *keys)
> +{
> +	socklen_t optlen = sizeof(struct tcp_ao_getsockopt);
> +
> +	memset(keys, 0, sizeof(struct tcp_ao_getsockopt) * FILTER_TEST_NKEYS);
> +	keys[0].get_all = 1;
> +	keys[0].nkeys = FILTER_TEST_NKEYS;
> +	if (getsockopt(sk, IPPROTO_TCP, TCP_AO_GET_KEYS, &keys[0], &optlen))
> +		test_error("getsockopt");
> +}
> +
> +static int prepare_test_keys(struct tcp_ao_getsockopt *keys)
> +{
> +	struct tcp_ao_add test_ao[FILTER_TEST_NKEYS];
> +	u8 rcvid = 100, sndid = 100;
> +	const char *test_password = "Test password number ";
> +	char test_password_scratch[64] = {};
> +	int sk = socket(test_family, SOCK_STREAM, IPPROTO_TCP);

Hi Leo,

In Networking code it is preferred to arrange local variables in
reverse xmas tree order. In this case I think that could be as
follows (completely untested!).

Also, as the sk needs to be checked for errors, I would
separate it's assignment form it's declaration

	const char *test_password = "Test password number ";
	struct tcp_ao_add test_ao[FILTER_TEST_NKEYS];
	char test_password_scratch[64] = {};
	u8 rcvid = 100, sndid = 100;
	int sk;

	sk = socket(test_family, SOCK_STREAM, IPPROTO_TCP);
	if (sk < 0)
		test_error("socket()");

This tool can be of assistance here:
https://github.com/ecree-solarflare/xmastree

> +
> +	if (sk < 0)
> +		test_error("socket()");
> +
> +	for (int i = 0; i < FILTER_TEST_NKEYS; i++) {
> +		snprintf(test_password_scratch, 64, "%s %d", test_password, i);
> +		test_prepare_key(&test_ao[i], DEFAULT_TEST_ALGO, this_ip_dest, false, false,
> +				 DEFAULT_TEST_PREFIX, 0, sndid++, rcvid++, 0, 0,
> +				 strlen(test_password_scratch), test_password_scratch);

Likewise, in Networking code it is still preferred to keep lines at or
below 80 columns wide, where it can trivially be achieved: don't split
strings across or otherwise make the code less readable because of this
guideline.

		test_prepare_key(&test_ao[i], DEFAULT_TEST_ALGO, this_ip_dest,
				 false, false, DEFAULT_TEST_PREFIX, 0, sndid++,
				 rcvid++, 0, 0, strlen(test_password_scratch),
				 test_password_scratch);

You can check for this using:
./scripts/checkpatch.pl --strict --max-line-length=80

I think it would be good if you could do a pass over this patch with the
above in mind.

Lastly, please include the target tree, net or net-next, in the subject
when posting patches for Networking.

	Subject: [PATCH net-next] ...

More information on processes for netdev can be found here:
https://docs.kernel.org/process/maintainer-netdev.html

-- 
pw-bot: changes-requested