Re: [Qemu-devel] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker

[Eric Blake]

On 9/26/18 6:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 26.09.2018 02:49, John Snow wrote:
> > Instead of both frozen and qmp_locked checks, wrap it into one check.
> > frozen implies the bitmap is split in two (for backup), and shouldn't
> > be modified. qmp_locked implies it's being used by another operation,
> > like being exported over NBD. In both cases it means we shouldn't allow
> > the user to modify it in any meaningful way.
> >
> > Replace any usages where we check both frozen and qmp_locked with the
> > new check.
> >
> > Signed-off-by: John Snow <address@hidden>
> > ---
> >   block/dirty-bitmap.c         |  6 ++++++
> >   blockdev.c                   | 29 ++++++++---------------------
> >   include/block/dirty-bitmap.h |  1 +
> >   3 files changed, 15 insertions(+), 21 deletions(-)
> >
> > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > index 8ac933cf1c..fc10543ab0 100644
> > --- a/block/dirty-bitmap.c
> > +++ b/block/dirty-bitmap.c
> > @@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap 
> > *bitmap)
> >       return bitmap->successor;
> >   }
> > +/* Both conditions disallow user-modification via QMP. */
> > +bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap) {
> > +    return !(bdrv_dirty_bitmap_frozen(bitmap) ||
> > +             bdrv_dirty_bitmap_qmp_locked(bitmap));
> > +}
>
> to reduce number of '!', we may use opposite check, for ex 
> "bdrv_dirty_bitmap_user_locked".

Meaning make this function return true if locked for one less negation 
in the function body...

> anyway,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> > +++ b/blockdev.c
> > @@ -2009,11 +2009,8 @@ static void 
> > block_dirty_bitmap_clear_prepare(BlkActionState *common,
> >           return;
> >       }
> > -    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> > -        error_setg(errp, "Cannot modify a frozen bitmap");
> > -        return;
> > -    } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
> > -        error_setg(errp, "Cannot modify a locked bitmap");
> > +    if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
> > +        error_setg(errp, "Cannot modify a bitmap in-use by another 
> > operation");
> >           return;

...and since most callers were negating sense as well?

I'm not sure I'm a fan of "in-use" with the hyphen. It sounds better to 
me to just spell it out as two words. (multiple instances)

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org