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

[Eric Blake <eblake@redhat.com>]

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

On 01/11/2016 05:44 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.
> 

> 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>
> ---
> 


> -CharDriverState *qemu_chr_alloc(void)
> +static void qemu_chr_free_common(CharDriverState *chr);
> +
> +CharDriverState *qemu_chr_alloc(ChardevCommon *backend, Error **errp)
>  {
>      CharDriverState *chr = g_malloc0(sizeof(CharDriverState));
>      qemu_mutex_init(&chr->chr_write_lock);
> +
> +    if (backend->has_logfile) {
> +        int flags = O_WRONLY | O_CREAT;
> +        if (backend->has_logappend &&
> +            backend->logappend) {
> +            flags |= O_APPEND;
> +        } else {
> +            flags |= O_TRUNC;
> +        }
> +        chr->logfd = qemu_open(backend->logfile, flags, 0666);
> +        if (chr->logfd < 0) {
> +            error_setg_errno(errp, errno,
> +                             "Unable to open logfile %s",
> +                             backend->logfile);
> +            g_free(chr);

Are we leaking anything mutex-related by freeing chr without tearing
down the just-initialized chr_write_lock?


>  
> +static void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend)
> +{
> +    const char *logfile = qemu_opt_get(opts, "logfile");
> +
> +    backend->has_logfile = logfile != NULL;
> +    backend->logfile = logfile ? g_strdup(logfile) : NULL;

Isn't g_strdup(NULL) safe, such that you could write this as:

backend->logfile = g_strdup(logfile);

Otherwise looked okay; with the mutex question answered or fixed, you
can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]