qemu_opt_get_bool_helper: back finding desc by name just if !opt->desc

RFC PATCH In review Tiejun Chen 4 participants 7 messages Jan 5, 2015 – Jan 7, 2015

Share & tools

Tiejun Chen

author

2015-01-05 05:38 UTC #0 lore raw

RFC PATCH 1/1 qemu_opt_get_bool_helper: back finding desc by name just if !opt->desc

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 <[email protected]>

---
 util/qemu-option.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

util/qemu-option.c +1 -1

index a708241..7cb3601 100644
@@ -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);

Signature · 1 line

1.9.1

Eric Blake

2015-01-05 17:13 UTC #1 lore raw

Quoted 22 lines · Tiejun Chen

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 <[email protected]>
---
 util/qemu-option.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
util/qemu-option.c +0 -0
index a708241..7cb3601 100644

util/qemu-option.c static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, · line 380

Quoted 5 lines

util/qemu-option.c +1 -1

@@ -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

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Chen, Tiejun

author

2015-01-06 01:21 UTC #2 lore raw

Quoted 25 lines · Eric Blake

Quoted 24 lines · Tiejun Chen
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 <[email protected]>
---
  util/qemu-option.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
util/qemu-option.c +0 -0
index a708241..7cb3601 100644
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name,
      }
 

Quoted 8 lines

util/qemu-option.c static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, · line 380
Quoted 3 lines
util/qemu-option.c +1 -1
      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.

Quoted 3 lines

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

Chen, Tiejun

author

2015-01-06 02:39 UTC #3 lore raw

Quoted 36 lines · Tiejun

Quoted 26 lines · Eric Blake
Quoted 25 lines · Tiejun Chen
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 <[email protected]>
---
  util/qemu-option.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
util/qemu-option.c +0 -0
index a708241..7cb3601 100644
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts
 *opts, const char *name,
      }
 
Quoted 8 lines
util/qemu-option.c static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, · line 380
Quoted 3 lines
util/qemu-option.c +1 -1
      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) {

Quoted 8 lines

Looks good.
Quoted 3 lines
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

Stefan Hajnoczi

2015-01-06 14:57 UTC #4 lore raw

Quoted 40 lines · Tiejun

Quoted 39 lines · Tiejun
Quoted 26 lines · Eric Blake
Quoted 25 lines · Tiejun Chen
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 <[email protected]>
---
 util/qemu-option.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
util/qemu-option.c +0 -0
index a708241..7cb3601 100644
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts
 *opts, const char *name,
     }
 
Quoted 8 lines
util/qemu-option.c static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, · line 380
Quoted 3 lines
util/qemu-option.c +1 -1
     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.
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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

Marcel Apfelbaum

2015-01-06 16:50 UTC #5 lore raw

Quoted 44 lines · Stefan Hajnoczi

Quoted 43 lines · Tiejun
Quoted 42 lines · Tiejun
Quoted 26 lines · Eric Blake
Quoted 25 lines · Tiejun Chen
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 <[email protected]>
---
  util/qemu-option.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
util/qemu-option.c +0 -0
index a708241..7cb3601 100644
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts
 *opts, const char *name,
      }
 
Quoted 8 lines
util/qemu-option.c static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, · line 380
Quoted 3 lines
util/qemu-option.c +1 -1
      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.
Quoted 3 lines
Also, there are other threads about the same topic.
https://lists.gnu.org/archive/html/qemu-devel/2015-01/msg00130.html

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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/[email protected]/msg272607.html

Thanks,
Marcel

Quoted 8 lines

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

Chen, Tiejun

author

2015-01-07 01:24 UTC #6 lore raw

In reply to Stefan Hajnoczi

Quoted 47 lines · Stefan Hajnoczi

Quoted 46 lines · Tiejun
Quoted 44 lines · Tiejun
Quoted 26 lines · Eric Blake
Quoted 25 lines · Tiejun Chen
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 <[email protected]>
---
  util/qemu-option.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
util/qemu-option.c +0 -0
index a708241..7cb3601 100644
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts
 *opts, const char *name,
      }
 
Quoted 8 lines
util/qemu-option.c static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, · line 380
Quoted 3 lines
util/qemu-option.c +1 -1
      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.
Quoted 3 lines
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.

Quoted 9 lines

Quoted 1 line
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