Re: [Qemu-devel] [PATCH 2/9] qapi: Add optional field "name" to block dirty bitmap

[Stefan Hajnoczi]

On Mon, Jan 13, 2014 at 06:39:41PM +0800, Fam Zheng wrote:
> diff --git a/block.c b/block.c
> index 6ad0368..16ef61b 100644
> --- a/block.c
> +++ b/block.c
> @@ -52,6 +52,7 @@
>  struct BdrvDirtyBitmap {
>      HBitmap *bitmap;
>      int refcnt;
> +    char name[1024];

Is there a reason for a fixed-size buffer?  g_strdup() is nicer than
arbitrary limits, especially since the lifecycle of BdrvDirtyBitmap is
well-defined and you just need to add a g_free().

> +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> +                                          int granularity,
> +                                          const char *name,
> +                                          Error **errp)
>  {
>      int64_t bitmap_size;
>      BdrvDirtyBitmap *bitmap;
>  
>      assert((granularity & (granularity - 1)) == 0);
>  
> +    if (name && bdrv_find_dirty_bitmap(bs, name)) {
> +        if (errp) {
> +            error_setg(errp, "Bitmap already exists: %s", name);
> +        }

error_setg() already checks that errp != NULL, you can drop the if
statement.

> diff --git a/block/mirror.c b/block/mirror.c
> index 2932bab..cc0665b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -598,7 +598,7 @@ static void mirror_start_job(BlockDriverState *bs, 
> BlockDriverState *target,
>      s->granularity = granularity;
>      s->buf_size = MAX(buf_size, granularity);
>  
> -    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity);
> +    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);

No error return means the coroutine will be started even if creating the
dirty bitmap fails.  I didn't check what happens but this definitely
makes the reader wonder if the error will be handled cleanly.

> diff --git a/qapi-schema.json b/qapi-schema.json
> index b9b051c..e91143a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -920,6 +920,8 @@
>  #
>  # Block dirty bitmap information.
>  #
> +# @name: the name of dirty bitmap

Since 2.0