Re: [PULL v2 14/53] qemu-option: Use returned bool to check for failure

[Peter Maydell]

On Fri, 10 Jul 2020 at 14:31, Markus Armbruster <armbru@redhat.com> wrote:
>
> The previous commit enables conversion of
>
>     foo(..., &err);
>     if (err) {
>         ...
>     }
>
> to
>
>     if (!foo(..., &err)) {
>         ...
>     }
>
> for QemuOpts functions that now return true / false on success /
> error.

Hi; this patch changes a lot of callsites of qemu_opts_absorb_qdict()
which previously didn't check their return value to now check it,
like this:

> diff --git a/block.c b/block.c
> index 62e40db2f1..850755e04e 100644
> --- a/block.c
> +++ b/block.c
> @@ -1629,8 +1629,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockBackend *file,
>      assert(options != NULL && bs->options != options);
>
>      opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
> -    qemu_opts_absorb_qdict(opts, options, &local_err);
> -    if (local_err) {
> +    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
>          error_propagate(errp, local_err);
>          ret = -EINVAL;
>          goto fail_opts;

This has triggered Coverity's "X out of Y callsites preferred to
check the return value" heuristic. Specifically,
null_file_open() (CID 1430366) and nvme_file_open() (CID 1430347)
don't check the return value. In both cases that's in some sense
OK, because they pass error_abort; but it seems a bit inconsistent
that other foo_file_open() functions check and pass up the error
and these do not.

thanks
-- PMM