| Message ID | 1450808260-17922-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <berrange@redhat.com> > --- >
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] > 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. 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 <berrange@redhat.com> > --- > > +++ b/qapi-schema.json > @@ -3093,6 +3093,21 @@ > ## > { 'command': 'screendump', 'data': {'filename': 'str'} } > > + > +## > +# @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 done: > > { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile', > 'serial' : 'ChardevHostdev', ... '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 = 0; > + ssize_t ret; > + > + if (s->logfd < 0) { > + return; > + } > + > + while (done < len) { > + do { > + ret = write(s->logfd, buf + done, len - done); > + if (ret == -1 && errno == EAGAIN) { > + g_usleep(100); > + } Do we really want to be sleeping here? > + } while (ret == -1 && errno == EAGAIN); > + > + if (ret <= 0) { Why '<='? Shouldn't this be '<'? > + return; > + } > + done += ret; > + } > +} > + > > + > +static int qemu_chr_open_common(CharDriverState *chr, > + ChardevBackend *backend, > + Error **errp) > +{ > + ChardevCommon *common = 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 = 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 = O_WRONLY | O_CREAT; > + if (!common->has_logappend || > + !common->logappend) { > + flags |= O_TRUNC; > + } Umm, don't you need to set O_APPEND when common->logappend is true? > + chr->logfd = qemu_open(common->logfile, flags, 0666); ... > @@ -367,9 +432,16 @@ static CharDriverState *qemu_chr_open_null(const char *id, > CharDriverState *chr; > > chr = 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 = qapi_ChardevDummy_base(backend->u.null); if (qemu_chr_open_common(chr, common, errp) { > > /* MUX driver for serial I/O splitting */ > @@ -673,6 +745,9 @@ static CharDriverState *qemu_chr_open_mux(const char *id, > } > > chr = qemu_chr_alloc(); > + if (qemu_chr_open_common(chr, backend, errp) < 0) { ChardevCommon *common = 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) > } > > /* 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, Error **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 is: > ETEXI > > DEF("chardev", HAS_ARG, QEMU_OPTION_chardev, > - "-chardev null,id=id[,mux=on|off]\n" > + "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" Do we want to allow logappend=on even when logfile is unspecified, or should we make that an error?
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] > > > > > 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" } } } } 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. > 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. Ok, so that's still backwards compatible, but the 'logfile' is appearing in an expected place IMHO. > 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). Yeah, that would require a bit of refactoring, since many of the codepaths I'm changing only get passed in the 'ChardevBackend' struct, not its parent owner. > 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. 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 ChardevCommon 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. So from that POV, I'd be against, pushing the 'logfile' field up either 1 or 2 levels. Regards, Daniel
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] > > > 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. 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 <berrange@redhat.com> > > --- > > > > > +++ b/qapi-schema.json > > @@ -3093,6 +3093,21 @@ > > ## > > { 'command': 'screendump', 'data': {'filename': 'str'} } > > > > + > > +## > > +# @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 done: > > > > > { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile', > > 'serial' : 'ChardevHostdev', > ... > 'pty': 'ChardevCommon', > 'null': 'ChardevCommon', > > and so on. I don't know which is better. Yep, me neither. Given the name it felt like some kind of placeholder for future work, so I left it alone. > > +/* 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 = 0; > > + ssize_t ret; > > + > > + if (s->logfd < 0) { > > + return; > > + } > > + > > + while (done < len) { > > + do { > > + ret = write(s->logfd, buf + done, len - done); > > + if (ret == -1 && errno == EAGAIN) { > > + g_usleep(100); > > + } > > Do we really want to be sleeping here? If logfile points to a plain file, you'll never get EAGAIN. For that matter even if it doesn't point to a plain file I realize that we've not called qemu_nonblock() on logfd, so it'll never get EAGAIN for that reason either. The qemu_chr_fe_write_all() currently has the same usleep which is what I followed. The qemu_chr_fe_write() just returns on EAGAIN. In the write_log() method we want to try to write all the data the qemu_chr_fe_write/write_all just sent. If we ignore EGAIN, we'll potentially loose data from the log, but if we honour it, we have to sleep a little or not set O_NONBLOCK. > > > + } while (ret == -1 && errno == EAGAIN); > > + > > + if (ret <= 0) { > > Why '<='? Shouldn't this be '<'? Well there's no difference really since write() will never return 0. > > > + return; > > + } > > + done += ret; > > + } > > +} > > + > > > > > + > > +static int qemu_chr_open_common(CharDriverState *chr, > > + ChardevBackend *backend, > > + Error **errp) > > +{ > > + ChardevCommon *common = 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 = 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 = O_WRONLY | O_CREAT; > > + if (!common->has_logappend || > > + !common->logappend) { > > + flags |= O_TRUNC; > > + } > > Umm, don't you need to set O_APPEND when common->logappend is true? > > > + chr->logfd = qemu_open(common->logfile, flags, 0666); > ... > > @@ -367,9 +432,16 @@ static CharDriverState *qemu_chr_open_null(const char *id, > > CharDriverState *chr; > > > > chr = 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 = qapi_ChardevDummy_base(backend->u.null); > if (qemu_chr_open_common(chr, common, errp) { Yep, I think this is the easiest thing todo, to avoid use of backend->u.data. > 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. Agreed > > +++ b/qemu-options.hx > > @@ -2034,40 +2034,43 @@ The general form of a character device option is: > > ETEXI > > > > DEF("chardev", HAS_ARG, QEMU_OPTION_chardev, > > - "-chardev null,id=id[,mux=on|off]\n" > > + "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" > > Do we want to allow logappend=on even when logfile is unspecified, or > should we make that an error? I figured it was harmless to just ignore it. Regards, Daniel
On 22/12/2015 19:17, Daniel P. Berrange wrote: > + if (common->has_logfile) { > + int flags = O_WRONLY | O_CREAT; > + if (!common->has_logappend || > + !common->logappend) { > + flags |= O_TRUNC; > + } Should it use O_APPEND if logappend is absent or true, or perhaps always? I can take care of the change myself. You are also missing a few qemu_chr_alloc calls outside qemu-char.c, which makes me wonder if it's better to add the new logic in qemu_chr_alloc itself. Paolo > + chr->logfd = qemu_open(common->logfile, flags, 0666); > + if (chr->logfd < 0) { > + error_setg_errno(errp, errno, > + "Unable to open logfile %s", > + common->logfile); > + return -1; > + } > + } > + > + return 0; > +}
On Wed, Dec 23, 2015 at 12:34:25PM +0100, Paolo Bonzini wrote: > > > On 22/12/2015 19:17, Daniel P. Berrange wrote: > > + if (common->has_logfile) { > > + int flags = O_WRONLY | O_CREAT; > > + if (!common->has_logappend || > > + !common->logappend) { > > + flags |= O_TRUNC; > > + } > > Should it use O_APPEND if logappend is absent or true, or perhaps > always? I can take care of the change myself. Yes it should use O_APPEND in the other branch of the if, that's a bug eric pointed out too. Basically same logic as the recently added 'append' flag in qmp_chardev_open_file > You are also missing a few qemu_chr_alloc calls outside qemu-char.c, > which makes me wonder if it's better to add the new logic in > qemu_chr_alloc itself. Oh, yes, perhaps I should just pass ChardevCommon straight into qemu_chr_alloc(). I'll look at that when doing the other changes Eric suggested wrt to removing use of backend.u.data Regards, Daniel
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" } } } } > > 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. > >> 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]): >> >> >> 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 in >> qapi. > >>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 ChardevCommon > 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*). > > So from that POV, I'd be against, pushing the 'logfile' field up > either 1 or 2 levels. > > Regards, > Daniel >
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: >> >>> >>> { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile', >>> 'serial' : 'ChardevHostdev', >> ... >> 'pty': 'ChardevCommon', >> 'null': 'ChardevCommon', >> >> and so on. I don't know which is better. > > 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 do >> an appropriate upcast before calling the common code: >> >> ChardevCommon *common = qapi_ChardevDummy_base(backend->u.null); >> if (qemu_chr_open_common(chr, common, errp) { > > Yep, I think this is the easiest thing todo, to avoid use of > backend->u.data. > >> 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. > > Agreed > 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 >>> >>> DEF("chardev", HAS_ARG, QEMU_OPTION_chardev, >>> - "-chardev null,id=id[,mux=on|off]\n" >>> + "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" >> >> Do we want to allow logappend=on even when logfile is unspecified, or >> should we make that an error? > > I figured it was harmless to just ignore it. Works for me.
diff --git a/include/sysemu/char.h b/include/sysemu/char.h index aff193f..5681785 100644 --- a/include/sysemu/char.h +++ b/include/sysemu/char.h @@ -77,6 +77,7 @@ struct CharDriverState { void *opaque; char *label; char *filename; + int logfd; int be_open; int fe_open; int explicit_fe_open; diff --git a/qapi-schema.json b/qapi-schema.json index 2e31733..dabb5ce 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3093,6 +3093,21 @@ ## { 'command': 'screendump', 'data': {'filename': 'str'} } + +## +# @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) +# +# Since: 2.6 +## +{ 'struct': 'ChardevCommon', 'data': { '*logfile': 'str', + '*logappend': 'bool' } } + ## # @ChardevFile: # @@ -3107,7 +3122,8 @@ ## { 'struct': 'ChardevFile', 'data': { '*in' : 'str', 'out' : 'str', - '*append': 'bool' } } + '*append': 'bool' }, + 'base': 'ChardevCommon' } ## # @ChardevHostdev: @@ -3120,7 +3136,8 @@ # # Since: 1.4 ## -{ 'struct': 'ChardevHostdev', 'data': { 'device' : 'str' } } +{ 'struct': 'ChardevHostdev', 'data': { 'device' : 'str' }, + 'base': 'ChardevCommon' } ## # @ChardevSocket: @@ -3147,7 +3164,8 @@ '*wait' : 'bool', '*nodelay' : 'bool', '*telnet' : 'bool', - '*reconnect' : 'int' } } + '*reconnect' : 'int' }, + 'base': 'ChardevCommon' } ## # @ChardevUdp: @@ -3160,7 +3178,8 @@ # Since: 1.5 ## { 'struct': 'ChardevUdp', 'data': { 'remote' : 'SocketAddress', - '*local' : 'SocketAddress' } } + '*local' : 'SocketAddress' }, + 'base': 'ChardevCommon' } ## # @ChardevMux: @@ -3171,7 +3190,8 @@ # # Since: 1.5 ## -{ 'struct': 'ChardevMux', 'data': { 'chardev' : 'str' } } +{ 'struct': 'ChardevMux', 'data': { 'chardev' : 'str' }, + 'base': 'ChardevCommon' } ## # @ChardevStdio: @@ -3184,7 +3204,9 @@ # # Since: 1.5 ## -{ 'struct': 'ChardevStdio', 'data': { '*signal' : 'bool' } } +{ 'struct': 'ChardevStdio', 'data': { '*signal' : 'bool' }, + 'base': 'ChardevCommon' } + ## # @ChardevSpiceChannel: @@ -3195,7 +3217,8 @@ # # Since: 1.5 ## -{ 'struct': 'ChardevSpiceChannel', 'data': { 'type' : 'str' } } +{ 'struct': 'ChardevSpiceChannel', 'data': { 'type' : 'str' }, + 'base': 'ChardevCommon' } ## # @ChardevSpicePort: @@ -3206,7 +3229,8 @@ # # Since: 1.5 ## -{ 'struct': 'ChardevSpicePort', 'data': { 'fqdn' : 'str' } } +{ 'struct': 'ChardevSpicePort', 'data': { 'fqdn' : 'str' }, + 'base': 'ChardevCommon' } ## # @ChardevVC: @@ -3223,7 +3247,8 @@ { 'struct': 'ChardevVC', 'data': { '*width' : 'int', '*height' : 'int', '*cols' : 'int', - '*rows' : 'int' } } + '*rows' : 'int' }, + 'base': 'ChardevCommon' } ## # @ChardevRingbuf: @@ -3234,7 +3259,8 @@ # # Since: 1.5 ## -{ 'struct': 'ChardevRingbuf', 'data': { '*size' : 'int' } } +{ 'struct': 'ChardevRingbuf', 'data': { '*size' : 'int' }, + 'base': 'ChardevCommon' } ## # @ChardevBackend: @@ -3243,7 +3269,8 @@ # # Since: 1.4 (testdev since 2.2) ## -{ 'struct': 'ChardevDummy', 'data': { } } +{ 'struct': 'ChardevDummy', 'data': { }, + 'base': 'ChardevCommon' } { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile', 'serial' : 'ChardevHostdev', diff --git a/qemu-char.c b/qemu-char.c index 00a7526..23f0b7a 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -159,10 +159,13 @@ static int sockaddr_to_str(char *dest, int max_len, static QTAILQ_HEAD(CharDriverStateHead, CharDriverState) chardevs = QTAILQ_HEAD_INITIALIZER(chardevs); +static void qemu_chr_free_common(CharDriverState *chr); + CharDriverState *qemu_chr_alloc(void) { CharDriverState *chr = g_malloc0(sizeof(CharDriverState)); qemu_mutex_init(&chr->chr_write_lock); + chr->logfd = -1; return chr; } @@ -188,12 +191,45 @@ void qemu_chr_be_generic_open(CharDriverState *s) qemu_chr_be_event(s, CHR_EVENT_OPENED); } + +/* 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 = 0; + ssize_t ret; + + if (s->logfd < 0) { + return; + } + + while (done < len) { + do { + ret = write(s->logfd, buf + done, len - done); + if (ret == -1 && errno == EAGAIN) { + g_usleep(100); + } + } while (ret == -1 && errno == EAGAIN); + + if (ret <= 0) { + return; + } + done += ret; + } +} + int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len) { int ret; qemu_mutex_lock(&s->chr_write_lock); ret = s->chr_write(s, buf, len); + + if (ret > 0) { + qemu_chr_fe_write_log(s, buf, ret); + } + qemu_mutex_unlock(&s->chr_write_lock); return ret; } @@ -218,6 +254,10 @@ int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len) offset += res; } + if (offset > 0) { + qemu_chr_fe_write_log(s, buf, offset); + } + qemu_mutex_unlock(&s->chr_write_lock); if (res < 0) { @@ -359,6 +399,31 @@ static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len) return len; } + +static int qemu_chr_open_common(CharDriverState *chr, + ChardevBackend *backend, + Error **errp) +{ + ChardevCommon *common = backend->u.data; + + if (common->has_logfile) { + int flags = O_WRONLY | O_CREAT; + if (!common->has_logappend || + !common->logappend) { + flags |= O_TRUNC; + } + chr->logfd = qemu_open(common->logfile, flags, 0666); + if (chr->logfd < 0) { + error_setg_errno(errp, errno, + "Unable to open logfile %s", + common->logfile); + return -1; + } + } + + return 0; +} + static CharDriverState *qemu_chr_open_null(const char *id, ChardevBackend *backend, ChardevReturn *ret, @@ -367,9 +432,16 @@ static CharDriverState *qemu_chr_open_null(const char *id, CharDriverState *chr; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } chr->chr_write = null_chr_write; chr->explicit_be_open = true; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } /* MUX driver for serial I/O splitting */ @@ -673,6 +745,9 @@ static CharDriverState *qemu_chr_open_mux(const char *id, } chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } d = g_new0(MuxDriver, 1); chr->opaque = d; @@ -693,6 +768,10 @@ static CharDriverState *qemu_chr_open_mux(const char *id, chr->is_mux = 1; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } @@ -1046,12 +1125,16 @@ static void fd_chr_close(struct CharDriverState *chr) } /* 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, Error **errp) { CharDriverState *chr; FDCharDriver *s; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } s = g_new0(FDCharDriver, 1); s->fd_in = io_channel_from_fd(fd_in); s->fd_out = io_channel_from_fd(fd_out); @@ -1064,6 +1147,10 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out) chr->chr_close = fd_chr_close; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } static CharDriverState *qemu_chr_open_pipe(const char *id, @@ -1092,7 +1179,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id, return NULL; } } - return qemu_chr_open_fd(fd_in, fd_out); + return qemu_chr_open_fd(fd_in, fd_out, backend, errp); } /* init terminal so that we can grab keys */ @@ -1173,7 +1260,7 @@ static CharDriverState *qemu_chr_open_stdio(const char *id, act.sa_handler = term_stdio_handler; sigaction(SIGCONT, &act, NULL); - chr = qemu_chr_open_fd(0, 1); + chr = qemu_chr_open_fd(0, 1, backend, errp); chr->chr_close = qemu_chr_close_stdio; chr->chr_set_echo = qemu_chr_set_echo_stdio; if (opts->has_signal) { @@ -1406,6 +1493,9 @@ static CharDriverState *qemu_chr_open_pty(const char *id, qemu_set_nonblock(master_fd); chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } chr->filename = g_strdup_printf("pty:%s", pty_name); ret->pty = g_strdup(pty_name); @@ -1426,6 +1516,10 @@ static CharDriverState *qemu_chr_open_pty(const char *id, s->timer_tag = 0; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } static void tty_serial_init(int fd, int speed, @@ -1628,12 +1722,14 @@ static void qemu_chr_close_tty(CharDriverState *chr) } } -static CharDriverState *qemu_chr_open_tty_fd(int fd) +static CharDriverState *qemu_chr_open_tty_fd(int fd, + ChardevBackend *backend, + Error **errp) { CharDriverState *chr; tty_serial_init(fd, 115200, 'N', 8, 1); - chr = qemu_chr_open_fd(fd, fd); + chr = qemu_chr_open_fd(fd, fd, backend, errp); chr->chr_ioctl = tty_serial_ioctl; chr->chr_close = qemu_chr_close_tty; return chr; @@ -1753,7 +1849,9 @@ static void pp_close(CharDriverState *chr) qemu_chr_be_event(chr, CHR_EVENT_CLOSED); } -static CharDriverState *qemu_chr_open_pp_fd(int fd, Error **errp) +static CharDriverState *qemu_chr_open_pp_fd(int fd, + ChardevBackend *backend, + Error **errp) { CharDriverState *chr; ParallelCharDriver *drv; @@ -1769,12 +1867,19 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd, Error **errp) drv->mode = IEEE1284_MODE_COMPAT; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } chr->chr_write = null_chr_write; chr->chr_ioctl = pp_ioctl; chr->chr_close = pp_close; chr->opaque = drv; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } #endif /* __linux__ */ @@ -1819,16 +1924,25 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg) return 0; } -static CharDriverState *qemu_chr_open_pp_fd(int fd, Error **errp) +static CharDriverState *qemu_chr_open_pp_fd(int fd, + ChardevBackend *backend, + Error **errp) { CharDriverState *chr; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } chr->opaque = (void *)(intptr_t)fd; chr->chr_write = null_chr_write; chr->chr_ioctl = pp_ioctl; chr->explicit_be_open = true; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } #endif @@ -2049,12 +2163,16 @@ static int win_chr_poll(void *opaque) } static CharDriverState *qemu_chr_open_win_path(const char *filename, + ChardevBackend *backend, Error **errp) { CharDriverState *chr; WinCharState *s; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } s = g_new0(WinCharState, 1); chr->opaque = s; chr->chr_write = win_chr_write; @@ -2062,10 +2180,13 @@ static CharDriverState *qemu_chr_open_win_path(const char *filename, if (win_chr_init(chr, filename, errp) < 0) { g_free(s); - g_free(chr); - return NULL; + goto error; } return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } static int win_chr_pipe_poll(void *opaque) @@ -2159,6 +2280,9 @@ static CharDriverState *qemu_chr_open_pipe(const char *id, WinCharState *s; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } s = g_new0(WinCharState, 1); chr->opaque = s; chr->chr_write = win_chr_write; @@ -2166,23 +2290,35 @@ static CharDriverState *qemu_chr_open_pipe(const char *id, if (win_chr_pipe_init(chr, filename, errp) < 0) { g_free(s); - g_free(chr); - return NULL; + goto error; } return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } -static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out) +static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out, + ChardevBackend *backend, + Error **errp) { CharDriverState *chr; WinCharState *s; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } s = g_new0(WinCharState, 1); s->hcom = fd_out; chr->opaque = s; chr->chr_write = win_chr_write; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } static CharDriverState *qemu_chr_open_win_con(const char *id, @@ -2190,7 +2326,8 @@ static CharDriverState *qemu_chr_open_win_con(const char *id, ChardevReturn *ret, Error **errp) { - return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE)); + return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), + backend, errp); } static int win_stdio_write(CharDriverState *chr, const uint8_t *buf, int len) @@ -2340,6 +2477,9 @@ static CharDriverState *qemu_chr_open_stdio(const char *id, int is_console = 0; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto err0; + } stdio = g_new0(WinStdioCharState, 1); stdio->hStdIn = GetStdHandle(STD_INPUT_HANDLE); @@ -2406,6 +2546,8 @@ err2: CloseHandle(stdio->hInputDoneEvent); err1: qemu_del_wait_object(stdio->hStdIn, NULL, NULL); +err0: + qemu_chr_free_common(chr); return NULL; } #endif /* !_WIN32 */ @@ -2511,12 +2653,17 @@ static void udp_chr_close(CharDriverState *chr) qemu_chr_be_event(chr, CHR_EVENT_CLOSED); } -static CharDriverState *qemu_chr_open_udp_fd(int fd) +static CharDriverState *qemu_chr_open_udp_fd(int fd, + ChardevBackend *backend, + Error **errp) { CharDriverState *chr = NULL; NetCharDriver *s = NULL; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } s = g_new0(NetCharDriver, 1); s->fd = fd; @@ -2530,6 +2677,10 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd) /* be isn't opened until we get a connection */ chr->explicit_be_open = true; return chr; + + error: + qemu_chr_free_common(chr); + return NULL; } /***********************************************************/ @@ -2927,7 +3078,7 @@ static int tcp_chr_sync_read(CharDriverState *chr, const uint8_t *buf, int len) #ifndef _WIN32 CharDriverState *qemu_chr_open_eventfd(int eventfd) { - CharDriverState *chr = qemu_chr_open_fd(eventfd, eventfd); + CharDriverState *chr = qemu_chr_open_fd(eventfd, eventfd, NULL, NULL); if (chr) { chr->avail_connections = 1; @@ -3210,9 +3361,12 @@ static CharDriverState *qemu_chr_open_ringbuf(const char *id, { ChardevRingbuf *opts = backend->u.ringbuf; CharDriverState *chr; - RingBufCharDriver *d; + RingBufCharDriver *d = NULL; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } d = g_malloc(sizeof(*d)); d->size = opts->has_size ? opts->size : 65536; @@ -3220,7 +3374,7 @@ static CharDriverState *qemu_chr_open_ringbuf(const char *id, /* The size must be power of 2 */ if (d->size & (d->size - 1)) { error_setg(errp, "size of ringbuf chardev must be power of two"); - goto fail; + goto error; } d->prod = 0; @@ -3233,9 +3387,9 @@ static CharDriverState *qemu_chr_open_ringbuf(const char *id, return chr; -fail: +error: g_free(d); - g_free(chr); + qemu_chr_free_common(chr); return NULL; } @@ -3479,6 +3633,19 @@ fail: return NULL; } +static void qemu_chr_parse_common(QemuOpts *opts, ChardevBackend *backend) +{ + ChardevCommon *common = backend->u.data; + const char *logfile = qemu_opt_get(opts, "logfile"); + + common->has_logfile = logfile != NULL; + common->logfile = logfile ? g_strdup(logfile) : NULL; + + common->has_logappend = true; + common->logappend = qemu_opt_get_bool(opts, "logappend", false); +} + + static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend, Error **errp) { @@ -3489,6 +3656,7 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend, return; } backend->u.file = g_new0(ChardevFile, 1); + qemu_chr_parse_common(opts, backend); backend->u.file->out = g_strdup(path); backend->u.file->has_append = true; @@ -3499,6 +3667,7 @@ static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend, Error **errp) { backend->u.stdio = g_new0(ChardevStdio, 1); + qemu_chr_parse_common(opts, backend); backend->u.stdio->has_signal = true; backend->u.stdio->signal = qemu_opt_get_bool(opts, "signal", true); } @@ -3514,6 +3683,7 @@ static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend, return; } backend->u.serial = g_new0(ChardevHostdev, 1); + qemu_chr_parse_common(opts, backend); backend->u.serial->device = g_strdup(device); } #endif @@ -3529,6 +3699,7 @@ static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend, return; } backend->u.parallel = g_new0(ChardevHostdev, 1); + qemu_chr_parse_common(opts, backend); backend->u.parallel->device = g_strdup(device); } #endif @@ -3543,6 +3714,7 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend, return; } backend->u.pipe = g_new0(ChardevHostdev, 1); + qemu_chr_parse_common(opts, backend); backend->u.pipe->device = g_strdup(device); } @@ -3552,6 +3724,7 @@ static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend, int val; backend->u.ringbuf = g_new0(ChardevRingbuf, 1); + qemu_chr_parse_common(opts, backend); val = qemu_opt_get_size(opts, "size", 0); if (val != 0) { @@ -3570,6 +3743,7 @@ static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend, return; } backend->u.mux = g_new0(ChardevMux, 1); + qemu_chr_parse_common(opts, backend); backend->u.mux->chardev = g_strdup(chardev); } @@ -3598,6 +3772,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, } backend->u.socket = g_new0(ChardevSocket, 1); + qemu_chr_parse_common(opts, backend); backend->u.socket->has_nodelay = true; backend->u.socket->nodelay = do_nodelay; @@ -3659,6 +3834,7 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend, } backend->u.udp = g_new0(ChardevUdp, 1); + qemu_chr_parse_common(opts, backend); addr = g_new0(SocketAddress, 1); addr->type = SOCKET_ADDRESS_KIND_INET; @@ -3890,17 +4066,25 @@ void qemu_chr_fe_release(CharDriverState *s) s->avail_connections++; } -void qemu_chr_free(CharDriverState *chr) +static void qemu_chr_free_common(CharDriverState *chr) { - if (chr->chr_close) { - chr->chr_close(chr); - } g_free(chr->filename); g_free(chr->label); qemu_opts_del(chr->opts); + if (chr->logfd != -1) { + close(chr->logfd); + } g_free(chr); } +void qemu_chr_free(CharDriverState *chr) +{ + if (chr->chr_close) { + chr->chr_close(chr); + } + qemu_chr_free_common(chr); +} + void qemu_chr_delete(CharDriverState *chr) { QTAILQ_REMOVE(&chardevs, chr, next); @@ -4053,6 +4237,12 @@ QemuOptsList qemu_chardev_opts = { },{ .name = "append", .type = QEMU_OPT_BOOL, + },{ + .name = "logfile", + .type = QEMU_OPT_STRING, + },{ + .name = "logappend", + .type = QEMU_OPT_BOOL, }, { /* end of list */ } }, @@ -4079,7 +4269,7 @@ static CharDriverState *qmp_chardev_open_file(const char *id, error_setg(errp, "open %s failed", file->out); return NULL; } - return qemu_chr_open_win_file(out); + return qemu_chr_open_win_file(out, backend, errp); } static CharDriverState *qmp_chardev_open_serial(const char *id, @@ -4088,7 +4278,7 @@ static CharDriverState *qmp_chardev_open_serial(const char *id, Error **errp) { ChardevHostdev *serial = backend->u.serial; - return qemu_chr_open_win_path(serial->device, errp); + return qemu_chr_open_win_path(serial->device, backend, errp); } #else /* WIN32 */ @@ -4134,7 +4324,7 @@ static CharDriverState *qmp_chardev_open_file(const char *id, } } - return qemu_chr_open_fd(in, out); + return qemu_chr_open_fd(in, out, backend, errp); } #ifdef HAVE_CHARDEV_SERIAL @@ -4151,7 +4341,7 @@ static CharDriverState *qmp_chardev_open_serial(const char *id, return NULL; } qemu_set_nonblock(fd); - return qemu_chr_open_tty_fd(fd); + return qemu_chr_open_tty_fd(fd, backend, errp); } #endif @@ -4168,7 +4358,7 @@ static CharDriverState *qmp_chardev_open_parallel(const char *id, if (fd < 0) { return NULL; } - return qemu_chr_open_pp_fd(fd, errp); + return qemu_chr_open_pp_fd(fd, backend, errp); } #endif @@ -4205,7 +4395,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, Error **errp) { CharDriverState *chr; - TCPCharDriver *s; + TCPCharDriver *s = NULL; ChardevSocket *sock = backend->u.socket; SocketAddress *addr = sock->addr; bool do_nodelay = sock->has_nodelay ? sock->nodelay : false; @@ -4215,6 +4405,9 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0; chr = qemu_chr_alloc(); + if (qemu_chr_open_common(chr, backend, errp) < 0) { + goto error; + } s = g_new0(TCPCharDriver, 1); s->fd = -1; @@ -4252,10 +4445,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, if (s->reconnect_time) { socket_try_connect(chr); } else if (!qemu_chr_open_socket_fd(chr, errp)) { - g_free(s); - g_free(chr->filename); - g_free(chr); - return NULL; + goto error; } if (is_listen && is_waitconnect) { @@ -4266,6 +4456,11 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, } return chr; + + error: + g_free(s); + qemu_chr_free_common(chr); + return NULL; } static CharDriverState *qmp_chardev_open_udp(const char *id, @@ -4280,7 +4475,7 @@ static CharDriverState *qmp_chardev_open_udp(const char *id, if (fd < 0) { return NULL; } - return qemu_chr_open_udp_fd(fd); + return qemu_chr_open_udp_fd(fd, backend, errp); } ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, diff --git a/qemu-options.hx b/qemu-options.hx index 49afe6c..4370413 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2034,40 +2034,43 @@ The general form of a character device option is: ETEXI DEF("chardev", HAS_ARG, QEMU_OPTION_chardev, - "-chardev null,id=id[,mux=on|off]\n" + "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" "-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4][,ipv6][,nodelay][,reconnect=seconds]\n" - " [,server][,nowait][,telnet][,reconnect=seconds][,mux=on|off] (tcp)\n" - "-chardev socket,id=id,path=path[,server][,nowait][,telnet][,reconnect=seconds][,mux=on|off] (unix)\n" + " [,server][,nowait][,telnet][,reconnect=seconds][,mux=on|off]\n" + " [,logfile=PATH][,logappend=on|off] (tcp)\n" + "-chardev socket,id=id,path=path[,server][,nowait][,telnet][,reconnect=seconds]\n" + " [,mux=on|off][,logfile=PATH][,logappend=on|off] (unix)\n" "-chardev udp,id=id[,host=host],port=port[,localaddr=localaddr]\n" " [,localport=localport][,ipv4][,ipv6][,mux=on|off]\n" - "-chardev msmouse,id=id[,mux=on|off]\n" + " [,logfile=PATH][,logappend=on|off]\n" + "-chardev msmouse,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" "-chardev vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]\n" - " [,mux=on|off]\n" - "-chardev ringbuf,id=id[,size=size]\n" - "-chardev file,id=id,path=path[,mux=on|off]\n" - "-chardev pipe,id=id,path=path[,mux=on|off]\n" + " [,mux=on|off][,logfile=PATH][,logappend=on|off]\n" + "-chardev ringbuf,id=id[,size=size][,logfile=PATH][,logappend=on|off]\n" + "-chardev file,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" + "-chardev pipe,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" #ifdef _WIN32 - "-chardev console,id=id[,mux=on|off]\n" - "-chardev serial,id=id,path=path[,mux=on|off]\n" + "-chardev console,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" + "-chardev serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" #else - "-chardev pty,id=id[,mux=on|off]\n" - "-chardev stdio,id=id[,mux=on|off][,signal=on|off]\n" + "-chardev pty,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" + "-chardev stdio,id=id[,mux=on|off][,signal=on|off][,logfile=PATH][,logappend=on|off]\n" #endif #ifdef CONFIG_BRLAPI - "-chardev braille,id=id[,mux=on|off]\n" + "-chardev braille,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" #endif #if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) - "-chardev serial,id=id,path=path[,mux=on|off]\n" - "-chardev tty,id=id,path=path[,mux=on|off]\n" + "-chardev serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" + "-chardev tty,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" #endif #if defined(__linux__) || defined(__FreeBSD__) || defined(__DragonFly__) - "-chardev parallel,id=id,path=path[,mux=on|off]\n" - "-chardev parport,id=id,path=path[,mux=on|off]\n" + "-chardev parallel,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" + "-chardev parport,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n" #endif #if defined(CONFIG_SPICE) - "-chardev spicevmc,id=id,name=name[,debug=debug]\n" - "-chardev spiceport,id=id,name=name[,debug=debug]\n" + "-chardev spicevmc,id=id,name=name[,debug=debug][,logfile=PATH][,logappend=on|off]\n" + "-chardev spiceport,id=id,name=name[,debug=debug][,logfile=PATH][,logappend=on|off]\n" #endif , QEMU_ARCH_ALL )
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. 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. 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 <berrange@redhat.com> --- Changed in v2: - Push logfile logic out of backend into common code so it is available for all backend types - Add docs to qemu -help text include/sysemu/char.h | 1 + qapi-schema.json | 49 +++++++--- qemu-char.c | 263 +++++++++++++++++++++++++++++++++++++++++++------- qemu-options.hx | 41 ++++---- 4 files changed, 290 insertions(+), 64 deletions(-)