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

[Eric Blake]

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 <address@hidden>
> ---

> @@ -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?

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)

> +++ 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature