Re: [Qemu-devel] [PATCH 9/9] block: Remove BlockDriverState.blk

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3135 bytes --]

On 22.03.2016 20:36, Kevin Wolf wrote:
> This patch removes the remaining users of bs->blk, which will allow us
> to have multiple BBs on top of a single BDS. All checks that are
> currently in place to prevent the user from creating such setups.

I think this sentence is missing a word or two.

> Future patches can allow them and e.g. enable users to mirror to a block
> device that already has a BlockBackend on it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    |  8 --------
>  block/block-backend.c      |  8 --------
>  block/mirror.c             |  4 ++--
>  blockdev.c                 | 19 ++++++++-----------
>  include/block/block_int.h  |  2 --
>  tests/qemu-iotests/085.out |  6 +++---
>  6 files changed, 13 insertions(+), 34 deletions(-)

[...]

> diff --git a/blockdev.c b/blockdev.c
> index c59cf3e..a658869 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1774,9 +1774,8 @@ static void external_snapshot_prepare(BlkActionState *common,
>          return;
>      }
>  
> -    if (state->new_bs->blk != NULL) {
> -        error_setg(errp, "The snapshot is already in use by %s",
> -                   blk_name(state->new_bs->blk));
> +    if (bdrv_has_blk(state->new_bs)) {
> +        error_setg(errp, "The snapshot is already in use");

You did add bdrv_get_parent_name(), so you could use it here...

>          return;
>      }
>  
> @@ -2492,9 +2491,8 @@ void qmp_x_blockdev_insert_medium(const char *device, const char *node_name,
>          return;
>      }
>  
> -    if (bs->blk) {
> -        error_setg(errp, "Node '%s' is already in use by '%s'", node_name,
> -                   blk_name(bs->blk));
> +    if (bdrv_has_blk(bs)) {
> +        error_setg(errp, "Node '%s' is already in use", node_name);

...and here...

>          return;
>      }
>  
> @@ -3439,7 +3437,7 @@ static void blockdev_mirror_common(BlockDriverState *bs,
>      if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
>          return;
>      }
> -    if (target->blk) {
> +    if (bdrv_has_blk(target)) {
>          error_setg(errp, "Cannot mirror to an attached block device");
>          return;
>      }
> @@ -4023,15 +4021,14 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
>          bs = blk_bs(blk);
>          aio_context = blk_get_aio_context(blk);
>      } else {
> +        blk = NULL;
>          bs = bdrv_find_node(node_name);
>          if (!bs) {
>              error_setg(errp, "Cannot find node %s", node_name);
>              return;
>          }
> -        blk = bs->blk;
> -        if (blk) {
> -            error_setg(errp, "Node %s is in use by %s",
> -                       node_name, blk_name(blk));
> +        if (bdrv_has_blk(bs)) {
> +            error_setg(errp, "Node %s is in use", node_name);

...and here.

If you consciously decide not to use it there, and after you have fixed
the commit message:

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

>              return;
>          }
>          aio_context = bdrv_get_aio_context(bs);

[...]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]