From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aK0TR-0003Kl-BV for qemu-devel@nongnu.org; Fri, 15 Jan 2016 04:15:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aK0TO-0002Pd-4M for qemu-devel@nongnu.org; Fri, 15 Jan 2016 04:15:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34663) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aK0TN-0002PW-VC for qemu-devel@nongnu.org; Fri, 15 Jan 2016 04:15:10 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 0F3718F22C for ; Fri, 15 Jan 2016 09:15:08 +0000 (UTC) Date: Fri, 15 Jan 2016 09:15:02 +0000 From: "Daniel P. Berrange" Message-ID: <20160115091502.GA2863@redhat.com> References: <1452516281-27519-1-git-send-email-berrange@redhat.com> <56986FEF.1040307@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56986FEF.1040307@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4] qemu-char: add logfile facility to all chardev backends Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Paolo Bonzini , qemu-devel@nongnu.org, Markus Armbruster 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 > > --- > > > > > > -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 :|