| Message ID | 20180925234924.14338-2-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | dirty-bitmaps: fix QMP command permissions | expand |
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 <jsnow@redhat.com> > --- > 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". anyway, Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > + > void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) > { > qemu_mutex_lock(bitmap->mutex); > diff --git a/blockdev.c b/blockdev.c > index 670ae5bbde..dedcebb0fa 100644 > --- a/blockdev.c > +++ 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; > } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { > error_setg(errp, "Cannot clear a disabled bitmap"); > @@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, > return; > } > > - if (bdrv_dirty_bitmap_frozen(bitmap)) { > + if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) { > error_setg(errp, > - "Bitmap '%s' is currently frozen and cannot be removed", > - name); > - return; > - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { > - error_setg(errp, > - "Bitmap '%s' is currently locked and cannot be removed", > - name); > + "Bitmap '%s' is currently in-use by another operation and" > + " cannot be removed", name); > return; > } > > @@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, > return; > } > > - if (bdrv_dirty_bitmap_frozen(bitmap)) { > + if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) { > error_setg(errp, > - "Bitmap '%s' is currently frozen and cannot be modified", > - name); > - return; > - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { > - error_setg(errp, > - "Bitmap '%s' is currently locked and cannot be modified", > - name); > + "Bitmap '%s' is currently in-use by another operation" > + " and cannot be cleared", name); > return; > } else if (!bdrv_dirty_bitmap_enabled(bitmap)) { > error_setg(errp, > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index 201ff7f20b..92cf7e39d2 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -94,6 +94,7 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); > bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); > bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap); > bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap); > +bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap); > bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs); > BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap);
26.09.2018 14:53, 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 <jsnow@redhat.com> >> --- >> 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". bad reason to rewrite the whole series, so, ignore this comment) > > anyway, > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> + >> void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool >> qmp_locked) >> { >> qemu_mutex_lock(bitmap->mutex); >> diff --git a/blockdev.c b/blockdev.c >> index 670ae5bbde..dedcebb0fa 100644 >> --- a/blockdev.c >> +++ 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; >> } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { >> error_setg(errp, "Cannot clear a disabled bitmap"); >> @@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char >> *node, const char *name, >> return; >> } >> - if (bdrv_dirty_bitmap_frozen(bitmap)) { >> + if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) { >> error_setg(errp, >> - "Bitmap '%s' is currently frozen and cannot be >> removed", >> - name); >> - return; >> - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { >> - error_setg(errp, >> - "Bitmap '%s' is currently locked and cannot be >> removed", >> - name); >> + "Bitmap '%s' is currently in-use by another >> operation and" >> + " cannot be removed", name); >> return; >> } >> @@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const >> char *node, const char *name, >> return; >> } >> - if (bdrv_dirty_bitmap_frozen(bitmap)) { >> + if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) { >> error_setg(errp, >> - "Bitmap '%s' is currently frozen and cannot be >> modified", >> - name); >> - return; >> - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { >> - error_setg(errp, >> - "Bitmap '%s' is currently locked and cannot be >> modified", >> - name); >> + "Bitmap '%s' is currently in-use by another >> operation" >> + " and cannot be cleared", name); >> return; >> } else if (!bdrv_dirty_bitmap_enabled(bitmap)) { >> error_setg(errp, >> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >> index 201ff7f20b..92cf7e39d2 100644 >> --- a/include/block/dirty-bitmap.h >> +++ b/include/block/dirty-bitmap.h >> @@ -94,6 +94,7 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); >> bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); >> bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap); >> bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap); >> +bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap); >> bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs); >> BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap); > >
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 <jsnow@redhat.com> >> --- >> 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 <vsementsov@virtuozzo.com> > >> +++ 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)
On 9/26/18 7:19 AM, Vladimir Sementsov-Ogievskiy wrote: > 26.09.2018 14:53, 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. >>> >> to reduce number of '!', we may use opposite check, for ex >> "bdrv_dirty_bitmap_user_locked". > > bad reason to rewrite the whole series, so, ignore this comment) Then again, if you change 'in-use' to 'in use' in the entire series, you might as well flip the logic (double-negative logic is slightly harder to follow than suitably-named positive logic). At this point, I have not queued this series on my NBD queue, because there may indeed be a reason for v4 to touch up spelling and logic.
On 09/26/2018 10:17 PM, Eric Blake wrote: > 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 <jsnow@redhat.com> >>> --- >>> 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 <vsementsov@virtuozzo.com> >> > >>> +++ 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) > I'll change both; since you both remarked on the double negation. (Just how my brain thinks, I suppose -- I prefer routines that check the positive instead of the negation, but most callers do indeed want the negation.)
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)); +} + void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) { qemu_mutex_lock(bitmap->mutex); diff --git a/blockdev.c b/blockdev.c index 670ae5bbde..dedcebb0fa 100644 --- a/blockdev.c +++ 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; } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { error_setg(errp, "Cannot clear a disabled bitmap"); @@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) { error_setg(errp, - "Bitmap '%s' is currently frozen and cannot be removed", - name); - return; - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { - error_setg(errp, - "Bitmap '%s' is currently locked and cannot be removed", - name); + "Bitmap '%s' is currently in-use by another operation and" + " cannot be removed", name); return; } @@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) { error_setg(errp, - "Bitmap '%s' is currently frozen and cannot be modified", - name); - return; - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { - error_setg(errp, - "Bitmap '%s' is currently locked and cannot be modified", - name); + "Bitmap '%s' is currently in-use by another operation" + " and cannot be cleared", name); return; } else if (!bdrv_dirty_bitmap_enabled(bitmap)) { error_setg(errp, diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 201ff7f20b..92cf7e39d2 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -94,6 +94,7 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap); +bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap); bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs); BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
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 <jsnow@redhat.com> --- block/dirty-bitmap.c | 6 ++++++ blockdev.c | 29 ++++++++--------------------- include/block/dirty-bitmap.h | 1 + 3 files changed, 15 insertions(+), 21 deletions(-)