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/19/2015 05:08 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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.
>
> bdrv_lookup_bs() is an awkward interface.
>
> If @device is non-null, try to look up a backend (BB) named @device.  If
> it exists, return the backend's root node (BDS).
>
> Else if @node_name is non-null, try to look up a node (BDS) named
> @node_name.  If it exists, return it.
>
> Else, set this error:
>
>      error_setg(errp, "Cannot find device=%s nor node_name=%s",
>                       device ? device : "",
>                       node_name ? node_name : "");
>
> The error message is crap unless both device and node_name are non-null
> and different.  Which is never the case: we always either pass two
> identical non-null arguments, or we pass a null and a non-null
> argument[*].  In other words, the error message is always crap.
>
> In case you wonder why @device takes precedence over node_name when both
> are given: makes no sense.  But when both are given, they are always
> identical, and since backend and node names share a name space, only one
> can resolve.
>
> A couple of cleaner solutions come to mind:
>
> * Make bdrv_lookup_bs() suck less
>
>    Assert its tacit preconditions:
>
>      assert(device || node_name);
>      assert(!device || !node_name || device == node_name);
>
>    Then make it produce a decent error:
>
>      if (device && node_name) {
>          error_setg(errp, "Neither block backend nor node %s found", device);
>      else if (device) {
>          error_setg(errp, "Block backend %s not found", device);
>      } else if (node_name) {
>          error_setg(errp, "Block node %s not found", node_name);
>      }
>
>    Note how the three cases mirror the three usage patterns.
>
>    Further note that the proposed error messages deviate from the
>    existing practice of calling block backends "devices".  Calling
>    everything and its dog a "device" is traditional, but it's also lazy
>    and confusing.  End of digression.
>
> * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its
>    callers
>
>    Drop the Error ** parameter.  Callers know whether a failed lookup was
>    for a device name, a node name or both, and can set an appropriate
>    error themselves.
>
>    I'd still assert the preconditions.
>
> * Replace the function by one for each of its usage patterns
>
>    I think that's what I'd do.
>
> [...]
>
>
> [*] See
> https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html
>

I can submit a patch for making bdrv_lookup_bs "nicer," or at least its 
usage more clear, in a separate patch.

If you really want me to fold it into this series, I'd invite you to 
review explicitly my usage of the parameter "node-ref" before I embark 
on cleaning up other interface areas.

Does this naming scheme look sane to you, and fit with your general 
expectations?

I can also add a "bdrv_lookup_noderef" function that takes only one 
argument, which will help enforce the "If both arguments are provided, 
they must be the same" paradigm.

This patch (#3) covers my shot at a unified parameter, and you can see 
further consequences in #7, and #10 (transactions).

CC'ing Eric Blake, as well, for comments on a "unified parameter" 
interface in general.

Thanks,
--js