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

[Chen, Tiejun]

On 2015/1/6 14:20, Shannon Zhao wrote:
> On 2015/1/6 10:37, Chen, Tiejun wrote:
> > On 2015/1/5 20:14, Marcel Apfelbaum wrote:
> > > On 01/05/2015 01:50 PM, Stefan Hajnoczi wrote:
> > > > On Mon, Jan 5, 2015 at 11:37 AM, Jan Kiszka <address@hidden>
> > > > 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 <address@hidden>
> > > > > > Cc: Tiejun Chen <address@hidden>
> > > > > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > > > > > ---
> > > > > >    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.
> >
> > Can this work out currently?
> Hi Tiejun,
>
> I apply your following patch and it works. At least it doesn't crash.

Thanks for your test.

Tiejun

> Thanks,
> Shannon
>
> > ---
> >   util/qemu-option.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/util/qemu-option.c b/util/qemu-option.c
> > index a708241..933b885 100644
> > --- a/util/qemu-option.c
> > +++ b/util/qemu-option.c
> > @@ -317,7 +317,7 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
> >       }
> >
> >       opt = qemu_opt_find(opts, name);
> > -    if (!opt) {
> > +    if (!opt || !opt->desc)  {
> >           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> >           if (desc && desc->def_value_str) {
> >               return desc->def_value_str;
> > @@ -341,7 +341,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
> >       }
> >
> >       opt = qemu_opt_find(opts, name);
> > -    if (!opt) {
> > +    if (!opt || !opt->desc)  {
> >           desc = find_desc_by_name(opts->list->desc, name);
> >           if (desc && desc->def_value_str) {
> >               str = g_strdup(desc->def_value_str);
> > @@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const 
> > char *name,
> >       }
> >
> >       opt = qemu_opt_find(opts, name);
> > -    if (opt == NULL) {
> > +    if (!opt || !opt->desc)  {
> >           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> >           if (desc && desc->def_value_str) {
> >               parse_option_bool(name, desc->def_value_str, &ret, &error_abort);
> > @@ -413,7 +413,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, 
> > const char *name,
> >       }
> >
> >       opt = qemu_opt_find(opts, name);
> > -    if (opt == NULL) {
> > +    if (!opt || !opt->desc)  {
> >           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> >           if (desc && desc->def_value_str) {
> >               parse_option_number(name, desc->def_value_str, &ret, 
> > &error_abort);
> > @@ -450,7 +450,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, 
> > const char *name,
> >       }
> >
> >       opt = qemu_opt_find(opts, name);
> > -    if (opt == NULL) {
> > +    if (!opt || !opt->desc)  {
> >           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> >           if (desc && desc->def_value_str) {
> >               parse_option_size(name, desc->def_value_str, &ret, &error_abort);