Re: [PATCH 17/46] qemu-option: Smooth error checking with Coccinelle

[Eric Blake]

On 6/24/20 11:43 AM, Markus Armbruster 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.  Coccinelle script:

> Eliminate error_propagate() that are now unnecessary.  Delete @err
> that are now unused.  Tidy up line breaks and whitespace.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

>   32 files changed, 70 insertions(+), 192 deletions(-)

Another decent chunk of cleanups.

> diff --git a/block.c b/block.c
> index 30a72bc4c2..77e85f13db 100644
> --- a/block.c
> +++ b/block.c

> @@ -6091,8 +6086,8 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,
>       }
>   
>       if (base_filename) {
> -        qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, &local_err);
> -        if (local_err) {
> +        if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename,
> +                          &local_err)) {
>               error_setg(errp, "Backing file not supported for file format 
> '%s'",
>                          fmt);

Pre-existing - it is odd that we collect a message into local_err, then 
write something else into errp; the out: label does 
error_propagate(errp, local_err) which ensures there is no leak but 
discards the original err.  You could pass NULL instead.  But as it is 
pre-existing, passing NULL should be a separate patch.

>               goto out;
> @@ -6100,8 +6095,7 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,
>       }
>   
>       if (base_fmt) {
> -        qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err);
> -        if (local_err) {
> +        if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err)) {
>               error_setg(errp, "Backing file format not supported for file "
>                                "format '%s'", fmt);

Ditto.

> +++ b/qemu-img.c
> @@ -467,8 +467,8 @@ static int add_old_style_options(const char *fmt, QemuOpts 
> *opts,
>       Error *err = NULL;
>   
>       if (base_filename) {
> -        qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, &err);
> -        if (err) {
> +        if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename,
> +                          &err)) {
>               error_report("Backing file not supported for file format '%s'",
>                            fmt);
>               error_free(err);
> @@ -476,8 +476,7 @@ static int add_old_style_options(const char *fmt, QemuOpts 
> *opts,
>           }
>       }
>       if (base_fmt) {
> -        qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &err);
> -        if (err) {
> +        if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &err)) {
>               error_report("Backing file format not supported for file "
>                            "format '%s'", fmt);
>               error_free(err);

Ditto.

But the conversion here is sane.
Reviewed-by: Eric Blake <eblake@redhat.com>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org