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é <berra...@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 :|
