From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBlXQ-0003kk-0d for qemu-devel@nongnu.org; Wed, 23 Dec 2015 10:41:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBlXL-0001kW-QT for qemu-devel@nongnu.org; Wed, 23 Dec 2015 10:41:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36186) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBlXL-0001k4-HI for qemu-devel@nongnu.org; Wed, 23 Dec 2015 10:41:11 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 0189E8E23B for ; Wed, 23 Dec 2015 15:41:10 +0000 (UTC) References: <1450808260-17922-1-git-send-email-berrange@redhat.com> <56799A59.90709@redhat.com> <20151223112454.GG20028@redhat.com> From: Eric Blake Message-ID: <567AC091.8010709@redhat.com> Date: Wed, 23 Dec 2015 08:41:05 -0700 MIME-Version: 1.0 In-Reply-To: <20151223112454.GG20028@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="v4KMrvKp4oifqvnl32lAGPvecfAm9HcbJ" 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" Cc: Paolo Bonzini , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --v4KMrvKp4oifqvnl32lAGPvecfAm9HcbJ Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 12/23/2015 04:24 AM, Daniel P. Berrange wrote: > 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= ] >> >> >> 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" } } } } >=20 > 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. Or in C terms, your proposal is: struct ChardevCommon { char *logname; ... }; struct ChardevFile { /* inherited fields from ChardevCommon */ char *logname; ... /* own fields */ char *out; ... }; struct ChardevBackend { ChardevBackendKind type; union { ChardevFile *file; ... } u; }; Each branch of ChardevBackend.u then has an upcast function qapi_ChardevFile_base() that gets you to a ChardevCommon pointer. >=20 >> Re-writing the existing ChardevBackend to a semantically-identical fla= t >> union would be (using my shorthand syntax for anonymous base [1] and >> anonymous branch wrappers [2]): >> >> >> 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' } } } In C terms, this one would be: struct ChardevCommon { char *logname; ... }; struct ChardevFile { char *out; ... }; struct ChardevBackend { /* inherited fields from ChardevCommon */ char *logname; ... /* own fields */ ChardevBackendKind type; union { ChardevFile *file; ... } u; }; Here, you can pass ChardevBackend* directly (and access backend->logname, regardless of which union branch is in use). >> So, depending on which of those three places we want to stick the new >> parameters determines which approach we should use for exposing them i= n >> qapi. >=20 >>>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 ChardevCommo= n > 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. Yep - the decision is up to you whether to add it to each struct used as a branch of ChardevBackend (and each caller then upcasts and passes a ChardevCommon* to the common code), or whether to add it directly to ChardevBackend (and each caller merely passes a ChardevBackend*). >=20 > So from that POV, I'd be against, pushing the 'logfile' field up > either 1 or 2 levels. >=20 > Regards, > Daniel >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --v4KMrvKp4oifqvnl32lAGPvecfAm9HcbJ 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/ iQEcBAEBCAAGBQJWesCRAAoJEKeha0olJ0NqnqEH/1YP3J5kQfQwWcWsmH4+ibqB GL525RATwb8aPKJF1XrBJkWYgqQ0dRc4TDMGc5gC2mLIKuO5L3fBXTALDDpd2QIZ rLOYICeQsyq8n6t8H1D0NGf9/3P2pf7hYUn569/iQLSY6aXLIlhP09BdCouCUSWI oOI/X2sxe88ZHE008H3+vR5p1u0n3ZeBK/DAig4u+liGbvyV2QQWkilNnC33a6TQ sSbdxDJUzaClXeD28xIV4GoYm/MWo5TRRLPAoxpVBHDj9OMi9caUxw8bJGnaJhHR LQzNow7/1AkiXMZsF0h2iNFjZt7j5LZ/hW3wfVuyE/DwM/kpkt9adqMmTxzg4H8= =XSUl -----END PGP SIGNATURE----- --v4KMrvKp4oifqvnl32lAGPvecfAm9HcbJ--