[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/7] block: Make bdrv_{pread, pwrite}() return 0 on success
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH 3/7] block: Make bdrv_{pread, pwrite}() return 0 on success |
Date: |
Fri, 13 May 2022 10:22:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 |
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
- [PATCH 0/7] Implement bdrv_{pread, pwrite, pwrite_sync, pwrite_zeroes}() using generated_co_wrapper, Alberto Faria, 2022/05/12
- [PATCH 3/7] block: Make bdrv_{pread,pwrite}() return 0 on success, Alberto Faria, 2022/05/12
- Re: [PATCH 3/7] block: Make bdrv_{pread, pwrite}() return 0 on success,
Paolo Bonzini <=
- [PATCH 2/7] block: Change bdrv_{pread, pwrite, pwrite_sync}() param order, Alberto Faria, 2022/05/12
- [PATCH 1/7] block: Add a 'flags' param to bdrv_{pread, pwrite, pwrite_sync}(), Alberto Faria, 2022/05/12
- [PATCH 4/7] block: Make bdrv_co_pwrite() take a const buffer, Alberto Faria, 2022/05/12
- [PATCH 6/7] block: Implement bdrv_{pread, pwrite, pwrite_zeroes}() using generated_co_wrapper, Alberto Faria, 2022/05/12