Re: [Qemu-devel] [PATCH 01/23] block: Split bdrv_new_named() off bdrv_new()

["Benoît Canet" <benoit.canet@irqsave.net>]

The Wednesday 10 Sep 2014 à 10:13:30 (+0200), Markus Armbruster wrote :
> Creating an anonymous BDS can't fail.  Make that obvious.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c               | 26 +++++++++++++++++++-------
>  block/iscsi.c         |  2 +-
>  block/vvfat.c         |  2 +-
>  blockdev.c            |  2 +-
>  hw/block/xen_disk.c   |  2 +-
>  include/block/block.h |  3 ++-
>  qemu-img.c            |  6 +++---
>  qemu-io.c             |  2 +-
>  qemu-nbd.c            |  2 +-
>  9 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d06dd51..4b3bcd4 100644
> --- a/block.c
> +++ b/block.c
> @@ -335,10 +335,11 @@ void bdrv_register(BlockDriver *bdrv)
>  }
>  
>  /* create a new block device (by default it is empty) */
> -BlockDriverState *bdrv_new(const char *device_name, Error **errp)
> +BlockDriverState *bdrv_new_named(const char *device_name, Error **errp)
>  {
>      BlockDriverState *bs;
> -    int i;
> +
> +    assert(*device_name);

This assert that device_name is not a null pointer.
But here we are pretty sure that the BDS should be named given the name of the function.
Should we bake an assert on device_name[0] != '\0' to avoid bdrv_new_named being called
with "" as device_name ?

>  
>      if (bdrv_find(device_name)) {
>          error_setg(errp, "Device with id '%s' already exists",
> @@ -351,12 +352,23 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp)
>          return NULL;
>      }
>  
> -    bs = g_new0(BlockDriverState, 1);
> -    QLIST_INIT(&bs->dirty_bitmaps);
> +    bs = bdrv_new();
> +
>      pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);

>      if (device_name[0] != '\0') {
Then we could remove this test.

>          QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
Because here would be too late anyway: an unammed BDS would have been created if device_name == "".


>      }
> +
> +    return bs;
> +}
> +
> +BlockDriverState *bdrv_new(void)
> +{
> +    BlockDriverState *bs;
> +    int i;
> +
> +    bs = g_new0(BlockDriverState, 1);
> +    QLIST_INIT(&bs->dirty_bitmaps);
>      for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
>          QLIST_INIT(&bs->op_blockers[i]);
>      }
> @@ -1217,7 +1229,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>          goto free_exit;
>      }
>  
> -    backing_hd = bdrv_new("", errp);
> +    backing_hd = bdrv_new();
>  
>      if (bs->backing_format[0] != '\0') {
>          back_drv = bdrv_find_format(bs->backing_format);
> @@ -1346,7 +1358,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>      qdict_put(snapshot_options, "file.filename",
>                qstring_from_str(tmp_filename));
>  
> -    bs_snapshot = bdrv_new("", &error_abort);
> +    bs_snapshot = bdrv_new();
>  
>      ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
>                      flags, bdrv_qcow2, &local_err);
> @@ -1417,7 +1429,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      if (*pbs) {
>          bs = *pbs;
>      } else {
> -        bs = bdrv_new("", &error_abort);
> +        bs = bdrv_new();
>      }
>  
>      /* NULL means an empty set of options */
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 3e19202..af3d0f6 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1528,7 +1528,7 @@ static int iscsi_create(const char *filename, QemuOpts *opts, Error **errp)
>      IscsiLun *iscsilun = NULL;
>      QDict *bs_options;
>  
> -    bs = bdrv_new("", &error_abort);
> +    bs = bdrv_new();
>  
>      /* Read out options */
>      total_size =
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 731e591..6c9fde0 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2939,7 +2939,7 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
>      unlink(s->qcow_filename);
>  #endif
>  
> -    bdrv_set_backing_hd(s->bs, bdrv_new("", &error_abort));
> +    bdrv_set_backing_hd(s->bs, bdrv_new());
>      s->bs->backing_hd->drv = &vvfat_write_target;
>      s->bs->backing_hd->opaque = g_new(void *, 1);
>      *(void**)s->bs->backing_hd->opaque = s;
> diff --git a/blockdev.c b/blockdev.c
> index e919566..9fbd888 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -458,7 +458,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      /* init */
>      dinfo = g_malloc0(sizeof(*dinfo));
>      dinfo->id = g_strdup(qemu_opts_id(opts));
> -    dinfo->bdrv = bdrv_new(dinfo->id, &error);
> +    dinfo->bdrv = bdrv_new_named(dinfo->id, &error);
>      if (error) {
>          error_propagate(errp, error);
>          goto bdrv_new_err;
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 2dcef07..8bac7ff 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -856,7 +856,7 @@ static int blk_connect(struct XenDevice *xendev)
>  
>          /* setup via xenbus -> create new block driver instance */
>          xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
> -        blkdev->bs = bdrv_new(blkdev->dev, NULL);
> +        blkdev->bs = bdrv_new_named(blkdev->dev, NULL);
>          if (!blkdev->bs) {
>              return -1;
>          }
> diff --git a/include/block/block.h b/include/block/block.h
> index 8f4ad16..95139c0 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -203,7 +203,8 @@ BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>  int bdrv_create(BlockDriver *drv, const char* filename,
>                  QemuOpts *opts, Error **errp);
>  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
> -BlockDriverState *bdrv_new(const char *device_name, Error **errp);
> +BlockDriverState *bdrv_new_named(const char *device_name, Error **errp);
> +BlockDriverState *bdrv_new(void);
>  void bdrv_make_anon(BlockDriverState *bs);
>  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
>  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
> diff --git a/qemu-img.c b/qemu-img.c
> index 91d1ac3..4490a22 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -296,7 +296,7 @@ static BlockDriverState *bdrv_new_open(const char *id,
>      Error *local_err = NULL;
>      int ret;
>  
> -    bs = bdrv_new(id, &error_abort);
> +    bs = bdrv_new_named(id, &error_abort);
>  
>      if (fmt) {
>          drv = bdrv_find_format(fmt);
> @@ -2425,7 +2425,7 @@ static int img_rebase(int argc, char **argv)
>      if (!unsafe) {
>          char backing_name[1024];
>  
> -        bs_old_backing = bdrv_new("old_backing", &error_abort);
> +        bs_old_backing = bdrv_new_named("old_backing", &error_abort);
>          bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>          ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, src_flags,
>                          old_backing_drv, &local_err);
> @@ -2436,7 +2436,7 @@ static int img_rebase(int argc, char **argv)
>              goto out;
>          }
>          if (out_baseimg[0]) {
> -            bs_new_backing = bdrv_new("new_backing", &error_abort);
> +            bs_new_backing = bdrv_new_named("new_backing", &error_abort);
>              ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL, src_flags,
>                              new_backing_drv, &local_err);
>              if (ret) {
> diff --git a/qemu-io.c b/qemu-io.c
> index d2ab694..44c2e1c 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -58,7 +58,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>          return 1;
>      }
>  
> -    qemuio_bs = bdrv_new("hda", &error_abort);
> +    qemuio_bs = bdrv_new_named("hda", &error_abort);
>  
>      if (growable) {
>          flags |= BDRV_O_PROTOCOL;
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 9bc152e..a56ebfc 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -687,7 +687,7 @@ int main(int argc, char **argv)
>          drv = NULL;
>      }
>  
> -    bs = bdrv_new("hda", &error_abort);
> +    bs = bdrv_new_named("hda", &error_abort);
>  
>      srcpath = argv[optind];
>      ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
> -- 
> 1.9.3
> 
>