From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34025) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBRwa-0001eA-GM for qemu-devel@nongnu.org; Tue, 22 Dec 2015 13:45:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBRwV-0003aB-VT for qemu-devel@nongnu.org; Tue, 22 Dec 2015 13:45:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42378) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBRwV-0003a3-NK for qemu-devel@nongnu.org; Tue, 22 Dec 2015 13:45:51 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 25DC042E5C7 for ; Tue, 22 Dec 2015 18:45:51 +0000 (UTC) References: <1450808260-17922-1-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56799A59.90709@redhat.com> Date: Tue, 22 Dec 2015 11:45:45 -0700 MIME-Version: 1.0 In-Reply-To: <1450808260-17922-1-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uRWjVpaAgKPGv7dk0WC3wpH4UULV0tLg8" Subject: Re: [Qemu-devel] [PATCH v2] qemu-char: add logfile facility to all chardev backends List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --uRWjVpaAgKPGv7dk0WC3wpH4UULV0tLg8 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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] >=20 > 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. >=20 > 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. >=20 > $QEMU -chardev socket,host=3Dlocalhost,port=3D9000,\ > server=3Don,nowait,id-charserial0,\ > logfile=3D/var/log/libvirt/qemu/test-serial0.log > -device isa-serial,chardev=3Dcharserial0,id=3Dserial0 >=20 > 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 ga= in: { "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). >=20 > Signed-off-by: Daniel P. Berrange > --- >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --uRWjVpaAgKPGv7dk0WC3wpH4UULV0tLg8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWeZpaAAoJEKeha0olJ0NqDygIAJ0BGoN2HDCpsMg65YgLKE41 s5koN1uURquNXs+zXoj/PJ4HA2x6CajkC/dehLEit/1UUVO9hBbp40R2AdgEueJx ZFRey+dl8O6VuSRbo4e8mclf9rCmuZCR1FjZZH3T9IYI7cwpduCfTDCpv8RK0gC5 dmpybmJ/89D2XPaTThETWX9ktG6USwZ5QRV/JZt/6QTFKiDvsxWueSpCd8jGMsNL 5/lyhe7KJNHW8gm6WhHqmPtll3FlERuFxZs0z09ljsCU1ud2+xjcyEqw/NabOQ/T bwnsom+H8hINKl/IFMeQ3K5CCvg3wNexu+oPEQQ8FdCNFKl/2/CVAEIXG4rFEvM= =+xXK -----END PGP SIGNATURE----- --uRWjVpaAgKPGv7dk0WC3wpH4UULV0tLg8--