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

[Marcel Apfelbaum <marcel@redhat.com>]

On 01/05/2015 01:50 PM, Stefan Hajnoczi wrote:
> On Mon, Jan 5, 2015 at 11:37 AM, Jan Kiszka <jan.kiszka@siemens.com> 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.
>
> I think we should revert the original commit.
>
> qemu_option_get_*() callers need to be converted to query MachineState
> fields instead of using QemuOpts functions.
>
> This means we need to modify the way default values are processed.
> MachineState fields should start with the default value and be
> overridden by command-line options.  Right now it works differently:
> we query the command-line option and fall back to the default if the
> option wasn't set.  That approach doesn't work for MachineState fields
> since C types (bool, int, etc) aren't tristate in the general case, so
> they cannot indicate whether or not they were set.
>
> The benefit of doing this work is that we eliminate the QemuOpts layer
> and work directly with QOM properties instead.
>
> Marcel: Do you want to do this or do you have another fix in mind?
Hi, sorry for not getting to this earlier, I thought Paolo already fixed
it, but I was wrong.

I think this is the right direction, I'll am going to take care of it. (At least I'll try...)

Thanks,
Marcel
>
> Stefan
>