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

[Markus Armbruster]

Ari Sundholm <ari@tuxera.com> writes:

> Hi,
>
> On 9/11/20 11:03 AM, Markus Armbruster wrote:
>> Ari Sundholm <ari@tuxera.com> writes:
>> 
>>> 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?
>> Error.h's big comment:
>>   * Error reporting system loosely patterned after Glib's GError.
>>   *
>>   * = Rules =
>>   [...]
>>   * - Whenever practical, also return a value that indicates success /
>>   *   failure.  This can make the error checking more concise, and can
>>   *   avoid useless error object creation and destruction.  Note that
>>   *   we still have many functions returning void.  We recommend
>>   *   • bool-valued functions return true on success / false on failure,
>>   *   • pointer-valued functions return non-null / null pointer, and
>>   *   • integer-valued functions return non-negative / negative.
>> blk_log_writes_find_cur_log_sector() does return such an error value
>> before the patch: (uint64_t)-1.
>> The caller does not use it to check for errors.  It uses @err
>> instead.
>> Awkward, has to error_propagate().
>> Avoiding error_propagate() reduces the error handling boileplate.
>> It
>> also improves behavior when @errp is &error_abort: we get the abort
>> right where the error happens instead of where we propagate it.
>> Furthermore, caller has to make an error code (-EINVAL), because
>> returning (uint64_t)-1 throws it away.  Yes, a detailed error is stored
>> into @err, but you can't cleanly extract the error code.
>> Using a signed integer for returning "non-negative offset or
>> negative
>> errno code" is pervasive, starting with read() and write().  It hasn't
>> been a problem there, and it hasn't been a problem in the block layer.
>> 8 exbi-blocks should do for a while.  Should it become troublesome, we
>> won't solve the problem by going unsigned and adding one bit, we'll
>> double the width and add 64.
>> 
>
> I am in complete agreement with eliminating error propagation within
> the blklogwrites driver. This was never a point of disagreement.
>
> As error propagation is dropped in this patch, the awkwardness
> referred to above will be no more, making that a moot point.
>
> My main issue was that this patch does more than just the mechanical
> transformation required. Changes that are not strictly necessary are 
> made, and they slightly change the semantics while duplicating the
> error code and halving the range of the return type (instead of just
> returning *some* value in the absence of a possibility to return
> nothing, which will be thrown away by the caller anyway when an error
> has occurred).

You have a point: the return type change should perhaps be a separate
patch.  Up to the maintainer.

> However, as the consensus seems to be that it is best to change the
> return type to int64_t for consistency with the rest of the codebase,
> I will not object any further regarding that point. Having conceded
> that, this makes the difference between my preferred minimalistic
> approach and this patch insignificant, and it is not my intention to
> needlessly obstruct a perfectly fine patch series.
>
> BTW, I do not think it would be difficult at all to extend the code in
> error.h to make it convenient to extract errno and/or win32 error
> values that have been explicitly provided, but this is a matter best
> discussed separately and left for a later patch.

Certainly feasible, but how would we deal with the fact that only some
Error objects have an error code?

> As for general matters regarding error handling and separation between
> results and errors, I am open to discussing these off-list.

Thanks!