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

[Vladimir Sementsov-Ogievskiy]

11.09.2020 18:22, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
> > 11.09.2020 14:21, Greg Kurz wrote:
> > > On Fri, 11 Sep 2020 13:18:32 +0300
> > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> > >
> > > > 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
>
> Either works for me.
>
> > > > > Anyway,
> > > > >
> > > > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > >
> > > > Thanks a lot for reviewing :)
> > > Don't mention it :)
> > >
> > > > 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.
>
> Yes, going the extra mile is better.
>
> I recommend it for code that is actively maintained.  Making the code
> simpler and thus easier to maintain is an investment that'll pay off.
>
> We may have code where it won't pay off.  Do you think a blind
> application of errp-guard.cocci might be better than nothing there?

I think, careful review is needed anyway. And it may be too large effort for 
dead (or almost dead) code.
So, let's start from popular subsystems. And make a decision for the rest later.

> > > > 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)
> > > I've started to follow this process for the spapr code and, indeed,
> > > I
> > > can come up with better changes by refactoring some code manually.
> > > Some of these changes are not that obvious that they could be made
> > > by someone who doesn't know the code, so I tend to agree with your
> > > arguments in 1.
> > > This is also the reason I didn't review patches 10, 13 and 14
> > > because
> > > they looked like I should understand the corresponding code a bit more.
> > >
> > > > 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:)
>
> LOL
>
> A lesser craftsman than you would've peddled the generated commits
> anyway.  Props!
>
> > > Ha ha :D ... as a consolation prize, maybe we can reach a fair
> > > number
> > > of r-b by reviewing each other's _simple_ cleanups ;-)
> > >
> > > > 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).
>
> If people care enough about [2] to submit patches, I'll feel obliged to
> help merging them.
>
> > > This sounds nice. My only concern would be to end up fixing code
> > > nobody
> > > uses or cares for, so I guess it would be better that active maintainers
> > > or supporters give impetus on that.
>
> A bit of weeding on the side is always appreciated, but please don't
> feel obliged to sink lots of time into code you don't care about.
>
> > > > 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..
> >
> > > I don't :) but the very massive series that were posted on the topic
> > > the last few months look like an announcement to me, at least for
> > > active maintainers and supporters.
> >
> > Aha, I know. Better than announcement is improving checkpatch.
>
> Yes, automated feedback works best.
>
> Relentless pressure from reviewers can also work in the long, long run.
> But it's tiresome.
>
> Of course, checkpatch.pl checks only new or changed code.  Any ideas on
> how to make people aware of the opportunity to simplify their existing
> code?  Obvious: posting more patches simplifying existing code we care
> about.  Any other smart ideas?

I think it would be great for checkpatch to warn about some code problems in 
function touched by checked patch. Code-style, error-api, etc. This will 
attract developers to improve existing code they are interested in.


--
Best regards,
Vladimir