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

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

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

Seconded.