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

[Alberto Faria <afaria@redhat.com>]

On Fri, May 13, 2022 at 9:22 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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.

Thanks, I'll fix this. I think I'll just fold it in, but let me know
if it really should be split into a separate patch.

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

Looks like this is the only occurrence. I'll add a patch to convert it
to bdrv_pwrite_sync().

Alberto