[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
From: |
Ari Sundholm |
Subject: |
Re: [PATCH 07/14] block/blklogwrites: drop error propagation |
Date: |
Fri, 11 Sep 2020 00:01:21 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
Hi Vladimir!
Thank you for working on this. My comments below.
On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
It's simple to avoid error propagation in blk_log_writes_open(), we
just need to refactor blk_log_writes_find_cur_log_sector() a bit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/blklogwrites.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 7ef046cee9..c7da507b2d 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -96,10 +96,10 @@ static inline bool
blk_log_writes_sector_size_valid(uint32_t sector_size)
sector_size < (1ull << 24);
}
-static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
- uint32_t sector_size,
- uint64_t nr_entries,
- Error **errp)
+static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
I'd rather not change the return type for reasons detailed below.
+ uint32_t sector_size,
+ uint64_t nr_entries,
+ Error **errp)
{
uint64_t cur_sector = 1;
uint64_t cur_idx = 0;
@@ -112,13 +112,13 @@ static uint64_t
blk_log_writes_find_cur_log_sector(BdrvChild *log,
if (read_ret < 0) {
error_setg_errno(errp, -read_ret,
"Failed to read log entry %"PRIu64, cur_idx);
- return (uint64_t)-1ull;
+ return read_ret;
This is OK, provided the change in return type signedness is necessary
in the first place.
}
if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
le64_to_cpu(cur_entry.flags), cur_idx);
- return (uint64_t)-1ull;
+ return -EINVAL;
This is OK, provided the return type signedness change is necessary,
although we already do have errp to indicate any errors.
}
/* Account for the sector of the entry itself */
@@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict
*options, int flags,
{
BDRVBlkLogWritesState *s = bs->opaque;
QemuOpts *opts;
- Error *local_err = NULL;
int ret;
uint64_t log_sector_size;
bool log_append;
@@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
QDict *options, int flags,
s->nr_entries = 0;
if (blk_log_writes_sector_size_valid(log_sector_size)) {
- s->cur_log_sector =
+ int64_t cur_log_sector =
blk_log_writes_find_cur_log_sector(s->log_file,
log_sector_size,
- le64_to_cpu(log_sb.nr_entries),
&local_err);
- if (local_err) {
- ret = -EINVAL;
- error_propagate(errp, local_err);
+ le64_to_cpu(log_sb.nr_entries), errp);
+ if (cur_log_sector < 0) {
+ ret = cur_log_sector;
This changes the semantics slightly. Changing the return type to int64
may theoretically cause valid return values to be interpreted as errors.
Since we already do have a dedicated out pointer for the error value
(errp), why not use it?
Assigning cur_log_sector to ret looks a bit odd to me. Why not use the
original -EINVAL - or maybe some more appropriate errno value than -EINVAL?
I think the desired effect of this change could be achieved with less
churn. How about just making just the following change in addition to
adding ERRP_GUARD() to the beginning of the function and getting rid of
local_err:
- le64_to_cpu(log_sb.nr_entries),
&local_err);
+ le64_to_cpu(log_sb.nr_entries), errp);
- if (local_err) {
+ if (*errp) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail_log;
}
+ s->cur_log_sector = cur_log_sector;
s->nr_entries = le64_to_cpu(log_sb.nr_entries);
}
} else {
Best regards,
Ari Sundholm
ari@tuxera.com
- Re: [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error, (continued)
Re: [PATCH 07/14] block/blklogwrites: drop error propagation,
Ari Sundholm <=
[PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Vladimir Sementsov-Ogievskiy, 2020/09/09
- Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Greg Kurz, 2020/09/11
- Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Vladimir Sementsov-Ogievskiy, 2020/09/11
- Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Greg Kurz, 2020/09/11
- Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Vladimir Sementsov-Ogievskiy, 2020/09/11
- Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Markus Armbruster, 2020/09/11
- Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Vladimir Sementsov-Ogievskiy, 2020/09/11