On 10/03/2018 08:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.10.2018 02:02, 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 <js...@redhat.com>
>> ---
>> block/dirty-bitmap.c | 6 ++++++
>> blockdev.c | 29 ++++++++---------------------
>> include/block/dirty-bitmap.h | 1 +
>> migration/block-dirty-bitmap.c | 10 ++--------
>> 4 files changed, 17 insertions(+), 29 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 8ac933cf1c..85bc668f6a 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_locked(BdrvDirtyBitmap *bitmap) {
>> + return (bdrv_dirty_bitmap_frozen(bitmap) ||
>> + bdrv_dirty_bitmap_qmp_locked(bitmap));
>
> hmm, extra parentheses
I'm fond of them, but I'll take them out.
>
>> +}
>> +
>> void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool
>> qmp_locked)
>> {
>> qemu_mutex_lock(bitmap->mutex);
>
> [...]
>
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void)
>> goto fail;
>> }
>>
>> - if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> - error_report("Can't migrate frozen dirty bitmap: '%s",
>> - bdrv_dirty_bitmap_name(bitmap));
>> - goto fail;
>> - }
>> -
>> - if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
>> - error_report("Can't migrate locked dirty bitmap: '%s",
>> + if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>> + error_report("Can't migrate a bitmap that is in use: '%s'",
>
> "by another operation" like in other cases?
>
Oh, sure. Can't hurt.
>> bdrv_dirty_bitmap_name(bitmap));
>> goto fail;
>> }
>
> anyway,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>
> --
> Best regards,
> Vladimir
>
Thanks, I'll just make these edits and trust that Eric is fine with it
as well.
--js
