Re: [Qemu-devel] [PATCH v2] qemu_opt_foreach: Fix crasher

[Daniel P. Berrange]

On Thu, Oct 06, 2016 at 02:10:17PM +0100, Peter Maydell wrote:
> On 6 October 2016 at 14:02, Peter Maydell <address@hidden> wrote:
> > On 6 October 2016 at 13:39, Michal Privoznik <address@hidden> 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/ :|