Re: [Qemu-devel] [PATCH v2] qemu-char: add logfile facility to all chardev backends

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Dec 22, 2015 at 11:45:45AM -0700, Eric Blake wrote:
> On 12/22/2015 11:17 AM, Daniel P. Berrange wrote:
> > Typically a UNIX guest OS will log boot messages to a serial
> > port in addition to any graphical console. An admin user
> > may also wish to use the serial port for an interactive
> > console. A virtualization management system may wish to
> > collect system boot messages by logging the serial port,
> > but also wish to allow admins interactive access.
> 
> [meta-review of JUST qapi decisions; code review in a separate message]
> 
> > 
> > Currently providing such a feature forces the mgmt app
> > to either provide 2 separate serial ports, one for
> > logging boot messages and one for interactive console
> > login, or to proxy all output via a separate service
> > that can multiplex the two needs onto one serial port.
> > While both are valid approaches, they each have their
> > own downsides. The former causes confusion and extra
> > setup work for VM admins creating disk images. The latter
> > places an extra burden to re-implement much of the QEMU
> > chardev backends logic in libvirt or even higher level
> > mgmt apps and adds extra hops in the data transfer path.
> > 
> > A simpler approach that is satisfactory for many use
> > cases is to allow the QEMU chardev backends to have a
> > "logfile" property associated with them.
> > 
> >  $QEMU -chardev socket,host=localhost,port=9000,\
> >                 server=on,nowait,id-charserial0,\
> > 		logfile=/var/log/libvirt/qemu/test-serial0.log
> >        -device isa-serial,chardev=charserial0,id=serial0
> > 
> > This patch introduces a 'ChardevCommon' struct which
> > is setup as a base for all the ChardevBackend types.
> > Ideally this would be registered directly as a base
> > against ChardevBackend, rather than each type, but
> > the QAPI generator doesn't allow that since the
> > ChardevBackend is a non-discriminated union.
> 
> It is possible to convert ChardevBackend into a discriminated union
> while still keeping the same QMP semantics.
> 
> But where it gets interesting is what the QMP semantics should be.
> 
> Right now, we have (simplifying to just two branches, for less typing):
> { 'union': 'ChardevBackend',
>   'data': { 'file': 'ChardevFile',
>             'serial': 'ChardevHostdev' } }
> 
> which means we support:
> 
> { "execute": "chardev-add", "arguments": {
>     "id": "foo", "backend": { "type": "file",
>        "data": { "out": "filename" } } } }
> 
> With your addition, ChardevFile now inherits from ChardevCommon, so we gain:
> 
> { "execute": "chardev-add", "arguments": {
>     "id": "foo", "backend": { "type": "file",
>       "data": { "logfile": "logname", "out": "filename" } } } }

Ok good that matches what I intended - namely that 'logfile'
should appear at the same dict as the rest of the backend
fields, to mirror the the fact that the C struct had all
the common fields in the same struct too.

> Re-writing the existing ChardevBackend to a semantically-identical flat
> union would be (using my shorthand syntax for anonymous base [1] and
> anonymous branch wrappers [2]):
> 
> { 'enum': 'ChardevType', 'data': [ 'file', 'serial' ] }
> { 'union': 'ChardevBackend',
>   'base': { 'type': 'ChardevType' }, 'discriminator': 'type',
>   'data': { 'file': { 'data': 'ChardevFile' },
>             'serial': { 'data': 'ChardevHostdev' } } }
> 
> [1] http://repo.or.cz/qemu/ericb.git/commitdiff/dbb8680b1
> [2] not yet posted to list or my git repo
> 
> Note that in my conversion, 'file' is no longer directly a
> 'ChardevFile', but an anonymous type with one field named 'data' where
> _that_ field is a ChardevFile; necessary to keep the existing QMP
> nesting the same.
> 
> Your proposal, then, is that the new 'logging' fields in your
> ChardevCommon should be made part of the base type of the
> 'ChardevBackend' union; which would be spelled as:
> 
> { 'enum': 'ChardevType', 'data': [ 'file', 'serial' ] }
> { 'struct': 'ChardevCommon', 'data': {
>   'type': 'ChardevType', '*logfile': 'str', '*logappend': bool } }
> { 'union': 'ChardevBackend',
>   'base': 'ChardevCommon', 'discriminator': 'type',
>   'data': { 'file': { 'data': 'ChardevFile' },
>             'serial': { 'data': 'ChardevHostdev' } } }
> 
> But done that way, the QMP wire form would be:
> 
> { "execute": "chardev-add", "arguments": {
>     "id": "foo", "backend": { "type": "file",
>       "logfile": "logname", "data": { "out": "filename" } } } }
> 
> Note the difference: "logfile" changes from being a child of "data" to
> being a sibling.

Ok, so that's still backwards compatible, but the 'logfile' is
appearing in an expected place IMHO.

> Hmm - now that I've typed all that, I wonder if it would make more sense
> to instead just make these parameters be siblings of "backend", by
> instead modifying:
> 
> { 'command': 'chardev-add', 'data': {
>     'id': 'str', 'backend': 'ChardevBackend',
>     '*logfile': 'str', '*logappend': bool } }
> 
> where the QMP command would be:
> 
> { "execute": "chardev-add", "arguments": {
>     "id": "foo", "logfile": "logname", "backend": { "type": "file",
>       "data": { "out": "filename" } } } }
> 
> But while that would certainly be less invasive to the qapi, it may make
> life harder for the C implementation (it's no longer associated with the
> ChardevBackend struct, but has to be tracked separately).

Yeah, that would require a bit of refactoring, since many of the
codepaths I'm changing only get passed in the 'ChardevBackend'
struct, not its parent owner.

> So, depending on which of those three places we want to stick the new
> parameters determines which approach we should use for exposing them in
> qapi.

>From the QMP representation POV, my preference is for the current
design since I think the 'logfile' attribute is really just another
one of the backend config attributes. The choice to have a ChardevCommon
struct was just a mechanism to avoid having to repeat the 'logfile'
parameter in every single Chardev* backend type. This naturally
matches the C-struct, where the ChardevCommon fields get directly
added to the ChardevFile, ChardevSocket, etc structs.

So from that POV, I'd be against, pushing the 'logfile' field up
either 1 or 2 levels.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|