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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> 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

Yes.

More anti-patterns:

      - Of multiple, nominally optional members, you must provide
        exactly one: listen, connect, mcast, udp.

      - Nominally optional member may only be provided together with
        another optional member: localaddr with mcast or udp.

      - Nominally optional member may be mandatory: localaddr with udp.

Such things can't always be avoided.  But they should always make you
think whether you could avoid them with judicious use of union types.

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

Yes.

>> > 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'] }

I understand 'dgram' asks for passing SOCK_DGRAM to socket().  There are
no SOCK_CLIENT, SOCK_SERVER.  I guess they mean active SOCK_STREAM,
i.e. connect(), vs. passive SOCK_STREAM, i.e. listen().  In short:

    SOCK_DGRAM              'dgram'
    SOCK_STREAM, active     'client'
    SOCK_STREAM, passive    'server'

If we add another connection-based type, say SOCK_SEQPACKET, do we get
to pick names for

    SOCK_SEQPACKET, active  ???
    SOCK_SEQPACKET, passive ???

Encoding type and, if applicable, is-active in a single enum may or may
not be a good idea.

>> > 
>> >   { '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.

Uh, "family": "fd" would be odd, wouldn't it?

Reminder:

    { 'union': 'SocketAddress',
      'base': { 'type': 'SocketAddressType' },
      'discriminator': 'type',
      'data': { 'inet': 'InetSocketAddress',
                'unix': 'UnixSocketAddress',
                'vsock': 'VsockSocketAddress',
                'fd': 'String' } }

>> Also, for "mode" (that could be called "type" to reflect
>> parameters for socket(2)), we should probably support SOCK_SEQPACKET as
>> well ("seq"?).

Well, 'mode' is more than just the socket type, it also includes
is-active for certain socket types.

> 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

What about

           addr.type = fd
           addr.fd = some-fd
           mode = dgram

If the file descriptor has the right type, isn't "mode" redundant?  And
what if it doesn't?

>> > 
>> > 
>> > 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'm confused.  Is this related to the socket's address (think
getsockname()) vs. the peer's address (think getpeername())?

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

This is always possible.  At best, it remains just as ugly as it is now.

> The only other alternative is a parallel type to completely obsolete
> NetdevSocketOptions,

Probably the only way to get a "modern" interface.  Whether it's worth
the effort is hard to tell.

>                      but I'm not sure what we'd call that.

If we can come up with a better backend name than "socket", it's easy:
NetdevBetterNameOptions.

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

Hope I could help at least a little.