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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 01/19/2015 05:08 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>>
>>> On 01/16/2015 10:36 AM, Max Reitz wrote:
>>>> On 2015-01-12 at 11:30, John Snow wrote:
>>>>> From: Fam Zheng <address@hidden>
>>>>>
>>>>> 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 <address@hidden>
>>>>> Signed-off-by: John Snow <address@hidden>
>>>>> ---
>>>>>    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.

Yes, please.

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

Follow-up patch is fine.  Adding one more bad error message along the
way before you fix them all doesn't bother me.

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

Do we want to introduce a @node-ref naming convention?

Currently, QMP calls parameters or members naming nodes @node-name or
similar, and parameters/members naming backends @device or similar.  The
one place where we already accept either is called @reference in the
schema, but it's a member of an anonymous union, so it's not really
visible in QMP.

Previously[*], we agreed (I think) to replace and deprecate the four
commands that use the "pair of names" convention to identify a node.
Their replacement would use the "single name" convention.  The name can
either be a node name or a backend name, and the latter automatically
resolves to its root node.

The "backend name resolves to its root node" convenience feature should
be available consistently or not at all.  I think the consensus it to
want it consistently.

Therefore, your new @node-ref is really the same as the existing
@node-name, isn't it?

Why a new naming convention @node-ref?  Is it meant to be in addition to
@node-name, or is it meant to replace it?

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

Good move.


[*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html