qemu-option: Cope with a missing QemuOptsList

[PATCH] qemu-option: Cope with a missing QemuOptsList

Posted by Fabiano Rosas 1 week, 5 days ago

It's a common pattern in vl.c to chain the qemu_find_opts() output
into the first argument of qemu_opts_parse_noisily():

opts = qemu_opts_parse_noisily(qemu_find_opts("spice"), optarg, false);
if (!opts) {
    exit(1);
}

In cases such as spice that have the group defined in the module file,
it's possible to reach qemu_opts_parse_noisily() with a NULL QemuOptsList if
the module is not present in the host filesystem.

  $ ../configure --enable-modules --enable-spice
  $ make
  $ mv qemu-bundle/usr/local/lib64/qemu/ui-spice-core.so{,.not}
  $ ./qemu-system-x86_64 -spice a
  qemu-system-x86_64: -spice a: There is no option group 'spice'
  Segmentation fault (core dumped)

Return NULL from qemu_opts_parse_noisily() if there is no list.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 util/qemu-option.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9fbf425f86..0ec12d252c 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -945,6 +945,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
     QemuOpts *opts;
     bool help_wanted = false;
 
+    if (!list) {
+        return NULL;
+    }
+
     opts = opts_parse(list, params, permit_abbrev, true,
                       opts_accepts_any(list) ? NULL : &help_wanted,
                       &err);
-- 
2.53.0

Re: [PATCH] qemu-option: Cope with a missing QemuOptsList

Posted by Markus Armbruster 1 week, 5 days ago

Fabiano Rosas <farosas@suse.de> writes:

> It's a common pattern in vl.c to chain the qemu_find_opts() output
> into the first argument of qemu_opts_parse_noisily():
>
> opts = qemu_opts_parse_noisily(qemu_find_opts("spice"), optarg, false);
> if (!opts) {
>     exit(1);
> }
>
> In cases such as spice that have the group defined in the module file,
> it's possible to reach qemu_opts_parse_noisily() with a NULL QemuOptsList if
> the module is not present in the host filesystem.
>
>   $ ../configure --enable-modules --enable-spice
>   $ make
>   $ mv qemu-bundle/usr/local/lib64/qemu/ui-spice-core.so{,.not}
>   $ ./qemu-system-x86_64 -spice a
>   qemu-system-x86_64: -spice a: There is no option group 'spice'
>   Segmentation fault (core dumped)
>
> Return NULL from qemu_opts_parse_noisily() if there is no list.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  util/qemu-option.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 9fbf425f86..0ec12d252c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -945,6 +945,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
>      QemuOpts *opts;
>      bool help_wanted = false;
>  
> +    if (!list) {
> +        return NULL;
> +    }
> +
>      opts = opts_parse(list, params, permit_abbrev, true,
>                        opts_accepts_any(list) ? NULL : &help_wanted,
>                        &err);

Before the patch, qemu_opts_parse_noisily() either

* Succeeds and returns non-null

* Fails, reports an error, and returns null

* Prints help and returns null

Your patch adds a fourth case:

* Fails silently and returns null

I dislike this case.  Functions should either always print something
whent they fail, or never.

Are all callers prepared for silent failure?

The pattern you quoted in the commit message is, because
qemu_find_opts() reports an error.

Here's a cleaner solution for this pattern.  Replace

    opts = qemu_opts_parse_noisily(qemu_find_opts(...), ...)

by a call of a new helper function that does

    list = qemu_find_opts(...);
    if (!list) {
        return NULL;
    }
    return qemu_opts_parse_noisily(list, ...);

Thoughts?

Re: [PATCH] qemu-option: Cope with a missing QemuOptsList

Posted by Fabiano Rosas 1 week, 2 days ago

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> It's a common pattern in vl.c to chain the qemu_find_opts() output
>> into the first argument of qemu_opts_parse_noisily():
>>
>> opts = qemu_opts_parse_noisily(qemu_find_opts("spice"), optarg, false);
>> if (!opts) {
>>     exit(1);
>> }
>>
>> In cases such as spice that have the group defined in the module file,
>> it's possible to reach qemu_opts_parse_noisily() with a NULL QemuOptsList if
>> the module is not present in the host filesystem.
>>
>>   $ ../configure --enable-modules --enable-spice
>>   $ make
>>   $ mv qemu-bundle/usr/local/lib64/qemu/ui-spice-core.so{,.not}
>>   $ ./qemu-system-x86_64 -spice a
>>   qemu-system-x86_64: -spice a: There is no option group 'spice'
>>   Segmentation fault (core dumped)
>>
>> Return NULL from qemu_opts_parse_noisily() if there is no list.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  util/qemu-option.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index 9fbf425f86..0ec12d252c 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>> @@ -945,6 +945,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
>>      QemuOpts *opts;
>>      bool help_wanted = false;
>>  
>> +    if (!list) {
>> +        return NULL;
>> +    }
>> +
>>      opts = opts_parse(list, params, permit_abbrev, true,
>>                        opts_accepts_any(list) ? NULL : &help_wanted,
>>                        &err);
>
> Before the patch, qemu_opts_parse_noisily() either
>
> * Succeeds and returns non-null
>
> * Fails, reports an error, and returns null
>
> * Prints help and returns null
>
> Your patch adds a fourth case:
>
> * Fails silently and returns null
>
> I dislike this case.  Functions should either always print something
> whent they fail, or never.
>
> Are all callers prepared for silent failure?
>
> The pattern you quoted in the commit message is, because
> qemu_find_opts() reports an error.
>
> Here's a cleaner solution for this pattern.  Replace
>
>     opts = qemu_opts_parse_noisily(qemu_find_opts(...), ...)
>
> by a call of a new helper function that does
>
>     list = qemu_find_opts(...);
>     if (!list) {
>         return NULL;
>     }
>     return qemu_opts_parse_noisily(list, ...);
>
> Thoughts?

I agree, but I didn't want to add another helper on top of
qemu_opts_parse_noisily() so I did some further cleanup, see whether you
hate it:

https://lore.kernel.org/r/20260918223002.1892021-1-farosas@suse.de

Re: [PATCH] qemu-option: Cope with a missing QemuOptsList

Posted by Marc-André Lureau 1 week, 5 days ago

On Tue, Sep 15, 2026 at 5:45 PM Fabiano Rosas <farosas@suse.de> wrote:
>
> It's a common pattern in vl.c to chain the qemu_find_opts() output
> into the first argument of qemu_opts_parse_noisily():
>
> opts = qemu_opts_parse_noisily(qemu_find_opts("spice"), optarg, false);
> if (!opts) {
>     exit(1);
> }
>
> In cases such as spice that have the group defined in the module file,
> it's possible to reach qemu_opts_parse_noisily() with a NULL QemuOptsList if
> the module is not present in the host filesystem.
>
>   $ ../configure --enable-modules --enable-spice
>   $ make
>   $ mv qemu-bundle/usr/local/lib64/qemu/ui-spice-core.so{,.not}
>   $ ./qemu-system-x86_64 -spice a
>   qemu-system-x86_64: -spice a: There is no option group 'spice'
>   Segmentation fault (core dumped)
>
> Return NULL from qemu_opts_parse_noisily() if there is no list.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  util/qemu-option.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 9fbf425f86..0ec12d252c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -945,6 +945,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
>      QemuOpts *opts;
>      bool help_wanted = false;
>
> +    if (!list) {
> +        return NULL;
> +    }
> +
>      opts = opts_parse(list, params, permit_abbrev, true,
>                        opts_accepts_any(list) ? NULL : &help_wanted,
>                        &err);
> --
> 2.53.0
>
>