Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/7] block/dirty-bitmaps: add block_dirty_bitmap_check function

[John Snow]

On 3/1/19 2:36 PM, Eric Blake wrote:
> On 3/1/19 1:15 PM, John Snow wrote:
>> Instead of checking against busy, inconsistent, or read only directly,
>> use a check function with permissions bits that let us streamline the
>> checks without reproducing them in many places.
>>
>> Included in this patch are permissions changes that simply add the
>> inconsistent check to existing permissions call spots, without
>> addressing existing bugs.
>>
>> In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
>> which checks against all three conditions. busy-only checks become
>> BDRV_BITMAP_ALLOW_RO.
>>
>> Notably, remove allows inconsistent bitmaps, so it doesn't follow the 
>> pattern.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  include/block/dirty-bitmap.h   | 13 ++++++++-
>>  block/dirty-bitmap.c           | 38 +++++++++++++++++++-------
>>  blockdev.c                     | 49 +++++++---------------------------
>>  migration/block-dirty-bitmap.c | 12 +++------
>>  nbd/server.c                   |  3 +--
>>  5 files changed, 54 insertions(+), 61 deletions(-)
>>
> 
> Diffstat proves its a win, even with the extra documentation for the new
> function. Nice.
> 

It would have been even more obvious if I had added the individual
"inconsistent" checks before conversion, so that it's even close to
breaking even seems like a win.

>> +int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>> +                            Error **errp)
>> +{
>> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
>> +                   " operation and cannot be used", bitmap->name);
> 
> Split before space,
> 
>> +        return -1;
>> +    }
>> +
>> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>> +                   bitmap->name);
>> +        return -1;
>> +    }
>> +
>> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
>> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>> +                   bitmap->name);
>> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
>> +                          "this bitmap from disk");
> 
> split after space.  Looks inconsistent within a single function (pardon
> the pun :)
> 

Ah... I've never known how to split strings. In fact, does anyone?
I'll address this either in staging or as a follow-up, as I assume
Vladimir will have some comments for me.

--js

> That's minor,
> Reviewed-by: Eric Blake <address@hidden>
> 

Thanks!