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

[Daniel P . Berrangé]

On Fri, Apr 23, 2021 at 06:54:08PM +0200, Stefano Brivio wrote:
> On Fri, 23 Apr 2021 17:21:38 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The current IP socket impl for the net socket backend uses SOCK_DGRAM,
> > so from a consistency POV it feels sensible todo the same for UNIX
> > sockets too.
> 
> That's just for UDP though -- it also supports TCP with the "connect="
> parameter, and given that a stream-oriented AF_UNIX socket behaves very
> similarly, I recycled that parameter and just extended that bit of
> documentation.
> 
> > None the less, your last point in particular about wanting to know
> > about disconnects feels valid, and if its useful to you for UNIX
> > sockets, then it ought to be useful for IP sockets too.
> > 
> > IOW, I wonder if  we should use DGRAM for UNIX sockets too by default
> > to match current behaviour, but then also add a CLI option that allows
> > choice of DGRAM vs STREAM, and wire that up for IP & UNIX sockets.
> 
> The choice would only apply to AF_UNIX (that is, not to TCP and UDP).
> 
> The current situation isn't entirely consistent, because for TCP you
> have "connect=", for UDP it's "udp=" or "mcast=", and I'm extending the
> "connect=" case to support stream-oriented AF_UNIX, which I think is
> consistent.
> 
> However, to have it symmetric for the datagram-oriented case
> (UDP and AF_UNIX), ideally it should be changed to
> "dgram=host:port|path" -- which I guess we can't do.
> 
> I see two alternatives:
> 
> 1.
>   - "connect=" (TCP only)
>   - "unix=path,type=dgram|stream"
>   - "udp=" (UDP only)

This doesn't work when you need the UNIX server to be a
listener socket, as we've no way to express that, without
adding yet another parameter.

> 2.
>   - "connect=" (TCP and AF_UNIX stream)
>   - "unix_dgram="
>   - "udp=" (UDP only)

Also needs

   "listen=" (TCP and AF_UNIX stream)

"udp" has a corresponding optional "localaddr" for the sending
address.

Also overloading "connect" means we have to parse the value
to guess whether its a UNIX path or a IP addr:port pair.

I doubt people will have UNIX paths called "127.0.0.1:3333"
but if we can avoid such ambiguity by design, it is better.

> The major thing I like of 2. is that we save some code and a further
> option, but other than that I don't have a strong preference.

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.

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

 - Redundancy of separate parameters for "mcast" and "udp" when
   it is distinguishable based on the address given AFAIR.

 - No support for UNIX sockets


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

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

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




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