Re: [mptcp-next] selftests: mptcp: cover 'mptcp_diag_dump_one' in mptcp_sockopt

[Matthieu Baerts <matttbe@kernel.org>]

Hi Gang Yan,

On 17/01/2025 07:44, Gang Yan wrote:
> From: Gang Yan <yangang@kylinos.cn>
> 
> Through code coverage analysis, it has been identified that the
> 'mptcp_diag_dump_one' function lacks test coverage on the testing front.
> 
> This patch introduces a function in mptcp_sockopt.c, which is built upon
> the 'inet_diag' module, and integrates it into the 'mptcp_sockopt.sh' to
> execute the 'mptcp_diag_dump_one' function when server bound a socket.

Thank you for increasing the code coverage!

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/524
> Signed-off-by: Gang Yan <yangang@kylinos.cn>
> ---
>  .../selftests/net/mptcp/mptcp_sockopt.c       | 76 ++++++++++++++++++-
>  .../selftests/net/mptcp/mptcp_sockopt.sh      |  6 ++
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index 926b0be87c99..caa0466d827f 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c

(...)

> +void diag(void)
> +{
> +	int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
> +
> +	if (fd < 0)
> +		die_perror("Netlink socket");
> +
> +	send_query(fd);

From what I see, you are sending a request, but you don't look at the
reply. I understand the objective is to increase the code coverage, but
while at it, it sounds better to verify the kernel is doing the right
thing: the reply looks correct.

Do you mind checking the reply? e.g. create a connection, request info
about it, and compare (a part of) the reply with info you already have.

(Also, is your code here covering mptcp_diag_dump_one() function?)

> +
> +	close(fd);
> +}
> +
>  static void parse_opts(int argc, char **argv)
>  {
>  	int c;
>  
> -	while ((c = getopt(argc, argv, "h6")) != -1) {
> +	while ((c = getopt(argc, argv, "h6d")) != -1) {
>  		switch (c) {
>  		case 'h':
>  			die_usage(0);
> @@ -262,6 +332,10 @@ static void parse_opts(int argc, char **argv)
>  		case '6':
>  			pf = AF_INET6;
>  			break;
> +		case 'd':
> +			diag();
> +			exit(0);
> +			break;
>  		default:
>  			die_usage(1);

(you didn't update the 'usage' menu)

>  			break;
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 5e8d5b83e2d0..3f74100bc023 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh

(...)

> @@ -356,6 +361,7 @@ run_tests $ns1 $ns2 dead:beef:1::1
>  
>  do_mptcp_sockopt_tests
>  do_tcpinq_tests
> +do_diag_tests

It sounds strange to do this Netlink request in the MPTCP Socket Option
test program.

Ideally, this should be done from diag.sh. I guess you modified
mptcp_sockopt.c because you needed to do some operations in C, right?

Here are some ideas:

- Create a new diag.c file to do these operations, and call the new
program from diag.sh. In diag.sh, you already have connections that are
created, you can do an extra check there, e.g. compare info from ss and
from diag.c.

- Modify 'ss' to be able to query one MPTCP connection having a given
token. Then use this feature (if available) in diag.sh.

- Modify mptcp_sockopt.c to query info about one connection via netlink,
e.g. to compare them with info retrieved via getsockopt(MPTCP_INFO) from
do_getsockopt_mptcp_info(). You could compare them in
process_one_client() I suppose (to be checked where it sounds better to
do that).

- ...


The first 2 options might make more sense: it is all about 'diag'. Maybe
the 3rd option can be justified if the goal is to compare more data from
getsockopt(MPTCP_INFO).


Note that modifying 'ss' to query info about a specific connection seems
useful anyway. In other words, we could have both option 1 and 2 in
place. Or only option 1 or 3 + the support in 'ss' but not used in the
selftests.

WDYT?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.