Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

11.09.2020 12:38, Greg Kurz wrote:
> s/startus/status
>
> On Wed,  9 Sep 2020 21:59:27 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
> > It's better to return status together with setting errp. It makes
> > possible to avoid error propagation.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/qcow2.h        |  2 +-
> >   block/qcow2-bitmap.c | 13 ++++++-------
> >   2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index e7e662533b..49824be5c6 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
> >                                   Qcow2BitmapInfoList **info_list, Error 
> > **errp);
> >   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> >   int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
> > -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> > +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >                                             bool release_stored, Error **errp);
> >   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> >   bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
> > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> > index f58923fce3..5eeff1cb1c 100644
> > --- a/block/qcow2-bitmap.c
> > +++ b/block/qcow2-bitmap.c
> > @@ -1524,9 +1524,10 @@ out:
> >    * readonly to begin with, and whether we opened directly or reopened to that
> >    * state shouldn't matter for the state we get afterward.
> >    */
> > -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> > +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >                                             bool release_stored, Error **errp)
> >   {
> > +    ERRP_GUARD();
>
> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
> an error_prepend(errp, ...) not visible in the patch context.

Ah yes. Actually this is occasional thing I didn't want to include into this 
patch
(and int this part I). So we can just drop it and leave for part II or part III,
or add a note into commit message

> Anyway,
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks a lot for reviewing :)

Hmm.. With this series I understand the following:

1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the 
whole code-base, because:

  - it produces a lot of "if (*errp)" in places where it is really simple to 
avoid error propagation at all, like in this series
  - reviewing is the hardest part of the process

So, if we have to review these changes anyway, it's better to invest a bit more 
time into patch creation, and make code correspond to our modern error API 
recommendations.

2. So, the process turns into following steps:

  - apply scripts/coccinelle/errp-guard.cocci
  - look through patches and do obvious refactorings (like this series)
  - keep ERRP_GUARD where necessary (appending info to error, or where 
refactoring of function return status is too invasive and not simple)

3. Obviously, that's too much for me :) Of course, I will invest some time into 
making the series like this one, and reviewing them, but I can't do it for 
weeks and months. (My original сunning plan to simply push ~100 generated 
commits with my s-o-b and become the greatest contributor failed:)

The good thing is that now, with ERRP_GUARD finally merged, we can produce 
parallel series like this, and they will be processed in parallel by different 
maintainers (and Markus will have to merge series for subsystems with 
unavailable maintainers).

So, everybody is welcome to the process [2]. Probably we want to make a 
separate announcement in a list with short recommendations and instructions? 
But who read announcements..

> >       BdrvDirtyBitmap *bitmap;
> >       BDRVQcow2State *s = bs->opaque;
> >       uint32_t new_nb_bitmaps = s->nb_bitmaps;
> > @@ -1546,7 +1547,7 @@ void 
> > qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> >           bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> >                                      s->bitmap_directory_size, errp);
> >           if (bm_list == NULL) {
> > -            return;
> > +            return false;
> >           }
> >       }
> >   
> > @@ -1661,7 +1662,7 @@ success:
> >       }
> >   
> >       bitmap_list_free(bm_list);
> > -    return;
> > +    return true;
> >   
> >   fail:
> >       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> > @@ -1679,16 +1680,14 @@ fail:
> >       }
> >   
> >       bitmap_list_free(bm_list);
> > +    return false;
> >   }
> >   
> >   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
> >   {
> >       BdrvDirtyBitmap *bitmap;
> > -    Error *local_err = NULL;
> >   
> > -    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
> > -    if (local_err != NULL) {
> > -        error_propagate(errp, local_err);
> > +    if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
> >           return -EINVAL;
> >       }
> >   


--
Best regards,
Vladimir