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

[John Snow]

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.

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