Re: [RFC PATCH 5/5] sockets: Support multipath TCP

["Daniel P. Berrangé" <berrange@redhat.com>]

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
-- 
|: 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 :|