Re: [Qemu-devel] [PATCH] vl.c: fix -usb option assertion failure in qemu_opt_get_bool_helper()

[Marcel Apfelbaum <marcel@redhat.com>]

On 01/31/2015 11:23 AM, Jan Kiszka wrote:
> On 2015-01-05 12:37, Jan Kiszka wrote:
>> On 2015-01-05 12:22, Stefan Hajnoczi wrote:
>>> Commit 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6 ("machine: remove
>>> qemu_machine_opts global list") removed option descriptions from the
>>> -machine QemuOptsList to avoid repeating MachineState's QOM properties.
>>>
>>> This change broke vl.c:usb_enabled() because qemu_opt_get_bool() cannot
>>> be used on QemuOptsList without option descriptions since QemuOpts
>>> doesn't know the type and therefore left an unparsed string value.
>>>
>>> This patch avoids calling qemu_opt_get_bool() to fix the assertion
>>> failure:
>>>
>>>    $ qemu-system-x86_64 -usb
>>>    qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
>>>
>>> Test the presence of -usb using qemu_opt_find() but use the
>>> MachineState->usb field instead of qemu_opt_get_bool().
>>>
>>> Cc: Marcel Apfelbaum <marcel.a@redhat.com>
>>> Cc: Tiejun Chen <tiejun.chen@intel.com>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>   vl.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index bea9656..6e8889c 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -999,8 +999,11 @@ static int parse_name(QemuOpts *opts, void *opaque)
>>>
>>>   bool usb_enabled(bool default_usb)
>>>   {
>>> -    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>>> -                             has_defaults && default_usb);
>>> +    if (qemu_opt_find(qemu_get_machine_opts(), "usb")) {
>>> +        return current_machine->usb;
>>> +    } else {
>>> +        return has_defaults && default_usb;
>>> +    }
>>>   }
>>>
>>>   #ifndef _WIN32
>>>
>>
>> That still leaves the other boolean machine options broken. A generic
>> fix would be good. Or revert the original commit until we have one.
>
> Just pulled current master, and at least the iommu machine option is
> still triggering an abort.
>
> What is the status of fixing these fallouts? Was anything else addressed
> by now, just this one forgotten?
No, is not forgotten. I waited to see that the approach to fix this issue
is accepted by the reviewers.

Thanks for the reminder, patches will be submitted soon.
Marcel


>
> Jan
>
>