Re: [PATCH mptcp-next v3 1/1] selftests/bpf: Use make/remove netns helpers in mptcp

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 10/10/2024 05:31, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> New netns selftest helpers make_netns() and remove_netns() are added in

(s/are/have been/)

> network_helpers.c, let's use them in mptcp selftests too.

Thank you for the patch!

I have one small comment below. The rest looks good to me:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Because it is just a single patch, and just a clean-up, do you mind
sending it directly to BPF maintainers please?

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/prog_tests/mptcp.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index acd79be134cd..7f70f8ec76aa 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -82,12 +82,19 @@ struct mptcp_storage {
>  
>  static struct nstoken *create_netns(void)
>  {
> -	SYS(fail, "ip netns add %s", NS_TEST);
> -	SYS(fail, "ip -net %s link set dev lo up", NS_TEST);
> +	struct nstoken *nstoken = NULL;
> +
> +	if (make_netns(NS_TEST))

I think it is clearer to explicitly add "< 0", so people would interpret
this line as "if there is an error when creating the netns". Otherwise,
it is not clear if we are going to take the error or the success branch:

  if (make_netns(NS_TEST) < 0)

> +		goto fail;

Maybe clearer with either:

  return NULL;

or below, also use a goto just for the error path:


      struct nstoken *nstoken = NULL;

      if (make_netns(NS_TEST) < 0)
          goto fail;

      nstoken = open_netns(NS_TEST);
      if (!nstoken)
          goto fail;

      return nstoken;

  fail:
      log_err("open netns %s failed", NS_TEST);
      remove_netns(NS_TEST);
      return NULL;

Up to you.

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