Re: [Qemu-devel] [PATCH v9 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
>> diff --git a/block.c b/block.c
>> index e5c6ccf..3f27519 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, 
>> BdrvDirtyBitmap *bitmap, int64_t sector
>>      }
>>  }
>>  
>> +#define BDB_MIN_DEF_GRANULARITY 4096
>> +#define BDB_MAX_DEF_GRANULARITY 65536
>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>> +
>> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>
> Long names are unwieldy but introducing multiple abbreviations is not a
> good solution, it makes the code more confusing (BDB vs dbm).
>
> I would call the function bdrv_get_default_bitmap_granularity().
>
> The constants weren't necessary since the point of this function is to
> capture the default value.  No one else should use the constants -
> otherwise there is a high probability that they are reimplementing this
> logic.  I would just leave them as literals in the code.
>
>> diff --git a/blockdev.c b/blockdev.c
>> index 5651a8e..4d30b09 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1894,6 +1894,60 @@ void qmp_block_set_io_throttle(const char *device, 
>> int64_t bps, int64_t bps_rd,
>>      aio_context_release(aio_context);
>>  }
>>  
>> +void qmp_block_dirty_bitmap_add(const char *device, const char *name,
>> +                                bool has_granularity, int64_t granularity,
>> +                                Error **errp)
>> +{
>> +    BlockDriverState *bs;
>> +
>> +    bs = bdrv_lookup_bs(device, NULL, errp);
>
> Markus: I think we need to support node-name here so dirty bitmaps can
> be applied at any node in the graph?

We need to consider node names for all new QMP commands.  Whenever we
add a backend name parameter, like we do for the two new commands in
this patch, we need to decide whether nodes make sense, too.  Do they?

I'm afraid we haven't quite made up our mind what to do when nodes make
sense.

Existing patterns of backend / node name parameters:

1. Backend name only

   Parameter is commonly called @device.  Examples: eject,
   block_set_io_throttle.

   Code uses blk_by_name() or bdrv_find().  The latter needs to be
   converted to the former.

2. Backend or node name

2a. Two optional parameters, commonly called @device and @node-name,
   of which exactly one must be given.  Example: block_passwd.

   Code uses

        bs = bdrv_lookup_bs(has_device ? device : NULL,
                            has_node_name ? node_name : NULL,
                                &local_err);

   which is a roundabout way to say

        bs = bdrv_lookup_bs(device, node_name, &local_err);

2b. Single parameter.  The single example is anonymous union
   BlockdevRef.

   Code uses

        bs = bdrv_lookup_bs(reference, reference, errp);

   If we want to adopt "single parameter" going forward, we need to
   decide on a naming convention.  Existing commands are probably stuck
   with @device for compatibility.  Do we care for names enough to
   improve on that?

   A convenience wrapper around bdrv_lookup_bs() to avoid stuttering
   name argument could make sense.

3. Node name only

   No known example.

4. Backend and node name

   The backend parameter is commonly called @device.  The node parameter
   name varies.  Example: blockdev-snapshot-sync's @snapshot-node-name,
   change-backing-file's @image-node-name.

   Code uses either

        bdrv_find_node(node_name)

   or

        bdrv_lookup_bs(NULL, node_name, &local_err).
 
This patch invents yet another code pattern:

        bdrv_lookup_bs(device, NULL, errp);

I take this as a sign that something's amiss, either with the existing
patterns or with the new one :)

[...]