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

[Markus Armbruster <armbru@redhat.com>]

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?