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

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 16 August 2016 at 16:17, Michal Privoznik <mprivozn@redhat.com> wrote:
>> The solution is to teach qemu_opt_foreach() to take a shortcut if
>> @opts is NULL.

Please provide a reproducer.  A stack backtrace wouldn't hurt.

>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> Even after this patch I'm still unable to attach vhost-user:
>>
>> {"id": "libvirt-20", "error": {"class": "GenericError", "desc": "chardev \"charnet2\" is not a unix socket"}}
>>
>> But at least, qemu does not crash anymore.
>>
>>  util/qemu-option.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index 3467dc2..78be7e1 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>> @@ -614,6 +614,11 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>>      QemuOpt *opt;
>>      int rc;
>>
>> +    if (!opts) {
>> +        /* Done, trivially. */
>> +        return 0;
>> +    }
>> +
>>      QTAILQ_FOREACH(opt, &opts->head, next) {
>>          rc = func(opaque, opt->name, opt->str, errp);
>>          if (rc) {
>> --
>> 2.8.4
>
> This seems plausible, but I don't understand our option
> code very well, and we seem to have a mix of "check for
> NULL" and "caller had better not pass NULL" in the various
> functions in util/qemu-option.c.
>
> Markus: how is this supposed to work?

I wouldn't say this is "supposed to work" in some specific way.
"Happens to work" would be closer to the truth.

If you want me to interpret some sense into the mess after the fact,
here's my best guess: we generally require non-null opts, except for
qemu_opts_del() and the qemu_opt_get_FOO().

Makes obvious sense for qemu_opts_del(), since when a failing
constructor returns null, the destructor should accept null.

The qemu_opt_get_FOO() feel like a (possibly misguided) attempt at
convenience to me.

> In any case something is clearly still busted in the
> vhost-user code, because it's expecting to get a non-NULL
> opts so it can properly parse the chardev, so that seems
> like the thing we really need to fix.

Probably.  If I had a reproducer or at least a stack backtrace, I'd even
know where to look.