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

["Daniel P. Berrange" <berrange@redhat.com>]

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

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