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

[Marcel Apfelbaum <marcel@redhat.com>]

On 02/04/2015 04:18 PM, Christian Borntraeger wrote:
> Am 02.02.2015 um 11:02 schrieb Marcel Apfelbaum:
>> 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
>
> Anything to look at a branch or something? Adding new machine bool options
> is currently pretty hard - as long as it doesnt work ;-)
It will be ready today or tomorrow :)

Thanks,
Marcel
>
> Christian
>