Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Max Reitz <mreitz@redhat.com>]

On 2015-01-16 at 11:48, John Snow wrote:
>
>
> On 01/16/2015 10:36 AM, Max Reitz wrote:
>> On 2015-01-12 at 11:30, John Snow wrote:
>>> From: Fam Zheng <famz@redhat.com>
>>>
>>> The new command pair is added to manage user created dirty bitmap. The
>>> dirty bitmap's name is mandatory and must be unique for the same 
>>> device,
>>> but different devices can have bitmaps with the same names.
>>>
>>> The granularity is an optional field. If it is not specified, we will
>>> choose a default granularity based on the cluster size if available,
>>> clamped to between 4K and 64K to mirror how the 'mirror' code was
>>> already choosing granularity. If we do not have cluster size info
>>> available, we choose 64K. This code has been factored out into a helper
>>> shared with block/mirror.
>>>
>>> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
>>> which takes a device name and a dirty bitmap name and validates the
>>> lookup, returning NULL and setting errp if there is a problem with
>>> either field. This helper will be re-used in future patches in this
>>> series.
>>>
>>> The types added to block-core.json will be re-used in future patches
>>> in this series, see:
>>> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable,
>>> disable}'
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   block.c               |  20 ++++++++++
>>>   block/mirror.c        |  10 +----
>>>   blockdev.c            | 100
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/block/block.h |   1 +
>>>   qapi/block-core.json  |  55 +++++++++++++++++++++++++++
>>>   qmp-commands.hx       |  51 +++++++++++++++++++++++++
>>>   6 files changed, 228 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index bfeae6b..3eb77ee 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap, int64_t sector
>>>       }
>>>   }
>>> +/**
>>> + * Chooses a default granularity based on the existing cluster size,
>>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that 
>>> there
>>> + * is no cluster size information available.
>>> + */
>>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
>>> +{
>>> +    BlockDriverInfo bdi;
>>> +    uint64_t granularity;
>>> +
>>> +    if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
>>> +        granularity = MAX(4096, bdi.cluster_size);
>>> +        granularity = MIN(65536, granularity);
>>> +    } else {
>>> +        granularity = 65536;
>>> +    }
>>> +
>>> +    return granularity;
>>> +}
>>> +
>>>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>>>                             BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>>>   {
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index d819952..fc545f1 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -667,15 +667,7 @@ static void mirror_start_job(BlockDriverState
>>> *bs, BlockDriverState *target,
>>>       MirrorBlockJob *s;
>>>       if (granularity == 0) {
>>> -        /* Choose the default granularity based on the target file's
>>> cluster
>>> -         * size, clamped between 4k and 64k.  */
>>> -        BlockDriverInfo bdi;
>>> -        if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 
>>> 0) {
>>> -            granularity = MAX(4096, bdi.cluster_size);
>>> -            granularity = MIN(65536, granularity);
>>> -        } else {
>>> -            granularity = 65536;
>>> -        }
>>> +        granularity = bdrv_get_default_bitmap_granularity(target);
>>>       }
>>>       assert ((granularity & (granularity - 1)) == 0);
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 5651a8e..95251c7 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1173,6 +1173,48 @@ out_aio_context:
>>>       return NULL;
>>>   }
>>> +/**
>>> + * Return a dirty bitmap (if present), after validating
>>> + * the node reference and bitmap names. Returns NULL on error,
>>> + * including when the BDS and/or bitmap is not found.
>>> + */
>>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char 
>>> *node_ref,
>>> +                                                  const char *name,
>>> + BlockDriverState
>>> **pbs,
>>> +                                                  Error **errp)
>>> +{
>>> +    BlockDriverState *bs;
>>> +    BdrvDirtyBitmap *bitmap;
>>> +
>>> +    if (!node_ref) {
>>> +        error_setg(errp, "Node reference cannot be NULL");
>>> +        return NULL;
>>> +    }
>>> +    if (!name) {
>>> +        error_setg(errp, "Bitmap name cannot be NULL");
>>> +        return NULL;
>>> +    }
>>> +
>>> +    bs = bdrv_lookup_bs(node_ref, node_ref, errp);
>>> +    if (!bs) {
>>> +        error_setg(errp, "Node reference '%s' not found", node_ref);
>>
>> No need to throw the (hopefully) perfectly fine Error code returned by
>> bdrv_lookup_bs() away.
>>
>
> I just wanted an error message consistent with the parameter name, in 
> this case. i.e., We couldn't find the "Node reference" instead of 
> "device" or "node name." Just trying to distinguish the fact that this 
> is an arbitrary reference in the error message.
>
> I can still remove it, but I am curious to see what Markus thinks of 
> the names I have chosen before I monkey with the errors too much more.

Very well then, but you should clean up the error returned by 
bdrv_lookup_bs() (call error_free()).

Feel free to keep my R-b whichever decision you'll make (as long as the 
error returned by bdrv_lookup_bs() is not leaked).

Max

>
>>> +        return NULL;
>>> +    }
>>> +
>>> +    /* If caller provided a BDS*, provide the result of that lookup,
>>> too. */
>>> +    if (pbs) {
>>> +        *pbs = bs;
>>> +    }
>>> +
>>> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
>>> +    if (!bitmap) {
>>> +        error_setg(errp, "Dirty bitmap not found: %s", name);
>>
>> I'd use "Dirty bitmap '%s' not found", because "foo: bar" in an error
>> message looks like "foo because bar" to me. But it's up to you, dirty
>> bitmap names most likely don't look like error reasons so nobody should
>> be able to confuse it.
>>
>
> No, I agree.
>
>> So with the error_setg() in the fail path for bdrv_lookup_bs() removed:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>