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

[Fabiano Rosas <farosas@suse.de>]

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