From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bs8v8-00061w-Ph for qemu-devel@nongnu.org; Thu, 06 Oct 2016 09:41:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bs8v3-0004m0-VG for qemu-devel@nongnu.org; Thu, 06 Oct 2016 09:41:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47222) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bs8v3-0004lu-OQ for qemu-devel@nongnu.org; Thu, 06 Oct 2016 09:41:05 -0400 Date: Thu, 6 Oct 2016 14:41:00 +0100 From: "Daniel P. Berrange" Message-ID: <20161006134059.GI14680@redhat.com> Reply-To: "Daniel P. Berrange" References: <8828dfd6d813ab5e0b6c66a68e62b54ca2ada086.1475757380.git.mprivozn@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v2] qemu_opt_foreach: Fix crasher List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Michal Privoznik , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , QEMU Developers , Markus Armbruster On Thu, Oct 06, 2016 at 02:10:17PM +0100, Peter Maydell wrote: > On 6 October 2016 at 14:02, Peter Maydell wrote: > > On 6 October 2016 at 13:39, Michal Privoznik wrote: > >> diff --git a/include/sysemu/char.h b/include/sysemu/char.h > >> index 0d0465a..df58ef8 100644 > >> --- a/include/sysemu/char.h > >> +++ b/include/sysemu/char.h > >> @@ -93,6 +93,7 @@ struct CharDriverState { > >> int avail_connections; > >> int is_mux; > >> guint fd_in_tag; > >> + /* Be aware that in some cases @opts might be NULL. */ > >> QemuOpts *opts; > >> bool replay; > >> QTAILQ_ENTRY(CharDriverState) next; > > > > Wouldn't it be better to ensure that it can't be NULL regardless of > > how the object was created? > > FWIW, a quick check of who uses this field (by commenting it out > and looking for the compile errors) shows: > net/vhost-user.c > net/colo-compare.c (which also just does a foreach on it) > qemu-char.c (in qemu_chr_new_from_opts and qemu_chr_free_common) > > Alternative possible approach: if you can create a CharDriver > validly with no opts, then the net/ code shouldn't be looking > at opts at all (but should instead look at wherever the config > stuff goes for the 'opts is null' case, which we should make > sure is correct regardless of how the CharDriver was created). Agreed, that net/vhost-user.c code is just evil - it has no business poking around the chardev internals in that way. AFAICT, the only reason it is doing that is so that it can report an error if you try to connect the vhost-suer to a chardev that isn't socket backed. If it requires some particular feature of the chardev backend, then we should make a way to query for existance of that feature directly. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|