Re: [Qemu-devel] [PATCH v2] qemu-char: add logfile facility to all chardev backends

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6819 bytes --]

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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]