Re: [PATCH 3/7] block: Make bdrv_{pread, pwrite}() return 0 on success
On Fri, May 13, 2022 at 9:22 AM Paolo Bonzini 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),
>, 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
Re: [PATCH 3/7] block: Make bdrv_{pread, pwrite}() return 0 on success
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),
, 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
Paolo
[PATCH 3/7] block: Make bdrv_{pread,pwrite}() return 0 on success
They currently return the value of their 'bytes' parameter on success.
Make them return 0 instead, for consistency with other I/O functions and
in preparation to implement them using generated_co_wrapper. This also
makes it clear that short reads/writes are not possible.
The few callers that rely on the previous behavior are adjusted
accordingly by hand.
Signed-off-by: Alberto Faria
---
block/cloop.c| 2 +-
block/crypto.c | 4 ++--
block/dmg.c | 10 +-
block/io.c | 10 ++
block/qcow.c | 2 +-
block/qcow2.c| 4 ++--
block/qed.c | 7 +--
block/vhdx-log.c | 3 +++
block/vmdk.c | 5 ++---
tests/unit/test-block-iothread.c | 4 ++--
10 files changed, 21 insertions(+), 30 deletions(-)
diff --git a/block/cloop.c b/block/cloop.c
index 9a2334495e..40b146e714 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -222,7 +222,7 @@ static inline int cloop_read_block(BlockDriverState *bs,
int block_num)
ret = bdrv_pread(bs->file, s->offsets[block_num], bytes,
s->compressed_block, 0);
-if (ret != bytes) {
+if (ret < 0) {
return -1;
}
diff --git a/block/crypto.c b/block/crypto.c
index deec7fae2f..e7f5c4e31a 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -70,7 +70,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
error_setg_errno(errp, -ret, "Could not read encryption header");
return ret;
}
-return ret;
+return buflen;
}
static ssize_t block_crypto_write_func(QCryptoBlock *block,
@@ -88,7 +88,7 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block,
error_setg_errno(errp, -ret, "Could not write encryption header");
return ret;
}
-return ret;
+return buflen;
}
diff --git a/block/dmg.c b/block/dmg.c
index 5a460c3eb1..98db18d82a 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -390,7 +390,7 @@ static int dmg_read_plist_xml(BlockDriverState *bs,
DmgHeaderState *ds,
buffer = g_malloc(info_length + 1);
buffer[info_length] = '\0';
ret = bdrv_pread(bs->file, info_begin, info_length, buffer, 0);
-if (ret != info_length) {
+if (ret < 0) {
ret = -EINVAL;
goto fail;
}
@@ -611,7 +611,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
* inflated. */
ret = bdrv_pread(bs->file, s->offsets[chunk], s->lengths[chunk],
s->compressed_chunk, 0);
-if (ret != s->lengths[chunk]) {
+if (ret < 0) {
return -1;
}
@@ -637,7 +637,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
* inflated. */
ret = bdrv_pread(bs->file, s->offsets[chunk], s->lengths[chunk],
s->compressed_chunk, 0);
-if (ret != s->lengths[chunk]) {
+if (ret < 0) {
return -1;
}
@@ -658,7 +658,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
* inflated. */
ret = bdrv_pread(bs->file, s->offsets[chunk], s->lengths[chunk],
s->compressed_chunk, 0);
-if (ret != s->lengths[chunk]) {
+if (ret < 0) {
return -1;
}
@@ -674,7 +674,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
case UDRW: /* copy */
ret = bdrv_pread(bs->file, s->offsets[chunk], s->lengths[chunk],
s->uncompressed_chunk, 0);
-if (ret != s->lengths[chunk]) {
+if (ret < 0) {
return -1;
}
break;
diff --git a/block/io.c b/block/io.c
index 2ed963d9e0..78a289192e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1115,7 +1115,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags
flags)
int bdrv_pread(BdrvChild *child, int64_t offset, int64_t bytes, void *buf,
BdrvRequestFlags flags)
{
-int ret;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
IO_CODE();
@@ -1123,9 +1122,7 @@ int bdrv_pread(BdrvChild *child, int64_t offset, int64_t
bytes, void *buf,
return -EINVAL;
}
-ret = bdrv_preadv(child, offset, bytes, , flags);
-
-return ret < 0 ? ret : bytes;
+return bdrv_preadv(child, offset, bytes, , flags);
}
/* Return no. of bytes on success or < 0 on error. Important errors are:
@@ -1137,7 +1134,6 @@ int bdrv_pread(BdrvChild *child, int64_t offset, int64_t
bytes, void *buf,
int bdrv_pwrite(BdrvChild *child, int64_t offset, int64_t bytes,
const void *buf, BdrvRequestFlags flags)
{
-int ret;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
