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

[Paolo Bonzini]

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 <pbonz...@redhat.com>

Paolo