| Message ID | 1420436129-8467-1-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
On 01/04/2015 10:35 PM, Tiejun Chen wrote: > After one commit 49d2e648e808, "machine: remove qemu_machine_opts > global list", is introduced, QEMU doesn't keep a global list of > options but set desc lately. Then we can see the following, > > $ x86_64-softmmu/qemu-system-x86_64 -usb > qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \ > Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. > Aborted (core dumped) > > So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name() > to work parse_option_bool() out just in case of !opt->desc. > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > --- > util/qemu-option.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index a708241..7cb3601 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -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 == NULL) || !opt->desc) { Over-parenthesized, and looks like you also introduced a spurious space. Simpler to just have: if (!opt || !opt->desc) { Also, there are other threads about the same topic. https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html
On 2015/1/6 1:13, Eric Blake wrote: > On 01/04/2015 10:35 PM, Tiejun Chen wrote: >> After one commit 49d2e648e808, "machine: remove qemu_machine_opts >> global list", is introduced, QEMU doesn't keep a global list of >> options but set desc lately. Then we can see the following, >> >> $ x86_64-softmmu/qemu-system-x86_64 -usb >> qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \ >> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. >> Aborted (core dumped) >> >> So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name() >> to work parse_option_bool() out just in case of !opt->desc. >> >> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >> --- >> util/qemu-option.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/util/qemu-option.c b/util/qemu-option.c >> index a708241..7cb3601 100644 >> --- a/util/qemu-option.c >> +++ b/util/qemu-option.c >> @@ -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 == NULL) || !opt->desc) { > > Over-parenthesized, and looks like you also introduced a spurious space. > Simpler to just have: > > if (!opt || !opt->desc) { Looks good. > > Also, there are other threads about the same topic. > https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html Yeah, I already notice that. Thanks Tiejun
On 2015/1/6 9:21, Chen, Tiejun wrote: > On 2015/1/6 1:13, Eric Blake wrote: >> On 01/04/2015 10:35 PM, Tiejun Chen wrote: >>> After one commit 49d2e648e808, "machine: remove qemu_machine_opts >>> global list", is introduced, QEMU doesn't keep a global list of >>> options but set desc lately. Then we can see the following, >>> >>> $ x86_64-softmmu/qemu-system-x86_64 -usb >>> qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \ >>> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. >>> Aborted (core dumped) >>> >>> So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name() >>> to work parse_option_bool() out just in case of !opt->desc. >>> >>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >>> --- >>> util/qemu-option.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/util/qemu-option.c b/util/qemu-option.c >>> index a708241..7cb3601 100644 >>> --- a/util/qemu-option.c >>> +++ b/util/qemu-option.c >>> @@ -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 == NULL) || !opt->desc) { >> >> Over-parenthesized, and looks like you also introduced a spurious space. >> Simpler to just have: >> >> if (!opt || !opt->desc) { > > Looks good. > >> >> Also, there are other threads about the same topic. >> https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html > > Yeah, I already notice that. > Then I realize I need to extend this to all qemu_opt_get_*. Tiejun
On Tue, Jan 06, 2015 at 10:39:13AM +0800, Chen, Tiejun wrote: > On 2015/1/6 9:21, Chen, Tiejun wrote: > >On 2015/1/6 1:13, Eric Blake wrote: > >>On 01/04/2015 10:35 PM, Tiejun Chen wrote: > >>>After one commit 49d2e648e808, "machine: remove qemu_machine_opts > >>>global list", is introduced, QEMU doesn't keep a global list of > >>>options but set desc lately. Then we can see the following, > >>> > >>>$ x86_64-softmmu/qemu-system-x86_64 -usb > >>>qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \ > >>> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. > >>>Aborted (core dumped) > >>> > >>>So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name() > >>>to work parse_option_bool() out just in case of !opt->desc. > >>> > >>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > >>>--- > >>> util/qemu-option.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>>diff --git a/util/qemu-option.c b/util/qemu-option.c > >>>index a708241..7cb3601 100644 > >>>--- a/util/qemu-option.c > >>>+++ b/util/qemu-option.c > >>>@@ -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 == NULL) || !opt->desc) { > >> > >>Over-parenthesized, and looks like you also introduced a spurious space. > >> Simpler to just have: > >> > >>if (!opt || !opt->desc) { > > > >Looks good. > > > >> > >>Also, there are other threads about the same topic. > >>https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html > > > >Yeah, I already notice that. > > > > Then I realize I need to extend this to all qemu_opt_get_*. Marcel is working on a fix. I think hacking qemu_opt_get_* is not a clean solution. The intention was to move away from QemuOpts to QOM properties. The patch that removed -machine QemuOptsList descriptions was buggy, but the proper fix is to make that approach work instead of hacking qemu_opt_get_*. Stefan
On 01/06/2015 04:56 PM, Stefan Hajnoczi wrote: > On Tue, Jan 06, 2015 at 10:39:13AM +0800, Chen, Tiejun wrote: >> On 2015/1/6 9:21, Chen, Tiejun wrote: >>> On 2015/1/6 1:13, Eric Blake wrote: >>>> On 01/04/2015 10:35 PM, Tiejun Chen wrote: >>>>> After one commit 49d2e648e808, "machine: remove qemu_machine_opts >>>>> global list", is introduced, QEMU doesn't keep a global list of >>>>> options but set desc lately. Then we can see the following, >>>>> >>>>> $ x86_64-softmmu/qemu-system-x86_64 -usb >>>>> qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \ >>>>> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. >>>>> Aborted (core dumped) >>>>> >>>>> So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name() >>>>> to work parse_option_bool() out just in case of !opt->desc. >>>>> >>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >>>>> --- >>>>> util/qemu-option.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c >>>>> index a708241..7cb3601 100644 >>>>> --- a/util/qemu-option.c >>>>> +++ b/util/qemu-option.c >>>>> @@ -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 == NULL) || !opt->desc) { >>>> >>>> Over-parenthesized, and looks like you also introduced a spurious space. >>>> Simpler to just have: >>>> >>>> if (!opt || !opt->desc) { >>> >>> Looks good. >>> >>>> >>>> Also, there are other threads about the same topic. >>>> https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html >>> >>> Yeah, I already notice that. >>> >> >> Then I realize I need to extend this to all qemu_opt_get_*. > > Marcel is working on a fix. I just posted a fix on the mailing list. https://www.mail-archive.com/qemu-devel@nongnu.org/msg272607.html Thanks, Marcel > > I think hacking qemu_opt_get_* is not a clean solution. The intention > was to move away from QemuOpts to QOM properties. The patch that > removed -machine QemuOptsList descriptions was buggy, but the proper fix > is to make that approach work instead of hacking qemu_opt_get_*. > > Stefan >
On 2015/1/6 22:56, Stefan Hajnoczi wrote: > On Tue, Jan 06, 2015 at 10:39:13AM +0800, Chen, Tiejun wrote: >> On 2015/1/6 9:21, Chen, Tiejun wrote: >>> On 2015/1/6 1:13, Eric Blake wrote: >>>> On 01/04/2015 10:35 PM, Tiejun Chen wrote: >>>>> After one commit 49d2e648e808, "machine: remove qemu_machine_opts >>>>> global list", is introduced, QEMU doesn't keep a global list of >>>>> options but set desc lately. Then we can see the following, >>>>> >>>>> $ x86_64-softmmu/qemu-system-x86_64 -usb >>>>> qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \ >>>>> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. >>>>> Aborted (core dumped) >>>>> >>>>> So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name() >>>>> to work parse_option_bool() out just in case of !opt->desc. >>>>> >>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >>>>> --- >>>>> util/qemu-option.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c >>>>> index a708241..7cb3601 100644 >>>>> --- a/util/qemu-option.c >>>>> +++ b/util/qemu-option.c >>>>> @@ -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 == NULL) || !opt->desc) { >>>> >>>> Over-parenthesized, and looks like you also introduced a spurious space. >>>> Simpler to just have: >>>> >>>> if (!opt || !opt->desc) { >>> >>> Looks good. >>> >>>> >>>> Also, there are other threads about the same topic. >>>> https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html >>> >>> Yeah, I already notice that. >>> >> >> Then I realize I need to extend this to all qemu_opt_get_*. > > Marcel is working on a fix. > > I think hacking qemu_opt_get_* is not a clean solution. The intention > was to move away from QemuOpts to QOM properties. The patch that > removed -machine QemuOptsList descriptions was buggy, but the proper fix > is to make that approach work instead of hacking qemu_opt_get_*. > Understand. Thanks Tiejun
diff --git a/util/qemu-option.c b/util/qemu-option.c index a708241..7cb3601 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -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 == NULL) || !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);
After one commit 49d2e648e808, "machine: remove qemu_machine_opts global list", is introduced, QEMU doesn't keep a global list of options but set desc lately. Then we can see the following, $ x86_64-softmmu/qemu-system-x86_64 -usb qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper: \ Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed. Aborted (core dumped) So inside qemu_opt_get_bool_helper, we need to call find_desc_by_name() to work parse_option_bool() out just in case of !opt->desc. Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> --- util/qemu-option.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)