* Daniel P. Berrangé (berra...@redhat.com) wrote: > On Thu, Apr 08, 2021 at 08:11:59PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@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 :| -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK