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 :|
