From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38679) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBSHB-00012I-75 for qemu-devel@nongnu.org; Tue, 22 Dec 2015 14:07:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBSH7-0008W6-M4 for qemu-devel@nongnu.org; Tue, 22 Dec 2015 14:07:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60300) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBSH7-0008W2-Co for qemu-devel@nongnu.org; Tue, 22 Dec 2015 14:07:09 -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 22127C0B7E01 for ; Tue, 22 Dec 2015 19:07:09 +0000 (UTC) References: <1450808260-17922-1-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56799F57.1080609@redhat.com> Date: Tue, 22 Dec 2015 12:07:03 -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="jVJXCggV3xuLqUO1Ugi7gsrqkGm7ililk" 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) --jVJXCggV3xuLqUO1Ugi7gsrqkGm7ililk 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. >=20 [code review, if we go with this design; see my other message for 2 possible alternative qapi designs that may supersede this code review] > 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. 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 > +++ b/qapi-schema.json > @@ -3093,6 +3093,21 @@ > ## > { 'command': 'screendump', 'data': {'filename': 'str'} } > =20 > + > +## > +# @ChardevCommon: > +# > +# Configuration shared across all chardev backends > +# > +# @logfile: #optional The name of a logfile to save output > +# @logappend: #optional true to append instead of truncate > +# (default to false to truncate) Could shorten to: # @logappend: #optional true to append to @logfile (default false to truncate) > ## > # @ChardevBackend: > @@ -3243,7 +3269,8 @@ > # > # Since: 1.4 (testdev since 2.2) > ## > -{ 'struct': 'ChardevDummy', 'data': { } } > +{ 'struct': 'ChardevDummy', 'data': { }, > + 'base': 'ChardevCommon' } Instead of changing ChardevDummy, you could have deleted this type and do= ne: > =20 > { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile', > 'serial' : 'ChardevHostdev', =2E.. 'pty': 'ChardevCommon', 'null': 'ChardevCommon', and so on. I don't know which is better. > +/* Not reporting errors from writing to logfile, as logs are > + * defined to be "best effort" only */ > +static void qemu_chr_fe_write_log(CharDriverState *s, > + const uint8_t *buf, size_t len) > +{ > + size_t done =3D 0; > + ssize_t ret; > + > + if (s->logfd < 0) { > + return; > + } > + > + while (done < len) { > + do { > + ret =3D write(s->logfd, buf + done, len - done); > + if (ret =3D=3D -1 && errno =3D=3D EAGAIN) { > + g_usleep(100); > + } Do we really want to be sleeping here? > + } while (ret =3D=3D -1 && errno =3D=3D EAGAIN); > + > + if (ret <=3D 0) { Why '<=3D'? Shouldn't this be '<'? > + return; > + } > + done +=3D ret; > + } > +} > + > =20 > + > +static int qemu_chr_open_common(CharDriverState *chr, > + ChardevBackend *backend, > + Error **errp) > +{ > + ChardevCommon *common =3D backend->u.data; Phooey. This conflicts with my pending patches to get rid of 'data': http://repo.or.cz/qemu/ericb.git/commitdiff/84aaab99 I mentioned above that you could do things like 'null':'ChardevCommon' instead of 'null':'ChardevDummy'; and we also know that qapi guarantees a layout where all base types occur at the front of the rest of the type. So you could write this as: ChardevCommon *common =3D backend->u.null; and things will work even when 'data' is gone. But maybe that argues more strongly that we should hoist the common members into the base fields of ChardevBackend struct, or even as separate parameters to the 'chardev-add' command (the two alternate qapi representations I described in my other message). > + > + if (common->has_logfile) { > + int flags =3D O_WRONLY | O_CREAT; > + if (!common->has_logappend || > + !common->logappend) { > + flags |=3D O_TRUNC; > + } Umm, don't you need to set O_APPEND when common->logappend is true? > + chr->logfd =3D qemu_open(common->logfile, flags, 0666); =2E.. > @@ -367,9 +432,16 @@ static CharDriverState *qemu_chr_open_null(const c= har *id, > CharDriverState *chr; > =20 > chr =3D qemu_chr_alloc(); > + if (qemu_chr_open_common(chr, backend, errp) < 0) { > + goto error; > + } The other thing we could do here is have qemu_chr_open_common() take a ChardevCommon* instead of a ChardevBackend*. Then each caller would do 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 > /* MUX driver for serial I/O splitting */ > @@ -673,6 +745,9 @@ static CharDriverState *qemu_chr_open_mux(const cha= r *id, > } > =20 > chr =3D qemu_chr_alloc(); > + if (qemu_chr_open_common(chr, backend, errp) < 0) { ChardevCommon *common =3D qapi_ChardevDummy_base(backend->u.mux); if (qemu_chr_open_common(chr, common, errp) { and so forth. That feels a bit more type-safe (and less reliant on qapi layout guarantees) than trying to rely on the backend->u.data that I'm trying to get rid of. > @@ -1046,12 +1125,16 @@ static void fd_chr_close(struct CharDriverState= *chr) > } > =20 > /* open a character device to a unix fd */ > -static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out) > +static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out, > + ChardevBackend *backend, Erro= r **errp) Might be better as ChardevCommon *common here as well. > +++ b/qemu-options.hx > @@ -2034,40 +2034,43 @@ The general form of a character device option i= s: > 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][,logappend=3D= on|off]\n" Do we want to allow logappend=3Don even when logfile is unspecified, or should we make that an error? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --jVJXCggV3xuLqUO1Ugi7gsrqkGm7ililk 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/ iQEcBAEBCAAGBQJWeZ9YAAoJEKeha0olJ0Nqh6kIAJUUeVpDmNJjAFfiuWTCpFn5 EfZxcmCWMS7MG4UTinucfl3y9qRTsD1wen16YLz3UgMGiHrVwYOYofqZGIZJp82e KEoEVg5eCEWtKcHF+nzSeo51vB23pKKBsNHjrUD852x8BUOwhD22k49QT9JcCSK5 MBsrRgDL63UfnHjXOlmRr77pYFEOPPDUOZ155QiIOT5izrrdNhy6IE9geodD+NK7 bD0kQiJ8bO5Wp+3uG2MKaVyl+H0yoB7IQGiQp3TO7dJTMO1ULCOaqiMFyn/hF4Wn ZrmMVgsgIo5MReZ8MCLlfEmuitMMs/Q5DTRuBNJ3memHSHd0EWvTeOkMMOWcDkg= =LK0L -----END PGP SIGNATURE----- --jVJXCggV3xuLqUO1Ugi7gsrqkGm7ililk--