Re: socket.c added support for unix domain socket datagram transport

[Daniel P . Berrangé]

On Tue, Apr 27, 2021 at 11:52:29PM +0200, Stefano Brivio wrote:
> On Mon, 26 Apr 2021 13:56:40 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The pain we're feeling is largely because the design of the net
> > option syntax is one of the oldest parts of QEMU and has only
> > been very partially improved upon. It is certainly not using
> > QAPI best practice, if we look at this:
> > 
> >   { 'struct': 'NetdevSocketOptions',
> >     'data': {
> >       '*fd':        'str',
> >       '*listen':    'str',
> >       '*connect':   'str',
> >       '*mcast':     'str',
> >       '*localaddr': 'str',
> >       '*udp':       'str' } }
> > 
> > Then some things come to mind
> > 
> >  - We're not provinding a way to say what kind of "fd" is
> >    passed in - is it a UDP/TCP FD, is it a listener or
> >    client FD, is it unicast or multicast FD. Though QEMU
> >    can interogate the socket to discover this I guess.
> 
> Some form of probing was already added in commit 894022e61601 ("net:
> check if the file descriptor is valid before using it"). Does qemu need
> to care, though, once the socket is connected? That is, what would it
> do with that information?

The only thing it really has to care about is the distinction between
a listener socket and a data socket.

> >  - All of the properties there except "fd" are encoding two values
> >    in a single property - address + port. This is an anti-pattern
> > 
> >  - No support for ipv4=on|off and ipv6=on|off flags to control
> >    dual-stack usage.
> 
> I wonder if this needs to be explicit -- it might simply derive from
> addressing.

This is explicitly everywhere we use sockets in QEMU - it is part
of the SocketAddress QAPI schema.

Consider an address "::", this is an IPv6 address, but depending on
how you configure the socket it can accept either IPv6-only or both
IPv6 and IPv4 incoming connections.

If passing a hostname instead of a raw address, then  the ipv4/ipv6
flags control whether we use all the returned DNS entries.

> > The "right" way to fix most of this long term is a radical change
> > to introduce use of the SocketAddress struct.
> > 
> > I could envisage something like this
> > 
> >   { 'enum': 'NetdevSocketMode',
> >     'data':  ['dgram', 'client', 'server'] }
> > 
> >   { 'struct': 'NetdevSocketOptions',
> >     'data': {
> >       'addr':      'SocketAddress',
> >       '*localaddr': 'SocketAddress',
> >       '*mode':      'NetdevSocketMode' } }
> > 
> > 
> >  - A TCP client
> > 
> >       addr.type = inet
> >       addr.host = 192.168.1.1
> >       mode = client
> >
> >  - A TCP server on all interfaces
> > 
> >       addr.type = inet
> >       addr.host = 
> >       mode = server
> > 
> >  - A TCP server on a specific interface
> > 
> >       addr.type = inet
> >       addr.host = 192.168.1.1
> >       mode = server
> > 
> >  - A TCP server on all interfaces, without IPv4
> > 
> >       addr.type = inet
> >       addr.host = 
> >       addr.has_ipv4 = true
> >       addr.ipv4 = false
> >       mode = server
> 
> ...perhaps it would be more consistent with other consolidated
> practices to have addr.type = inet | inet6... and perhaps call it
> addr.family.
>
> Also, for "mode" (that could be called "type" to reflect
> parameters for socket(2)), we should probably support SOCK_SEQPACKET as
> well ("seq"?).

The naming I use here is determined by the QAPI 'SocketAddress'
struct which has a 'type' field, and separate 'ipv4' and 'ipv6'
flags.

> 
> >  - A UDP unicast transport
> > 
> >       addr.type = inet
> >       addr.host = 192.168.1.1
> >       mode = dgram
> > 
> >  - A UDP unicast transport with local addr
> > 
> >       addr.type = inet
> >       addr.host = 192.168.1.1
> >       localaddr.type = inet
> >       localaddr.host = 192.168.1.2
> >       mode = dgram
> > 
> >  - A UDP multicast transport
> > 
> >      addr.type = inet
> >      addr.host = 224.0.23.166
> >      mode = dgram
> > 
> >  - A UNIX stream client
> > 
> >       addr.type = unix
> >       addr.path = /some/socket
> >       mode = client
> > 
> >  - A UNIX stream server
> > 
> >       addr.type = unix
> >       addr.path = /some/socket
> >       mode = server
> > 
> >  - A UNIX dgram transport
> > 
> >       addr.type = unix
> >       addr.path = /some/socket
> >       mode = dgram
> > 
> > 
> > Now, of course you're just interested in adding UNIX socket support.
> > 
> > This design I've outlined above is very much "boiling the ocean".
> > Thus I'm not requesting you implement this, unless you have a strong
> > desire to get heavily involved in some QEMU refactoring work.
> 
> I don't really have a strong desire to do that, but to my naive eyes it
> doesn't look that complicated, I'll give it a try.

The hard bit is always the backwards compatibility for existing usage....


> > The key question is whether we try to graft UNIX socket support onto
> > the existing args ("connect"/"listen") or try to do something more
> > explicit.
> > 
> > Given the desire to have both dgram + stream support, I'd be inclined
> > to do something more explicit, that's slightly more aligned with a
> > possible future best praactice QAPI design
> > 
> > 
> > IOW, we could take a simplified variant of the above as follows:
> > 
> > 
> >   { 'enum': 'NetdevSocketMode',
> >     'data':  ['dgram', 'client', 'server'] }
> > 
> >   { 'struct': 'NetdevSocketOptions',
> >     'data': {
> >       '*fd':        'str',
> >       '*listen':    'str',
> >       '*connect':   'str',
> >       '*mcast':     'str',
> >       '*localaddr': 'str',
> >       '*udp':       'str',
> >       '*path':      'str' } }
> >       '*localpath': 'str' } }
> > 
> > 
> > Cli examples:
> > 
> >  * Unix stream client
> > 
> >   -netdev socket,path=/wibble,mode=client
> > 
> >  * Unix stream server
> >  
> >   -netdev socket,path=/wibble,mode=server
> > 
> >  * Unix datagram 
> > 
> >   -netdev socket,path=/wibble,mode=dgram
> >   -netdev socket,path=/wibble,localpath=/fish,mode=dgram
> 
> I think this looks reasonable, I'm not sure about "localpath",
> because also /wibble is local in some sense.

"local" as in local to the process, rather than "local" as in
local to the host.

> I don't know what would be a good implementation practice to keep the
> current options available -- should this be done with some new code
> that applies a translation to the new parameters?

At the CLI parser side we'd just do translation to the new QAPI style
usually, but I'm not sure how to handle the existing QAPI stuff though.

Perhaps just add new fields to "NetdevSocketOptions" and deprecate
existing ones that become obsolete.

The only other alternative is a parallel type to completely obsolete
NetdevSocketOptions, but I'm not sure what we'd call that.

I had added Markus / Eric to CC to get advice on QAPI side here..

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