| Message ID | 1514136201-111109-1-git-send-email-sowmini.varadhan@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] selftests/net: fix bugs in cfg_port initialization | expand |
On Sun, Dec 24, 2017 at 12:23 PM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > If -S is not used in the command line, we should > be binding to *.<cfg-port>. Similarly, cfg_port should be > used to connect to the remote host even if it is processed > after -D. Thus we need to make sure that the cfg_port in > cfg_src_addr and cfg_dst_addr are always initialized > after all other command line options are parsed. Thanks, Sowmini. Yes, input parsing as is is dependent on order, which is surprising and fragile. It would be good to also address the other instance of this at the same time: protocol family (-4 or -6) has to be set before a call to setup_sockaddr. Unlike this case, it hits an error() if called in the "incorrect" order, but that is still a crutch. The new init_sockaddr_port duplicates most of setup_sockaddr. More concise is to add a branch to that to skip inet_pton if !str_addr. But even better will be to save cfg_port, and src + dst addr optargs as local variables, then call the init function only after parsing when all state is available. Where this patch adds calls to init_sockaddr_port, indeed.
On (12/25/17 15:49), Willem de Bruijn wrote: > > It would be good to also address the other instance of this at the > same time: protocol family (-4 or -6) has to be set before a call to > setup_sockaddr. Unlike this case, it hits an error() if called in the > "incorrect" order, but that is still a crutch. Yeah, I thought of that one too, but "usually" (at least that's what is instinctive to me) you bind to INADDR_ANY or ::, so this only becomes an issue on the client side. (And there, most people put the -4 or -6 before the address, but I agree it would be nice to fix this..) > But even better will be to save cfg_port, and src + dst addr optargs > as local variables, then call the init function only after parsing when > all state is available. Where this patch adds calls to > init_sockaddr_port, indeed. sure, I can put that out on V2 later this week.
diff --git a/tools/testing/selftests/net/msg_zerocopy.c b/tools/testing/selftests/net/msg_zerocopy.c index 3ab6ec4..561fff7 100644 --- a/tools/testing/selftests/net/msg_zerocopy.c +++ b/tools/testing/selftests/net/msg_zerocopy.c @@ -259,6 +259,27 @@ static int setup_ip6h(struct ipv6hdr *ip6h, uint16_t payload_len) return sizeof(*ip6h); } +static void init_sockaddr_port(sa_family_t af, + struct sockaddr_storage *sockaddr) +{ + struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *) sockaddr; + struct sockaddr_in *addr4 = (struct sockaddr_in *) sockaddr; + + switch (af) { + case PF_INET: + addr4->sin_family = PF_INET; + addr4->sin_port = htons(cfg_port); + break; + case PF_INET6: + addr4->sin_family = PF_INET6; + addr6->sin6_port = htons(cfg_port); + break; + default: + error(1, 0, "illegal domain"); + break; + } +} + static void setup_sockaddr(int domain, const char *str_addr, void *sockaddr) { struct sockaddr_in6 *addr6 = (void *) sockaddr; @@ -638,7 +659,7 @@ static void parse_opts(int argc, char **argv) cfg_cork_mixed = true; break; case 'p': - cfg_port = htons(strtoul(optarg, NULL, 0)); + cfg_port = strtoul(optarg, NULL, 0); break; case 'r': cfg_rx = true; @@ -660,6 +681,8 @@ static void parse_opts(int argc, char **argv) break; } } + init_sockaddr_port(cfg_family, &cfg_dst_addr); + init_sockaddr_port(cfg_family, &cfg_src_addr); if (cfg_payload_len > max_payload_len) error(1, 0, "-s: payload exceeds max (%d)", max_payload_len);
If -S is not used in the command line, we should be binding to *.<cfg-port>. Similarly, cfg_port should be used to connect to the remote host even if it is processed after -D. Thus we need to make sure that the cfg_port in cfg_src_addr and cfg_dst_addr are always initialized after all other command line options are parsed. Store cfg_port in host-byte order, and use htons() to set up the sin_port/sin6_port before bind/connect, so that the network system calls get the correct values in network-byte order. Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> --- tools/testing/selftests/net/msg_zerocopy.c | 25 ++++++++++++++++++++++++- 1 files changed, 24 insertions(+), 1 deletions(-)