[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: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v4] qemu-char: add logfile facility to all chardev backends |
Date: |
Fri, 15 Jan 2016 09:15:02 +0000 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Thu, Jan 14, 2016 at 09:05:03PM -0700, Eric Blake wrote:
> 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?
Not on Linux, but i think it would be a problem on Windows. This is
pre-existing in the chardev code in general - qemu_chr_free() doesn't
destroy the mutex.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|