Re: [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

[Vladimir Sementsov-Ogievskiy]

17.09.2020 19:35, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy 
> <vsementsov@virtuozzo.com> wrote:
> > -/* qcow2_load_dirty_bitmaps()
> > - * Return value is a hint for caller: true means that the Qcow2 header was
> > - * updated. (false doesn't mean that the header should be updated by the
> > - * caller, it just means that updating was not needed or the image cannot be
> > - * written to).
> > - * On failure the function returns false.
> > - */
> > -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> > +/* Return true on success, false on failure. */
> > +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> > +                              Error **errp)
>
> I think that the documentation should clarify under what conditions
> 'header_updated' is modified.
>
> >       if (s->nb_bitmaps == 0) {
> >           /* No bitmaps - nothing to do */
> > -        return false;
> > +        return true;
> >       }
>
> Here is it not for example (should it be set to false?).

Ha, I think, it just shows that patch is wrong :) We should set header_updated 
at least on every success path. Or better always (if it is non-NULL of course). 
Thanks for careful review!

> > -    if (bm_list == NULL) {
> > +    if (!bm_list) {
> >           return false;
> >       }
>
> This looks like a cosmetic change unrelated to the rest of the patch.
>
> Berto


--
Best regards,
Vladimir