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.