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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> 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.

PATCH 20.

>>               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.

Likewise.

>> +++ 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.

PATCH 44.

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

Thanks!