Ari Sundholm <a...@tuxera.com> writes:
> Hi,
>
> On 9/11/20 11:03 AM, Markus Armbruster wrote:
>> Ari Sundholm <a...@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
>>>> <vsement...@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!
