Re: [PATCH bpf-next 1/5] selftests/bpf: Drop type from network_helper_opts

[Geliang Tang <geliang@kernel.org>]

On Tue, 2024-06-11 at 14:20 -0700, Eduard Zingerman wrote:
> On Tue, 2024-06-11 at 09:59 +0800, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The opts.{type, noconnect, must_fail} is at least a bit non
> > intuitive or
> > unnecessary. The only use case now is in
> > test_bpf_ip_check_defrag_ok which
> > ends up bypassing most (or at least some) of the
> > connect_to_fd_opts()
> > logic. It's much better that test should have its own
> > connect_to_fd_opts()
> > instead.
> > 
> > This patch adds a new helper named __connect_to_fd_opts() to do
> > this. It
> > accepts a new "type" parameter, then opts->type can be replaced by
> > "type"
> > parameter in this helper. In test_bpf_ip_check_defrag_ok, different
> > types
> > are passed to it. And the strcut member "type" of
> > network_helper_opts can
> > be dropped now.
> > 
> > Then connect_to_fd_opts can implement as a wrapper of this new
> > helper.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> 
> Patches #1,2,3 trade options specified as struct fields for options
> specified as function parameters. Tbh, this seems to be an
> opinionated
> stylistic change, what is the need for it?

Thanks for your review.

Patches 1-3 address Martin's comment for "Drop type parameter of
start_server_addr" [1].

Since opts.{type, noconnect} are only used in ip_check_defrag.c and
opts.must_fail is only used in cgroup_v1v2.c, they are not generic
enough to be added into network_helper_opts. So this set removes them
from network_helper_opts and use them as function parameters.

Thanks,
-Geliang

[1]
https://patchwork.kernel.org/project/netdevbpf/patch/65dd42dd91d678740e9c05e32852f5e01ba2b7bc.1716369375.git.tanggeliang@kylinos.cn/

> 
> If anything, I think that this is less readable:
> 
> > +	client_rx_fd = __connect_to_fd_opts(srv_fd, 0, &rx_opts);
> 
> compared to this:
> 
> >  	struct network_helper_opts tx_ops = {
> >  		.timeout_ms = 1000,
> > -		.type = SOCK_RAW,
> >  		.proto = IPPROTO_RAW,
> >  		.noconnect = true,
> >  	};
> ...
> > -	client_rx_fd = connect_to_fd_opts(srv_fd, &rx_opts);
> 
> (given that by patch #3 three parameters are added to
>  __connect_to_fd_opts() *and* it also accepts options).
> 
> [...]