From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34761) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBlbZ-0005ms-2T for qemu-devel@nongnu.org; Wed, 23 Dec 2015 10:45:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBlbU-00044G-Se for qemu-devel@nongnu.org; Wed, 23 Dec 2015 10:45:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49348) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBlbU-00043k-Lq for qemu-devel@nongnu.org; Wed, 23 Dec 2015 10:45:28 -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 1B203A789 for ; Wed, 23 Dec 2015 15:45:28 +0000 (UTC) References: <1450808260-17922-1-git-send-email-berrange@redhat.com> <56799F57.1080609@redhat.com> <20151223113214.GH20028@redhat.com> From: Eric Blake Message-ID: <567AC197.3050002@redhat.com> Date: Wed, 23 Dec 2015 08:45:27 -0700 MIME-Version: 1.0 In-Reply-To: <20151223113214.GH20028@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="eAH3eTh0iWxb22prb1c6HeWVS9TafSVuH" 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) --eAH3eTh0iWxb22prb1c6HeWVS9TafSVuH Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 12/23/2015 04:32 AM, Daniel P. Berrange wrote: > On Tue, Dec 22, 2015 at 12:07:03PM -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. >>> >> >> [code review, if we go with this design; see my other message for 2 >> possible alternative qapi designs that may supersede this code review]= >> >>> ## >>> -{ 'struct': 'ChardevDummy', 'data': { } } >>> +{ 'struct': 'ChardevDummy', 'data': { }, >>> + 'base': 'ChardevCommon' } >> >> Instead of changing ChardevDummy, you could have deleted this type and= done: >> >>> =20 >>> { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile', >>> 'serial' : 'ChardevHostdev', >> ... >> 'pty': 'ChardevCommon', >> 'null': 'ChardevCommon', >> >> and so on. I don't know which is better. >=20 > Yep, me neither. Given the name it felt like some kind of placeholder > for future work, so I left it alone. ChardevDummy exists because we have no way (yet) to represent an empty type as a union branch. That is, since you can't do: 'pty': {}, we had to instead create a named empty type. But now that we have a non-empty type, I see no real reason to keep the name, and no reason to have a subclass that adds no additional fields; so dropping ChardevDummy is my recommendation. >> >> The other thing we could do here is have qemu_chr_open_common() take a= >> ChardevCommon* instead of a ChardevBackend*. Then each caller would d= o >> an appropriate upcast before calling the common code: >> >> ChardevCommon *common =3D qapi_ChardevDummy_base(backend->u.null); >> if (qemu_chr_open_common(chr, common, errp) { >=20 > Yep, I think this is the easiest thing todo, to avoid use of > backend->u.data. >=20 >> and so forth. That feels a bit more type-safe (and less reliant on qa= pi >> layout guarantees) than trying to rely on the backend->u.data that I'm= >> trying to get rid of. >=20 > Agreed >=20 Okay, I think having each branch be a subclass of ChardevCommon (and not ChardevBackend using ChardevCommon as a base) sounds like the way to go, and now it's up to v3 to rework the clients to be a bit more typesafe. >>> +++ b/qemu-options.hx >>> @@ -2034,40 +2034,43 @@ The general form of a character device option= is: >>> ETEXI >>> =20 >>> DEF("chardev", HAS_ARG, QEMU_OPTION_chardev, >>> - "-chardev null,id=3Did[,mux=3Don|off]\n" >>> + "-chardev null,id=3Did[,mux=3Don|off][,logfile=3DPATH][,logappen= d=3Don|off]\n" >> >> Do we want to allow logappend=3Don even when logfile is unspecified, o= r >> should we make that an error? >=20 > I figured it was harmless to just ignore it. Works for me. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --eAH3eTh0iWxb22prb1c6HeWVS9TafSVuH 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/ iQEcBAEBCAAGBQJWesGXAAoJEKeha0olJ0NqTvUH/RH/HE/ePPupHf4nCFjBD/Pj Fo8GUt3SBsIAPY4CbtnM09IP9neaeXFu4sOuRNpkZbsinhr7KWEdCfndvnlhxJc/ PsBni7h9w5uHf+3YcyLRJe5fSdTWs0zNkdIOl/4hQbE66dxOhYsO5pqmAoi5sYsG oTZl9yhrMXbMe+7i/eXroAK+vmM9M8ttAGQpwRaY4f+v2Cn1HH061SLqmrbKIgao py0Lnyzhh6wgKyOm1uHRkE0sWvM1806fTGkuLRubTi+VmTl16qbX1ABzm0cUPwZC Goe9PeKwxya9PnpqueLyzgiPyTORzGT5+r2Y1OgYL8/FQghGZ/Zo4GHWokUejmU= =dLuV -----END PGP SIGNATURE----- --eAH3eTh0iWxb22prb1c6HeWVS9TafSVuH--