| Message ID | 20210408191159.133644-6-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | mptcp support | expand |
On Thu, Apr 08, 2021 at 08:11:59PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Multipath TCP allows combining multiple interfaces/routes into a single > socket, with very little work for the user/admin. > > It's enabled by 'mptcp' on most socket addresses: > > ./qemu-system-x86_64 -nographic -incoming tcp:0:4444,mptcp > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > io/dns-resolver.c | 2 ++ > qapi/sockets.json | 5 ++++- > util/qemu-sockets.c | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/io/dns-resolver.c b/io/dns-resolver.c > index 743a0efc87..b081e098bb 100644 > --- a/io/dns-resolver.c > +++ b/io/dns-resolver.c > @@ -122,6 +122,8 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver, > .ipv4 = iaddr->ipv4, > .has_ipv6 = iaddr->has_ipv6, > .ipv6 = iaddr->ipv6, > + .has_mptcp = iaddr->has_mptcp, > + .mptcp = iaddr->mptcp, > }; > > (*addrs)[i] = newaddr; > diff --git a/qapi/sockets.json b/qapi/sockets.json > index 2e83452797..43122a38bf 100644 > --- a/qapi/sockets.json > +++ b/qapi/sockets.json > @@ -57,6 +57,8 @@ > # @keep-alive: enable keep-alive when connecting to this socket. Not supported > # for passive sockets. (Since 4.2) > # > +# @mptcp: enable multi-path TCP. (Since 6.0) > +# > # Since: 1.3 > ## > { 'struct': 'InetSocketAddress', > @@ -66,7 +68,8 @@ > '*to': 'uint16', > '*ipv4': 'bool', > '*ipv6': 'bool', > - '*keep-alive': 'bool' } } > + '*keep-alive': 'bool', > + '*mptcp': 'bool' } } I think this would need to be '*mptcp': { 'type': 'bool', 'if': 'IPPROTO_MPTCP' } so that mgmt apps can probe when it truely is supported or not for this build > > ## > # @UnixSocketAddress: > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 8af0278f15..72527972d5 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -206,6 +206,21 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e) > #endif > } > > +static int check_mptcp(const InetSocketAddress *saddr, struct addrinfo *ai, > + Error **errp) > +{ > + if (saddr->has_mptcp && saddr->mptcp) { > +#ifdef IPPROTO_MPTCP > + ai->ai_protocol = IPPROTO_MPTCP; > +#else > + error_setg(errp, "MPTCP unavailable in this build"); > + return -1; > +#endif > + } > + > + return 0; > +} > + > static int inet_listen_saddr(InetSocketAddress *saddr, > int port_offset, > int num, > @@ -278,6 +293,11 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > /* create socket + bind/listen */ > for (e = res; e != NULL; e = e->ai_next) { > + if (check_mptcp(saddr, e, &err)) { > + error_propagate(errp, err); > + return -1; > + } So this is doing two different things - it checks whether mptcp was requested and if not compiled in, reports an error. Second it sets the mptcp flag. The second thing is suprising given the name of the function but also it delays error reporting until after we've gone through the DNS lookup which I think is undesirable. If we make the 'mptcp' field in QAPI schema use the conditional that I show above, then we make it literally impossible to have the mptcp field set when IPPROTO_MPTCP is unset, avoiding the need to do error reporting at all. IOW, the above 4 lines could be simplified to just #ifdef IPPROTO_MPTCP if (saddr->has_mptcp && saddr->mptcp) { ai->ai_protocol = IPPROTO_MPTCP; } #else > @@ -687,6 +712,15 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp) > } > addr->has_keep_alive = true; > } > + begin = strstr(optstr, ",mptcp"); > + if (begin) { > + if (inet_parse_flag("mptcp", begin + strlen(",mptcp"), > + &addr->mptcp, errp) < 0) > + { > + return -1; > + } > + addr->has_mptcp = true; > + } This reminds me that inet_parse_flag is a bit of a crude design right now, because it only does half the job, leaving half the repeated code pattern in the caller still, with use having the string ",mtcp" /"mptcp" repeated three times ! If you fancy refactoring it, i think it'd make more sense if we could just have a caller pattern of if (inet_parse_flag(optstr, "mptcp", &addr->has_mptcp, &addr->mptcp, errp) < 0) Not a blocker todo this though. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Apr 08, 2021 at 08:11:59PM +0100, Dr. David Alan Gilbert (git) wrote: >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> Multipath TCP allows combining multiple interfaces/routes into a single >> socket, with very little work for the user/admin. >> >> It's enabled by 'mptcp' on most socket addresses: >> >> ./qemu-system-x86_64 -nographic -incoming tcp:0:4444,mptcp >> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> --- >> io/dns-resolver.c | 2 ++ >> qapi/sockets.json | 5 ++++- >> util/qemu-sockets.c | 34 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/io/dns-resolver.c b/io/dns-resolver.c >> index 743a0efc87..b081e098bb 100644 >> --- a/io/dns-resolver.c >> +++ b/io/dns-resolver.c >> @@ -122,6 +122,8 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver, >> .ipv4 = iaddr->ipv4, >> .has_ipv6 = iaddr->has_ipv6, >> .ipv6 = iaddr->ipv6, >> + .has_mptcp = iaddr->has_mptcp, >> + .mptcp = iaddr->mptcp, >> }; >> >> (*addrs)[i] = newaddr; >> diff --git a/qapi/sockets.json b/qapi/sockets.json >> index 2e83452797..43122a38bf 100644 >> --- a/qapi/sockets.json >> +++ b/qapi/sockets.json >> @@ -57,6 +57,8 @@ >> # @keep-alive: enable keep-alive when connecting to this socket. Not supported >> # for passive sockets. (Since 4.2) >> # >> +# @mptcp: enable multi-path TCP. (Since 6.0) >> +# >> # Since: 1.3 >> ## >> { 'struct': 'InetSocketAddress', >> @@ -66,7 +68,8 @@ >> '*to': 'uint16', >> '*ipv4': 'bool', >> '*ipv6': 'bool', >> - '*keep-alive': 'bool' } } >> + '*keep-alive': 'bool', >> + '*mptcp': 'bool' } } > > I think this would need to be > > '*mptcp': { 'type': 'bool', 'if': 'IPPROTO_MPTCP' } > > so that mgmt apps can probe when it truely is supported or not for > this build Yes. Instance of a somewhat common anti-pattern "declare unconditionally (this hunk), write unconditionally (previous hunk), read conditionally (next hunk). Besides defeating introspection, it also exposes configuration knobs that don't do anything. >> >> ## >> # @UnixSocketAddress: >> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c >> index 8af0278f15..72527972d5 100644 >> --- a/util/qemu-sockets.c >> +++ b/util/qemu-sockets.c >> @@ -206,6 +206,21 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e) >> #endif >> } >> >> +static int check_mptcp(const InetSocketAddress *saddr, struct addrinfo *ai, >> + Error **errp) >> +{ >> + if (saddr->has_mptcp && saddr->mptcp) { >> +#ifdef IPPROTO_MPTCP >> + ai->ai_protocol = IPPROTO_MPTCP; >> +#else >> + error_setg(errp, "MPTCP unavailable in this build"); >> + return -1; >> +#endif >> + } >> + >> + return 0; >> +} >> + >> static int inet_listen_saddr(InetSocketAddress *saddr, >> int port_offset, >> int num, [...]
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Thu, Apr 08, 2021 at 08:11:59PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Multipath TCP allows combining multiple interfaces/routes into a single > > socket, with very little work for the user/admin. > > > > It's enabled by 'mptcp' on most socket addresses: > > > > ./qemu-system-x86_64 -nographic -incoming tcp:0:4444,mptcp > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > io/dns-resolver.c | 2 ++ > > qapi/sockets.json | 5 ++++- > > util/qemu-sockets.c | 34 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 40 insertions(+), 1 deletion(-) > > > > diff --git a/io/dns-resolver.c b/io/dns-resolver.c > > index 743a0efc87..b081e098bb 100644 > > --- a/io/dns-resolver.c > > +++ b/io/dns-resolver.c > > @@ -122,6 +122,8 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver, > > .ipv4 = iaddr->ipv4, > > .has_ipv6 = iaddr->has_ipv6, > > .ipv6 = iaddr->ipv6, > > + .has_mptcp = iaddr->has_mptcp, > > + .mptcp = iaddr->mptcp, > > }; > > > > (*addrs)[i] = newaddr; > > diff --git a/qapi/sockets.json b/qapi/sockets.json > > index 2e83452797..43122a38bf 100644 > > --- a/qapi/sockets.json > > +++ b/qapi/sockets.json > > @@ -57,6 +57,8 @@ > > # @keep-alive: enable keep-alive when connecting to this socket. Not supported > > # for passive sockets. (Since 4.2) > > # > > +# @mptcp: enable multi-path TCP. (Since 6.0) > > +# > > # Since: 1.3 > > ## > > { 'struct': 'InetSocketAddress', > > @@ -66,7 +68,8 @@ > > '*to': 'uint16', > > '*ipv4': 'bool', > > '*ipv6': 'bool', > > - '*keep-alive': 'bool' } } > > + '*keep-alive': 'bool', > > + '*mptcp': 'bool' } } > > I think this would need to be > > '*mptcp': { 'type': 'bool', 'if': 'IPPROTO_MPTCP' } > > so that mgmt apps can probe when it truely is supported or not for > this build Done; now remember that just tells you if your C library knows about it, not whether your kernel, firewall, or avian carriers know about it. > > > > ## > > # @UnixSocketAddress: > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > > index 8af0278f15..72527972d5 100644 > > --- a/util/qemu-sockets.c > > +++ b/util/qemu-sockets.c > > @@ -206,6 +206,21 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e) > > #endif > > } > > > > +static int check_mptcp(const InetSocketAddress *saddr, struct addrinfo *ai, > > + Error **errp) > > +{ > > + if (saddr->has_mptcp && saddr->mptcp) { > > +#ifdef IPPROTO_MPTCP > > + ai->ai_protocol = IPPROTO_MPTCP; > > +#else > > + error_setg(errp, "MPTCP unavailable in this build"); > > + return -1; > > +#endif > > + } > > + > > + return 0; > > +} > > + > > static int inet_listen_saddr(InetSocketAddress *saddr, > > int port_offset, > > int num, > > @@ -278,6 +293,11 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > > > /* create socket + bind/listen */ > > for (e = res; e != NULL; e = e->ai_next) { > > + if (check_mptcp(saddr, e, &err)) { > > + error_propagate(errp, err); > > + return -1; > > + } > > So this is doing two different things - it checks whether mptcp was > requested and if not compiled in, reports an error. Second it sets > the mptcp flag. The second thing is suprising given the name of > the function but also it delays error reporting until after we've > gone through the DNS lookup which I think is undesirable. > > If we make the 'mptcp' field in QAPI schema use the conditional that > I show above, then we make it literally impossible to have the mptcp > field set when IPPROTO_MPTCP is unset, avoiding the need to do error > reporting at all. > > IOW, the above 4 lines could be simplified to just > > #ifdef IPPROTO_MPTCP > if (saddr->has_mptcp && saddr->mptcp) { > ai->ai_protocol = IPPROTO_MPTCP; > } > #else OK, done - (with a #endif) > > > > @@ -687,6 +712,15 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp) > > } > > addr->has_keep_alive = true; > > } > > + begin = strstr(optstr, ",mptcp"); > > + if (begin) { > > + if (inet_parse_flag("mptcp", begin + strlen(",mptcp"), > > + &addr->mptcp, errp) < 0) > > + { > > + return -1; > > + } > > + addr->has_mptcp = true; > > + } > > This reminds me that inet_parse_flag is a bit of a crude design right > now, because it only does half the job, leaving half the repeated code > pattern in the caller still, with use having the string ",mtcp" /"mptcp" > repeated three times ! Yeh I noticed that. > If you fancy refactoring it, i think it'd make more sense if we could > just have a caller pattern of > > if (inet_parse_flag(optstr, > "mptcp", > &addr->has_mptcp, > &addr->mptcp, errp) < 0) > > Not a blocker todo this though. A job for another day. Dave > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
diff --git a/io/dns-resolver.c b/io/dns-resolver.c index 743a0efc87..b081e098bb 100644 --- a/io/dns-resolver.c +++ b/io/dns-resolver.c @@ -122,6 +122,8 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver, .ipv4 = iaddr->ipv4, .has_ipv6 = iaddr->has_ipv6, .ipv6 = iaddr->ipv6, + .has_mptcp = iaddr->has_mptcp, + .mptcp = iaddr->mptcp, }; (*addrs)[i] = newaddr; diff --git a/qapi/sockets.json b/qapi/sockets.json index 2e83452797..43122a38bf 100644 --- a/qapi/sockets.json +++ b/qapi/sockets.json @@ -57,6 +57,8 @@ # @keep-alive: enable keep-alive when connecting to this socket. Not supported # for passive sockets. (Since 4.2) # +# @mptcp: enable multi-path TCP. (Since 6.0) +# # Since: 1.3 ## { 'struct': 'InetSocketAddress', @@ -66,7 +68,8 @@ '*to': 'uint16', '*ipv4': 'bool', '*ipv6': 'bool', - '*keep-alive': 'bool' } } + '*keep-alive': 'bool', + '*mptcp': 'bool' } } ## # @UnixSocketAddress: diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 8af0278f15..72527972d5 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -206,6 +206,21 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e) #endif } +static int check_mptcp(const InetSocketAddress *saddr, struct addrinfo *ai, + Error **errp) +{ + if (saddr->has_mptcp && saddr->mptcp) { +#ifdef IPPROTO_MPTCP + ai->ai_protocol = IPPROTO_MPTCP; +#else + error_setg(errp, "MPTCP unavailable in this build"); + return -1; +#endif + } + + return 0; +} + static int inet_listen_saddr(InetSocketAddress *saddr, int port_offset, int num, @@ -278,6 +293,11 @@ static int inet_listen_saddr(InetSocketAddress *saddr, /* create socket + bind/listen */ for (e = res; e != NULL; e = e->ai_next) { + if (check_mptcp(saddr, e, &err)) { + error_propagate(errp, err); + return -1; + } + getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, uaddr,INET6_ADDRSTRLEN,uport,32, NI_NUMERICHOST | NI_NUMERICSERV); @@ -456,6 +476,11 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp) for (e = res; e != NULL; e = e->ai_next) { error_free(local_err); local_err = NULL; + + if (check_mptcp(saddr, e, &local_err)) { + break; + } + sock = inet_connect_addr(saddr, e, &local_err); if (sock >= 0) { break; @@ -687,6 +712,15 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp) } addr->has_keep_alive = true; } + begin = strstr(optstr, ",mptcp"); + if (begin) { + if (inet_parse_flag("mptcp", begin + strlen(",mptcp"), + &addr->mptcp, errp) < 0) + { + return -1; + } + addr->has_mptcp = true; + } return 0; }