[v2,01/23] block: Split bdrv_new_root() off bdrv_new()

Creating an anonymous BDS can't fail.  Make that obvious.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c               | 28 +++++++++++++++++++---------
 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(+), 19 deletions(-)

On 13.09.2014 17:00, Markus Armbruster wrote:
> Creating an anonymous BDS can't fail.  Make that obvious.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block.c               | 28 +++++++++++++++++++---------
>   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(+), 19 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

On Sat, Sep 13, 2014 at 05:00:05PM +0200, Markus Armbruster wrote:
> Creating an anonymous BDS can't fail.  Make that obvious.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c               | 28 +++++++++++++++++++---------
>  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(+), 19 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 02ea90f..4fe3b62 100644
> --- a/block.c
> +++ b/block.c
> @@ -336,10 +336,11 @@ void bdrv_register(BlockDriver *bdrv)
>  }
>  

>  /* create a new block device (by default it is empty) */
I wonder if this comment is stale.
What is the emptiness of a block device anyway ?

> -BlockDriverState *bdrv_new(const char *device_name, Error **errp)
> +BlockDriverState *bdrv_new_root(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",
> @@ -353,12 +354,21 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp)
>          return NULL;
>      }
>  
> +    bs = bdrv_new();
> +
> +    pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
> +    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);
> -    pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
> -    if (device_name[0] != '\0') {
> -        QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
> -    }
>      for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
>          QLIST_INIT(&bs->op_blockers[i]);
>      }
> @@ -1219,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);
> @@ -1348,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);
> @@ -1419,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 450f95c..c9463e3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -463,7 +463,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      }
>  
>      /* init */
> -    bs = bdrv_new(qemu_opts_id(opts), errp);
> +    bs = bdrv_new_root(qemu_opts_id(opts), errp);
>      if (!bs) {
>          goto early_err;
>      }
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 2dcef07..71f0953 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_root(blkdev->dev, NULL);
>          if (!blkdev->bs) {
>              return -1;
>          }
> diff --git a/include/block/block.h b/include/block/block.h
> index 8f4ad16..6cabc98 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_root(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..58d59d0 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_root(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_root("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_root("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..24ca59c 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_root("hda", &error_abort);
>  
>      if (growable) {
>          flags |= BDRV_O_PROTOCOL;
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index f3cf8a2..24b8454 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_root("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 <benoit.canet@nodalink.com> writes:

> On Sat, Sep 13, 2014 at 05:00:05PM +0200, Markus Armbruster wrote:
>> Creating an anonymous BDS can't fail.  Make that obvious.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block.c               | 28 +++++++++++++++++++---------
>>  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(+), 19 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 02ea90f..4fe3b62 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -336,10 +336,11 @@ void bdrv_register(BlockDriver *bdrv)
>>  }
>>  
>
>>  /* create a new block device (by default it is empty) */
> I wonder if this comment is stale.
> What is the emptiness of a block device anyway ?

No medium.

To make an empty BDS non-empty, bdrv_open() an image.  To make it empty
again, bdrv_close() it.

The comment still applies.

In the BB world, there's no such thing as an empty BDS.  A BB without
medium simply has no BDS.  This series doesn't get us there, it's just a
first step.  When we get there, bdrv_new() will be folded into
bdrv_open(), and bdrv_close() into bdrv_delete().

[...]
> Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Thanks!