[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 07/14] block/blklogwrites: drop error propagation |
Date: |
Fri, 11 Sep 2020 07:23:10 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Greg Kurz <groug@kaod.org> writes:
> On Wed, 9 Sep 2020 21:59:23 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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,
>> + 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 changes the error status of blk_log_writes_open() from -EINVAL to
> whatever is returned by bdrv_pread(). I guess this is an improvement
> worth to be mentioned in the changelog.
>
>> }
>>
>> 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;
>> }
>>
>> /* 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 is converting int64_t to int, which is usually not recommended.
> In practice this is probably okay because cur_log_sector is supposed
> to be a negative errno (ie. an int) in this case.
It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
on error.
Aside: returning uint64_t is awkward. We commonly use a signed integer
for non-negative offset or negative error.
> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
Unless we change blk_log_writes_find_cur_log_sector() to return a
negative errno code, we need to ret = -EINVAL here. Let's keep this
series simple.
>> goto fail_log;
>> }
>>
>> + s->cur_log_sector = cur_log_sector;
>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>> }
>> } else {
- Re: [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd, (continued)
Re: [PATCH 07/14] block/blklogwrites: drop error propagation, Ari Sundholm, 2020/09/10
[PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps, Vladimir Sementsov-Ogievskiy, 2020/09/09