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

[Ari Sundholm]

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