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

[Eric Blake]

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" } } } }

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.

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).

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.

> The
> ChardevCommon struct provides the optional 'logfile'
> parameter, as well as 'logappend' which controls
> whether QEMU truncates or appends (default truncate).
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
> 


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature