Re: [PATCH 07/14] block/blklogwrites: drop error propagation

From: Markus Armbruster <armbru@redhat.com>
To: Greg Kurz <groug@kaod.org>
Cc: kwolf@redhat.com,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	berto@igalia.com, stefanha@redhat.com, qemu-block@nongnu.org,
	qemu-devel@nongnu.org, mreitz@redhat.com,
	pavel.dovgaluk@ispras.ru, pbonzini@redhat.com, jsnow@redhat.com,
	ari@tuxera.com
Subject: Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Date: Fri, 11 Sep 2020 07:23:10 +0200	[thread overview]
Message-ID: <878sdghoox.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200910192440.695b8e81@bahia.lan> (Greg Kurz's message of "Thu,  10 Sep 2020 19:24:40 +0200")

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 {