| Message ID | 20190213232356.21034-3-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | dirty-bitmaps: deprecate @status field | expand |
On 2/13/19 5:23 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. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/dirty-bitmap.c | 32 +++++++++++++++++--------------- > include/block/dirty-bitmap.h | 2 +- > migration/block-dirty-bitmap.c | 2 +- > 3 files changed, 19 insertions(+), 17 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
14.02.2019 2:23, 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 I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? I think, it would be simpler for me to read patch itself :) > 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. hm, you mean user_locked, not qmp_locked. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/dirty-bitmap.c | 32 +++++++++++++++++--------------- > include/block/dirty-bitmap.h | 2 +- > migration/block-dirty-bitmap.c | 2 +- > 3 files changed, 19 insertions(+), 17 deletions(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 101383b3af..639ebc0645 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap { > HBitmap *meta; /* Meta dirty bitmap */ > bool qmp_locked; /* Bitmap is locked, it can't be modified > through QMP */ > - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */ > + BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */ aha, looks like a good moment to fix preexisting misalignment of the comment, but new line is exactly 80 characters length, so, not a good moment) > char *name; /* Optional non-empty unique ID */ > int64_t size; /* Size of the bitmap, in bytes */ > bool disabled; /* Bitmap is disabled. It ignores all writes to > @@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) > } > > /* Called with BQL taken. */ > -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) > +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) > { > return bitmap->successor; > } > > /* Both conditions disallow user-modification via QMP. */ > bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { > - return bdrv_dirty_bitmap_frozen(bitmap) || > + return bdrv_dirty_bitmap_has_successor(bitmap) || > bdrv_dirty_bitmap_qmp_locked(bitmap); > } > > @@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) > /* Called with BQL taken. */ > DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) > { > - if (bdrv_dirty_bitmap_frozen(bitmap)) { > + if (bdrv_dirty_bitmap_has_successor(bitmap)) { > return DIRTY_BITMAP_STATUS_FROZEN; > } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { > return DIRTY_BITMAP_STATUS_LOCKED; > @@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap) > > /** > * Create a successor bitmap destined to replace this bitmap after an operation. > - * Requires that the bitmap is not frozen and has no successor. > + * Requires that the bitmap is not locked and has no successor. I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in other comments in this patch. > * Called with BQL taken. > */ > int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, > @@ -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_user_locked(bitmap)) { > + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " > + "by an operation"); > + return -1; > + } > + if (bdrv_dirty_bitmap_has_successor(bitmap)) { > + error_setg(errp, "Cannot create a successor for a bitmap that already " > + "has one"); Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion.. > return -1; > } > - assert(!bitmap->successor); > > /* Create an anonymous successor */ > granularity = bdrv_dirty_bitmap_granularity(bitmap);
18.02.2019 16:57, Vladimir Sementsov-Ogievskiy wrote: > 14.02.2019 2:23, 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 > > I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? > I think, it would be simpler for me to read patch itself :) > >> 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. > > hm, you mean user_locked, not qmp_locked. > >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/dirty-bitmap.c | 32 +++++++++++++++++--------------- >> include/block/dirty-bitmap.h | 2 +- >> migration/block-dirty-bitmap.c | 2 +- >> 3 files changed, 19 insertions(+), 17 deletions(-) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 101383b3af..639ebc0645 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap { >> HBitmap *meta; /* Meta dirty bitmap */ >> bool qmp_locked; /* Bitmap is locked, it can't be modified >> through QMP */ >> - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */ >> + BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */ > > aha, looks like a good moment to fix preexisting misalignment of the comment, > but new line is exactly 80 characters length, so, not a good moment) and there was no any misalignment, only thunderbird bug...
On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote: > 14.02.2019 2:23, 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 > > I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? > I think, it would be simpler for me to read patch itself :) > Touched this up. I meant enable and disable, not enabled and disabled. >> 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. > > hm, you mean user_locked, not qmp_locked. > Yes. [...] >> /** >> * Create a successor bitmap destined to replace this bitmap after an operation. >> - * Requires that the bitmap is not frozen and has no successor. >> + * Requires that the bitmap is not locked and has no successor. > > I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in > other comments in this patch. > You're right. It gets changed again later, but I didn't make this easy to read. >> * Called with BQL taken. >> */ >> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> @@ -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_user_locked(bitmap)) { >> + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " >> + "by an operation"); >> + return -1; >> + } >> + if (bdrv_dirty_bitmap_has_successor(bitmap)) { >> + error_setg(errp, "Cannot create a successor for a bitmap that already " >> + "has one"); > > > Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion.. > It gets changed later in the series, but I didn't do a great job of explaining that in advance. I'll amend the commit message to explain what I'm trying to do. I tried to hint at this with: "which presently also checks for has_successor" as an admission that it was redundant, but I need to call it out in stronger language.
19.02.2019 1:32, John Snow wrote: > > > On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote: >> 14.02.2019 2:23, 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 >> >> I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? >> I think, it would be simpler for me to read patch itself :) >> > > Touched this up. I meant enable and disable, not enabled and disabled. > >>> 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. >> >> hm, you mean user_locked, not qmp_locked. >> > > Yes. > > [...] > >>> /** >>> * Create a successor bitmap destined to replace this bitmap after an operation. >>> - * Requires that the bitmap is not frozen and has no successor. >>> + * Requires that the bitmap is not locked and has no successor. >> >> I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in >> other comments in this patch. >> > > You're right. It gets changed again later, but I didn't make this easy > to read. > >>> * Called with BQL taken. >>> */ >>> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >>> @@ -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_user_locked(bitmap)) { >>> + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " >>> + "by an operation"); >>> + return -1; >>> + } >>> + if (bdrv_dirty_bitmap_has_successor(bitmap)) { >>> + error_setg(errp, "Cannot create a successor for a bitmap that already " >>> + "has one"); >> >> >> Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion.. >> > > It gets changed later in the series, but I didn't do a great job of > explaining that in advance. I'll amend the commit message to explain > what I'm trying to do. > > I tried to hint at this with: "which presently also checks for > has_successor" as an admission that it was redundant, but I need to call > it out in stronger language. > Hmm, isn't it better to keep an assertion, than add dead code, to be fixed in later commits?
On 2/19/19 5:17 AM, Vladimir Sementsov-Ogievskiy wrote: > 19.02.2019 1:32, John Snow wrote: >> >> >> On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 14.02.2019 2:23, 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 >>> >>> I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? >>> I think, it would be simpler for me to read patch itself :) >>> >> >> Touched this up. I meant enable and disable, not enabled and disabled. >> >>>> 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. >>> >>> hm, you mean user_locked, not qmp_locked. >>> >> >> Yes. >> >> [...] >> >>>> /** >>>> * Create a successor bitmap destined to replace this bitmap after an operation. >>>> - * Requires that the bitmap is not frozen and has no successor. >>>> + * Requires that the bitmap is not locked and has no successor. >>> >>> I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in >>> other comments in this patch. >>> >> >> You're right. It gets changed again later, but I didn't make this easy >> to read. >> >>>> * Called with BQL taken. >>>> */ >>>> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >>>> @@ -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_user_locked(bitmap)) { >>>> + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " >>>> + "by an operation"); >>>> + return -1; >>>> + } >>>> + if (bdrv_dirty_bitmap_has_successor(bitmap)) { >>>> + error_setg(errp, "Cannot create a successor for a bitmap that already " >>>> + "has one"); >>> >>> >>> Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion.. >>> >> >> It gets changed later in the series, but I didn't do a great job of >> explaining that in advance. I'll amend the commit message to explain >> what I'm trying to do. >> >> I tried to hint at this with: "which presently also checks for >> has_successor" as an admission that it was redundant, but I need to call >> it out in stronger language. >> > > Hmm, isn't it better to keep an assertion, than add dead code, to be fixed in later commits? > Eh. I wrote code that looked semantically correct without worrying about what the calls actually do: - We want to make sure this bitmap is not in use (user_locked, qmp_locked, or busy -- however you want to spell it), and - We want to make sure this bitmap doesn't have a successor. Now, you and I happen to know that these two conditions aren't actually independent, but that's not necessarily obvious from this one function to a new reader. Adding the second check gives some assurance to the reader. In my mind, the concept of having a successor is now distinct from that of being busy, and create_successor actually wants to check both things: (1) That is able to create a successor, because it doesn't have one, and (2) That it is not modifying a bitmap in use by some operation. But, you're right, there's no way to have a bitmap with a successor that isn't busy, so an assertion will suffice for the instructional purpose. I'll change it. --js
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 101383b3af..639ebc0645 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap { HBitmap *meta; /* Meta dirty bitmap */ bool qmp_locked; /* Bitmap is locked, it can't be modified through QMP */ - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */ + BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */ char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap, in bytes */ bool disabled; /* Bitmap is disabled. It ignores all writes to @@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) } /* Called with BQL taken. */ -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) { return bitmap->successor; } /* Both conditions disallow user-modification via QMP. */ bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { - return bdrv_dirty_bitmap_frozen(bitmap) || + return bdrv_dirty_bitmap_has_successor(bitmap) || bdrv_dirty_bitmap_qmp_locked(bitmap); } @@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) { - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (bdrv_dirty_bitmap_has_successor(bitmap)) { return DIRTY_BITMAP_STATUS_FROZEN; } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { return DIRTY_BITMAP_STATUS_LOCKED; @@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap) /** * Create a successor bitmap destined to replace this bitmap after an operation. - * Requires that the bitmap is not frozen and has no successor. + * Requires that the bitmap is not locked and has no successor. * Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, @@ -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_user_locked(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " + "by an operation"); + return -1; + } + if (bdrv_dirty_bitmap_has_successor(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that already " + "has one"); return -1; } - assert(!bitmap->successor); /* Create an anonymous successor */ granularity = bdrv_dirty_bitmap_granularity(bitmap); @@ -268,7 +272,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) { - assert(!bdrv_dirty_bitmap_frozen(bitmap)); bitmap->disabled = false; } @@ -285,7 +288,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap) static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) { assert(!bitmap->active_iterators); - assert(!bdrv_dirty_bitmap_frozen(bitmap)); + assert(!bdrv_dirty_bitmap_user_locked(bitmap)); assert(!bitmap->meta); QLIST_REMOVE(bitmap, list); hbitmap_free(bitmap->bitmap); @@ -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, nor explicitly re-enabled. * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, @@ -373,7 +376,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes) bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { - assert(!bdrv_dirty_bitmap_frozen(bitmap)); + assert(!bdrv_dirty_bitmap_user_locked(bitmap)); assert(!bitmap->active_iterators); hbitmap_truncate(bitmap->bitmap, bytes); bitmap->size = bytes; @@ -391,7 +394,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) /** * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). - * There must not be any frozen bitmaps attached. + * There must not be any user_locked bitmaps attached. * This function does not remove persistent bitmaps from the storage. * Called with BQL taken. */ @@ -428,7 +431,6 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { bdrv_dirty_bitmap_lock(bitmap); - assert(!bdrv_dirty_bitmap_frozen(bitmap)); bitmap->disabled = true; bdrv_dirty_bitmap_unlock(bitmap); } diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 04a117fc81..cdbb4dfefd 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -36,7 +36,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs); uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap); -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap); +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap); const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap); int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap); DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap); diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 6426151e4f..ac6954142f 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -542,7 +542,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s) } } - if (bdrv_dirty_bitmap_frozen(s->bitmap)) { + if (bdrv_dirty_bitmap_has_successor(s->bitmap)) { bdrv_dirty_bitmap_lock(s->bitmap); if (enabled_bitmaps == NULL) { /* in postcopy */
"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. Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 32 +++++++++++++++++--------------- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-)