Re: [PATCH 3/7] block: Make bdrv_{pread, pwrite}() return 0 on success

[Paolo Bonzini <pbonzini@redhat.com>]

On 5/13/22 01:38, Alberto Faria wrote:
> 
> @@ -113,7 +113,7 @@ static ssize_t qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset,
>          error_setg_errno(errp, -ret, "Could not read encryption header");
>          return -1;
>      }
> -    return ret;
> +    return buflen;
>  }
>  
>  
> @@ -174,7 +174,7 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset,
>          error_setg_errno(errp, -ret, "Could not read encryption header");
>          return -1;
>      }
> -    return ret;
> +    return buflen;
>  }
>  
>  static QDict*

As a follow-up you could change the calling convention of readfunc and 
writefunc in crypto/block-luks.c and crypto/block-qcow.c.

More in general, crypto/block-luks.c and crypto/block-qcow.c should be 
annotated for coroutine_fn.  Let's put it on the todo list.

> @@ -88,6 +88,7 @@ static int vhdx_log_peek_hdr(BlockDriverState *bs, VHDXLogEntries *log,
>      if (ret < 0) {
>          goto exit;
>      }
> +    ret = sizeof(VHDXLogEntryHeader);
>      vhdx_log_entry_hdr_le_import(hdr);
>  
>  exit:

The callers only check the return code of vhdx_log_peek_hdr, 
vhdx_log_read_sectors, vhdx_log_write_sectors with ret < 0.  But looking 
at the callers:

- vhdx_log_read_desc propagates ret directly from a successful 
vhdx_log_read_sectors, but its callers don't care about which 
nonnegative result is returned

- vhdx_validate_log_entry might occasionally return ret directly from a 
successful vhdx_log_read_desc or vhdx_log_read_sectors, but 
vhdx_log_search, the caller of vhdx_validate_log_entry, also doesn't 
care about which nonnegative result is returned

- vhdx_log_flush only returns a successful return value from bdrv_flush

- vhdx_log_write propagates ret directly from a successful 
vhdx_log_write_sectors, but vhdx_log_write_and_flush only returns a 
successful return value from vhdx_log_flush

So (perhaps as a separate patch?) you can remove the three assignments 
of ret.


A possible cleanup is missing in vdi_co_pwritev:

>         ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
>                           n_sectors * SECTOR_SIZE);
>     }
> 
>     return ret < 0 ? ret : 0;

ret is returned by either bdrv_pwrite or bdrv_co_writev, so it can be 
simplified to just "return ret".


As an aside, while reviewing I noticed this in qcow2_mark_dirty:

     ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, 
incompatible_features),
                       &val, sizeof(val));
     if (ret < 0) {
         return ret;
     }
     ret = bdrv_flush(bs->file->bs);
     if (ret < 0) {
         return ret;
     }

I'm not sure if there are other occurrencies, perhaps you can try using 
Coccinelle to find them and change them to a bdrv_pwrite_sync.

But anyway:

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo