Re: [Qemu-devel] [PATCH 1/4] block-commit: Expose granularity

[Max Reitz <mreitz@redhat.com>]

On 07.04.2014 21:06, Eric Blake wrote:
> On 04/07/2014 11:29 AM, Max Reitz wrote:
>> Allow QMP users to manipulate the granularity used in the block-commit
>> command.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> @@ -214,6 +215,13 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>>       orig_base_flags    = bdrv_get_flags(base);
>>       orig_overlay_flags = bdrv_get_flags(overlay_bs);
>>   
>> +    if (!granularity) {
>> +        granularity = COMMIT_BUFFER_SIZE;
>> +    }
> Default granularity of 0 becomes buffer size...
>
>> @@ -1876,6 +1876,15 @@ void qmp_block_commit(const char *device,
>>        */
>>       BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>   
>> +    granularity = has_granularity ? granularity : 0;
>> +
>> +    if (has_granularity &&
>> +        (granularity < BDRV_SECTOR_SIZE || (granularity & (granularity - 1))))
>> +    {
>> +        error_set(errp, QERR_INVALID_PARAMETER, "granularity");
> ...but this code rejects attempts for me to explicitly set granularity
> to the default.  Should it be 'if (granularity &&' instead of your
> current wording?

I intentionally rejected a granularity of 0, as I thought the user were 
always capable of dropping this parameter for obtaining the default. But 
since you are bringing this up, I guess there may be cases where a user 
is forced to give the parameter but wants to keep the default value 
nonetheless; I will change it to accept 0 in v2.

> Also, is it worth using qemu-common.h's is_power_of_2 instead of
> inlining it yourself?  (I don't care, as I recognize the bit
> manipulations, but other readers might prefer the named version for its
> legibility)

You are right, I will use the function from qemu-common.h.

Max

>> +++ b/qapi-schema.json
>> @@ -2112,6 +2112,9 @@
>>   #
>>   # @speed:  #optional the maximum speed, in bytes per second
>>   #
>> +# @granularity: #optional the granularity to be used for the operation, in
>> +#               bytes; has to be a power of two and at least 512 (since 2.1)
>> +#
> At least you documented here that an explicit '0' is rejected, even
> though it might be nicer to allow it for the sake of requesting the
> default even if the default later changes in size.
>
> Overall, though, I'm liking this series.
>