Re: [Qemu-devel] [PATCH v2 3/4] block: Catch duplicate IDs in bdrv_new()

[Fam Zheng]

On Thu, 04/17 17:48, Kevin Wolf wrote:
> Since commit f298d071, block devices added with blockdev-add don't have
> a QemuOpts around in dinfo->opts. Consequently, we can't rely any more
> on QemuOpts catching duplicate IDs for block devices.
> 
> This patch add a new check for duplicate IDs to bdrv_new(), and moves the

s/add/adds/

> existing check that the ID isn't already taken for a node-name there as
> well.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c                    | 11 +++++++++++
>  blockdev.c                 |  6 ------
>  tests/qemu-iotests/087     | 33 +++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/087.out | 13 +++++++++++++
>  4 files changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f3b93c9..fc2edd3 100644
> --- a/block.c
> +++ b/block.c
> @@ -336,6 +336,17 @@ BlockDriverState *bdrv_new(const char *device_name, 
> Error **errp)
>  {
>      BlockDriverState *bs;
>  
> +    if (bdrv_find(device_name)) {
> +        error_setg(errp, "Device with id '%s' already exists",
> +                   device_name);
> +        return NULL;
> +    }
> +    if (bdrv_find_node(device_name)) {
> +        error_setg(errp, "Device with node-name '%s' already exists",
> +                   device_name);
> +        return NULL;
> +    }
> +
>      bs = g_malloc0(sizeof(BlockDriverState));
>      QLIST_INIT(&bs->dirty_bitmaps);
>      pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
> diff --git a/blockdev.c b/blockdev.c
> index 3a11a62..09826f1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -452,12 +452,6 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>          }
>      }
>  
> -    if (bdrv_find_node(qemu_opts_id(opts))) {
> -        error_setg(errp, "device id=%s is conflicting with a node-name",
> -                   qemu_opts_id(opts));
> -        goto early_err;
> -    }
> -
>      /* init */
>      dinfo = g_malloc0(sizeof(*dinfo));
>      dinfo->id = g_strdup(qemu_opts_id(opts));
> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> index a38bb70..37d82fc 100755
> --- a/tests/qemu-iotests/087
> +++ b/tests/qemu-iotests/087
> @@ -73,6 +73,39 @@ run_qemu <<EOF
>  EOF
>  
>  echo
> +echo === Duplicate ID ===
> +echo
> +
> +run_qemu <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add",
> +  "arguments": {
> +      "options": {
> +        "driver": "$IMGFMT",
> +        "id": "disk",
> +        "file": {
> +            "driver": "file",
> +            "filename": "$TEST_IMG"
> +        }
> +      }
> +    }
> +  }
> +{ "execute": "blockdev-add",
> +  "arguments": {
> +      "options": {
> +        "driver": "$IMGFMT",
> +        "id": "disk",
> +        "file": {
> +            "driver": "file",
> +            "filename": "$TEST_IMG"
> +        }
> +      }
> +    }
> +  }
> +{ "execute": "quit" }
> +EOF
> +
> +echo
>  echo === aio=native without O_DIRECT ===
>  echo
>  
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> index e65dcdf..479bf86 100644
> --- a/tests/qemu-iotests/087.out
> +++ b/tests/qemu-iotests/087.out
> @@ -13,6 +13,19 @@ QMP_VERSION
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
>  
>  
> +=== Duplicate ID ===
> +
> +Testing:
> +QMP_VERSION
> +{"return": {}}
> +{"return": {}}
> +{"error": {"class": "GenericError", "desc": "Device with id 'disk' already 
> exists"}}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "SHUTDOWN"}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
> +
> +
>  === aio=native without O_DIRECT ===
>  
>  Testing:
> -- 
> 1.8.3.1
> 
>