diff mbox

[01/23] block: Split bdrv_new_named() off bdrv_new()

Message ID 1410336832-22160-2-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Sept. 10, 2014, 8:13 a.m. UTC
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(-)

Comments

Benoît Canet Sept. 10, 2014, 11:03 a.m. UTC | #1
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
> 
>
Eric Blake Sept. 10, 2014, 3:05 p.m. UTC | #2
On 09/10/2014 05:03 AM, Benoît Canet wrote:
> 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>
>> ---

>>  /* 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.

No, it assumes that device_name is non-NULL and blindly dereferences it;
it is asserting that the first byte of the dereferenced pointer is not NUL.

> 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 ?

That's already what the code does.  About the only thing it could do
differently is:

assert(device_name && *device_name);
Eric Blake Sept. 10, 2014, 3:07 p.m. UTC | #3
On 09/10/2014 02:13 AM, 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Benoît Canet Sept. 10, 2014, 3:27 p.m. UTC | #4
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);
>  
>      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') {
>          QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
>      }
> +
> +    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
> 
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
Benoît Canet Sept. 10, 2014, 9:22 p.m. UTC | #5
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);
>  
>      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') {

If finally understood why I said crap about the above assert.
If *device_name is true then device_name[0] != '\0' is true.
I diagnosed it in reverse.

I think we can remove the if up here the code will be simpler.

>          QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
>      }
> +
> +    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
>
Fam Zheng Sept. 11, 2014, 6:33 a.m. UTC | #6
On Wed, 09/10 10:13, 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(-)

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> 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);
>  
>      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') {
>          QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
>      }
> +
> +    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
>
Markus Armbruster Sept. 11, 2014, 8:20 a.m. UTC | #7
Eric Blake <eblake@redhat.com> writes:

> On 09/10/2014 05:03 AM, Benoît Canet wrote:
>> 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>
>>> ---
>
>>>  /* 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.
>
> No, it assumes that device_name is non-NULL and blindly dereferences it;
> it is asserting that the first byte of the dereferenced pointer is not NUL.
>
>> 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 ?
>
> That's already what the code does.  About the only thing it could do
> differently is:
>
> assert(device_name && *device_name);

Converts a crash into an assertion failure.  No difference to me, but if
you guys think it's a worthwhile improvement...
Markus Armbruster Sept. 11, 2014, 8:29 a.m. UTC | #8
Benoît Canet <benoit.canet@irqsave.net> writes:

> 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 == "".

Short answer: not worth it, the function goes away in PATCH 08, and its
replacement works like you suggest.

Slightly longer answer: PATCH 02 adds BlockBackend, which also checks
device names.  blk_new() does it the way you suggest.  PATCH 08 removes
the now redundant BlockDriverState device name, including this function.

[...]
diff mbox

Patch

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);
 
     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') {
         QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
     }
+
+    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);