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

[Stefano Brivio]

On Mon, 26 Apr 2021 13:56:40 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

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

Ah, right.

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

Yes, I forgot about this here, but it's actually already in my patch
(see the changes to net_socket_listen_init() and documentation).

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

Agreed... I didn't actually consider that.

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

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?

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

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

Strictly speaking, for IPv4, addresses are reserved for multicast usage
(by RFC 5771), but as far as I can tell, also other addresses could
legitimately be used for multicast. I've never seen that in practice
and it's unlikely to be of any use, though.

For IPv6, things seem to be defined more strictly (RFC 5771 and
updates).

All in all, yes, I guess inferring this from the address would make the
usage more practical.

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

...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"?).

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

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?

-- 
Stefano