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/65dd42dd91d678740e9c05e...
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).
[...]