[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4] qemu-char: add logfile facility to all chard
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4] qemu-char: add logfile facility to all chardev backends |
Date: |
Thu, 14 Jan 2016 21:05:03 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
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 <address@hidden>
> ---
>
> -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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature