Re: [Qemu-devel] [PATCH v1 2/2] char: allow passing pre-opened socket file descriptor at startup

[Eric Blake]

On 12/21/2017 07:48 AM, Marc-André Lureau wrote:

> >       ChardevSocket *sock;
> > +    int num = 0;
> > +
> > +    if (path) {
> > +        num++;
> > +    }
> > +    if (fd) {
> > +        num++;
> > +    }
> > +    if (fdset) {
> > +        num++;
> > +    }
> > +    if (host) {
> > +        num++;
> > +    }
> > +    if (num != 1) {
> > +        error_setg(errp,
> > +                   "Exactly one of 'path', 'fd', 'fdset' or 'host' required");
> > +        return;
> > +    }
>
> That could be shorter ;)

Here's one way:

if (!!path + !!fd + !!fdset ++ !!host) {
    error_setg...

> > +++ b/qapi/common.json
> > @@ -74,6 +74,17 @@
> >   { 'enum': 'OnOffSplit',
> >     'data': [ 'on', 'off', 'split' ] }
> >
> > +##
> > +# @Int:
> > +#
> > +# A fat type wrapping 'int', to be embedded in lists.
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'Int',
> > +  'data': {
> > +    'i': 'int' } }
> > +

The documentation is odd - you need this type because flat unions 
require a struct rather than a native type for each branch, and not 
because you are embedding 'int' in lists (that is, we support ['int'] 
just fine; the documentation for other fat types pre-dates when we 
supported lists of native types).

> >   ##
> >   # @String:
> >   #
> > diff --git a/qapi/sockets.json b/qapi/sockets.json
> > index ac022c6ad0..f3cac02166 100644
> > --- a/qapi/sockets.json
> > +++ b/qapi/sockets.json
> > @@ -112,7 +112,8 @@
> >       'inet': 'InetSocketAddress',
> >       'unix': 'UnixSocketAddress',
> >       'vsock': 'VsockSocketAddress',
> > -    'fd': 'String' } }
> > +    'fd': 'String',
> > +    'fdset': 'Int' } }

This is SocketAddressLegacy, which is an old-style union.  Do we really 
want to be expanding it at this point in time?  Old-style unions support:

'fd':'str',
'fdset':'int'

except that we already used the fat 'String', so using the fat 'Int' is 
done for consistency more than anything else, if we really do want it 
added to this type.

Missing documentation of when the new branch types were added.

> >   ##
> >   # @SocketAddressType:
> > @@ -123,10 +124,16 @@
> >   #
> >   # @unix:  Unix domain socket
> >   #
> > +# @vsock: VSOCK socket
> > +#
> > +# @fd: socket file descriptor passed over monitor
> > +#
> > +# @fdset: socket file descriptor passed via CLI (since 2.12)
> > +#
> >   # Since: 2.9
> >   ##
> >   { 'enum': 'SocketAddressType',
> > -  'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
> > +  'data': [ 'inet', 'unix', 'vsock', 'fd', 'fdset' ] }

Hmm, good catch on the missing vsock documentation (SocketAddressType 
was named SocketAddressFlatType back in 2.9, but 'vsock' did exist back 
then).

> >   ##
> >   # @SocketAddress:
> > @@ -144,4 +151,5 @@
> >     'data': { 'inet': 'InetSocketAddress',
> >               'unix': 'UnixSocketAddress',
> >               'vsock': 'VsockSocketAddress',
> > -            'fd': 'String' } }
> > +            'fd': 'String',
> > +            'fdset': 'Int' } }

Again, missing documentation on when the branch was added.

But here, you are absolutely correct that we need the fat 'Int' here, as 
this is a flat union, which requires a struct for each branch (not 
native scalar types).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org