On Thu, Dec 21, 2017 at 05:49:14PM +0100, Markus Armbruster wrote:
> QAPI schema review only.
>
> "Daniel P. Berrange" <berra...@redhat.com> writes:
>
> > When starting QEMU management apps will usually setup a monitor socket, and
> > then open it immediately after startup. If not using QEMU's own -daemonize
> > arg, this process can be troublesome to handle correctly. The mgmt app will
> > need to repeatedly call connect() until it succeeds, because it does not
> > know when QEMU has created the listener socket. If can't retry connect()
> > forever though, because an error might have caused QEMU to exit before it
> > even creates the monitor.
> >
> > The obvious way to fix this kind of problem is to just pass in a pre-opened
> > socket file descriptor for the QEMU monitor to listen on. The management app
> > can now immediately call connect() just once. If connect() fails it knows
> > that QEMU has exited with an error.
> >
> > The SocketAddress(Legacy) structs allow for FD passing via the monitor,
> > using
> > the 'getfd' command, but only when using QMP JSON syntax. The HMP syntax has
> > no way to initialize the SocketAddress(Legacy) 'fd' variant. So this patch
> > first wires up the 'fd' parameter to refer to a monitor file descriptor,
> > allowing HMP to use
> >
> > getfd myfd
> > chardev-add socket,fd=myfd
> >
> > The SocketAddress 'fd' variant is currently tied to the use of the monitor
> > 'getfd' command, so we have a chicken & egg problem with reusing that at
> > startup wher no monitor connection is available. We could define that the
>
> s/wher/where/
>
> > special fd name prefix '/dev/fdset' refers to a FD passed via the CLI, but
> > magic strings feel unpleasant.
> >
> > Instead we define a SocketAddress 'fdset' variant that takes an fd set
> > number
> > that works in combination with the 'add-fd' command line argument. e.g.
> >
> > -add-fd fd=3,set=1
> > -chardev socket,fdset=1,id=mon
> > -mon chardev=mon,mode=control
> >
> > Note that we do not wire this up in the legacy chardev syntax, so you cannot
> > use FD passing with '-qmp', you must use the modern '-mon' + '-chardev' pair
> >
> > An illustrative example of usage is:
> >
> > #!/usr/bin/perl
> >
> > use IO::Socket::UNIX;
> > use Fcntl;
> >
> > unlink "/tmp/qmp";
> > my $srv = IO::Socket::UNIX->new(
> > Type => SOCK_STREAM(),
> > Local => "/tmp/qmp",
> > Listen => 1,
> > );
> >
> > my $flags = fcntl $srv, F_GETFD, 0;
> > fcntl $srv, F_SETFD, $flags & ~FD_CLOEXEC;
> >
> > my $fd = $srv->fileno();
> >
> > exec "qemu-system-x86_64", \
> > "-add-fd", "fd=$fd,set=1", \
> > "-chardev", "socket,fdset=1,server,nowait,id=mon", \
> > "-mon", "chardev=mon,mode=control";
> >
> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> [...]
> > diff --git a/qapi/common.json b/qapi/common.json
> > index 6eb01821ef..a15cdc36e9 100644
> > --- a/qapi/common.json
> > +++ 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.
>
> I figure you got the "to be embedded in lists" part from @String. That
> one's occasionally used as list element type, but there are other uses.
> @Int has only such other uses so far. Let's drop this line from both types.
>
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'Int',
> > + 'data': {
> > + 'i': 'int' } }
> > +
> > ##
> > # @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' } }
> >
> > ##
> > # @SocketAddressType:
> > @@ -123,10 +124,16 @@
> > #
> > # @unix: Unix domain socket
> > #
> > +# @vsock: VSOCK socket
> > +#
> > +# @fd: socket file descriptor passed over monitor
> > +#
>
> Indepedent doc fix. I'd put it in a separate patch.
>
> One inaccuracy: @fd is *not* a file descriptor, it's the *name* of a
> file descriptor. Please fix.
>
> > +# @fdset: socket file descriptor passed via CLI (since 2.12)
> > +#
>
> I gather we have to ways to pass file descriptors. One way identifies
> them by name (member @fd), the other by numeric ID (member @fdset).
>
> 0. This is disgusting. Is there any way to unify the two, and deprecate
> the loser (hopefully the numeric one)?
Checkout my v2 which takes a different, less disgusting approach.
> 1. What makes the second one a *set*?
Just that the API i used was called fdset.
> 2. What ties the second one to the CLI? Accidents of implementation or
> something deeper?
Nothing strict - just convention of usage. You could technically (ab)use
it from the monitor too. My v2 approach enforces the distinct usage
more strictly.
>
> > # Since: 2.9
> > ##
> > { 'enum': 'SocketAddressType',
> > - 'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
> > + 'data': [ 'inet', 'unix', 'vsock', 'fd', 'fdset' ] }
> >
> > ##
> > # @SocketAddress:
> > @@ -144,4 +151,5 @@
> > 'data': { 'inet': 'InetSocketAddress',
> > 'unix': 'UnixSocketAddress',
> > 'vsock': 'VsockSocketAddress',
> > - 'fd': 'String' } }
> > + 'fd': 'String',
> > + 'fdset': 'Int' } }
> [...]
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 :|
