Daniel P. Berrangé <berra...@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é <berra...@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.
