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