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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
>
> 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@xxxxxxxxxx/
>
> 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).
>
> [...]
