|
From: | Eric Blake |
Subject: | Re: [Qemu-block] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker |
Date: | Wed, 26 Sep 2018 21:17:48 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |