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

[John Snow <jsnow@redhat.com>]

On 01/16/2015 11:51 AM, Max Reitz wrote:
> 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
>

Or a new helper designed specifically for single argument "BB -or- BDS" 
lookups, too, that uses the name of the parameter that we eventually 
decide upon.

I'll clean up the leak for now.

Thanks!

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

-- 
—js