Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[John Snow <jsnow@redhat.com>]

On 11/07/2014 07:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>> +    if (!name || name[0] == '\0') {
> Isn't is better to move "name[0] == '\0'" check to
> bdrv_create_dirty_bitmap, near existed name checking?

Hm, OK, but then we still need to check for the presence of a name in 
these functions, so we're still going to be checking the validity of the 
name in both places regardless.

Probably not worth the effort. If somebody adds an empty string bitmap 
using bdrv_dirty_bitmap_create, it will at least be the only one named 
as such, and I don't think it will create any problems.

>> +        if (granularity < 512 || is_power_of_2(granularity)) {
>> +            error_setg(errp, "Granularity must be power of 2 "
>> +                             "and greater than 512");
>> +            return;
>> +        }
>> +    } else {
>> +        granularity = 65536;
>> +    }
> Why not using something like DEFAULT_CLUSTER_SIZE, as in block/qcow2.h ?
>

I don't want to tie this to explicitly qcow2, but on a suggestion from 
Fam, I think I will look at how block/mirror.c does this and tie it to 
the current cluster size -- which, if it's qcow2 -- will accomplish what 
you want, here.

-- 
—js