Commit Message

"Frozen" was a good description a long time ago, but it isn't adequate now.
Rename the frozen predicate to has_successor to make the semantics of the
predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically
make sense. For enabled and disabled in particular, it's actually okay for
the internals to do this but only forbidden for users to invoke them, and
all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use
by another operation -- use the qmp_locked function for this instead, which
presently also checks for has_successor.
---
block/dirty-bitmap.c | 32 +++++++++++++++++---------------
include/block/dirty-bitmap.h | 2 +-
migration/block-dirty-bitmap.c | 2 +-
3 files changed, 19 insertions(+), 17 deletions(-)

Comments

On 2/11/19 7:02 PM, John Snow wrote:
> "Frozen" was a good description a long time ago, but it isn't adequate now.> Rename the frozen predicate to has_successor to make the semantics of the> predicate more clear to outside callers.> > In the process, remove some calls to frozen() that no longer semantically> make sense. For enabled and disabled in particular, it's actually okay for> the internals to do this but only forbidden for users to invoke them, and> all of the QMP entry uses already check against qmp_locked.> > Several other assertions really want to check that the bitmap isn't in-use> by another operation -- use the qmp_locked function for this instead, which> presently also checks for has_successor.> ---
Missing S-o-b on entire series, so you have to send v2 anyway :)
> @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,> uint64_t granularity;> BdrvDirtyBitmap *child;> > - if (bdrv_dirty_bitmap_frozen(bitmap)) {> - error_setg(errp, "Cannot create a successor for a bitmap that is "> - "currently frozen");> + if (bdrv_dirty_bitmap_has_successor(bitmap)) {> + error_setg(errp, "Cannot create a successor for a bitmap that already "> + "has one");> + return -1;> + }> + if (bdrv_dirty_bitmap_user_locked(bitmap)) {> + error_setg(errp, "Cannot create a successor for a bitmap that is in-use "> + "by an operation.");> return -1;
No trailing dot in error_setg().
Should these two errors be swapped (check for locked before
has_successor)? After all, having a successor is an internal detail,
whereas being in use by something I already triggered is fairly
straightforward to understand.
> @@ -325,7 +328,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,> /**> * In cases of failure where we can no longer safely delete the parent,> * we may wish to re-join the parent and child/successor.> - * The merged parent will be un-frozen, but not explicitly re-enabled.> + * The merged parent will not be user_locked, but not explicitly re-enabled.
s/but not/nor/

On 2/12/19 1:26 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:>> "Frozen" was a good description a long time ago, but it isn't adequate now.>> Rename the frozen predicate to has_successor to make the semantics of the>> predicate more clear to outside callers.>>>> In the process, remove some calls to frozen() that no longer semantically>> make sense. For enabled and disabled in particular, it's actually okay for>> the internals to do this but only forbidden for users to invoke them, and>> all of the QMP entry uses already check against qmp_locked.>>>> Several other assertions really want to check that the bitmap isn't in-use>> by another operation -- use the qmp_locked function for this instead, which>> presently also checks for has_successor.>> ---> > Missing S-o-b on entire series, so you have to send v2 anyway :)> >> @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,>> uint64_t granularity;>> BdrvDirtyBitmap *child;>> >> - if (bdrv_dirty_bitmap_frozen(bitmap)) {>> - error_setg(errp, "Cannot create a successor for a bitmap that is ">> - "currently frozen");>> + if (bdrv_dirty_bitmap_has_successor(bitmap)) {>> + error_setg(errp, "Cannot create a successor for a bitmap that already ">> + "has one");>> + return -1;>> + }>> + if (bdrv_dirty_bitmap_user_locked(bitmap)) {>> + error_setg(errp, "Cannot create a successor for a bitmap that is in-use ">> + "by an operation.");>> return -1;> > No trailing dot in error_setg().>
D'oh. I need to re-enable checkpatch, obviously.
> Should these two errors be swapped (check for locked before> has_successor)? After all, having a successor is an internal detail,> whereas being in use by something I already triggered is fairly> straightforward to understand.>
Good point. Will do.
> >> @@ -325,7 +328,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,>> /**>> * In cases of failure where we can no longer safely delete the parent,>> * we may wish to re-join the parent and child/successor.>> - * The merged parent will be un-frozen, but not explicitly re-enabled.>> + * The merged parent will not be user_locked, but not explicitly re-enabled.> > s/but not/nor/>
I was trying to draw a contrast between "We will forcibly set locked =
false, but make no guaranteed about enable/disable."
I guess nor still works in that case.