[v4,03/11] block: Accept node-name for blockdev-backup

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This converts
blockdev-backup and the corresponding transaction action to accept a
node-name without lifting the restriction that we're operating at a root
node.

In case of an invalid device name, the command returns the GenericError
error class now instead of DeviceNotFound, because this is what
qmp_get_root_bs() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 35 ++++++++++-------------------------
 qapi/block-core.json |  2 +-
 qmp-commands.hx      |  2 +-
 3 files changed, 12 insertions(+), 27 deletions(-)

On 07/14/2016 07:28 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> blockdev-backup and the corresponding transaction action to accept a
> node-name without lifting the restriction that we're operating at a root
> node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 35 ++++++++++-------------------------
>  qapi/block-core.json |  2 +-
>  qmp-commands.hx      |  2 +-
>  3 files changed, 12 insertions(+), 27 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

On 14.07.2016 15:28, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> blockdev-backup and the corresponding transaction action to accept a
> node-name without lifting the restriction that we're operating at a root
> node.
> 
> In case of an invalid device name, the command returns the GenericError
> error class now instead of DeviceNotFound, because this is what
> qmp_get_root_bs() returns.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 35 ++++++++++-------------------------
>  qapi/block-core.json |  2 +-
>  qmp-commands.hx      |  2 +-
>  3 files changed, 12 insertions(+), 27 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 22bf03d..e5a5642 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1954,38 +1954,31 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
>  {
>      BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
>      BlockdevBackup *backup;
> -    BlockBackend *blk, *target;
> +    BlockDriverState *bs, *target;
>      Error *local_err = NULL;
>  
>      assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
>      backup = common->action->u.blockdev_backup.data;
>  
> -    blk = blk_by_name(backup->device);
> -    if (!blk) {
> -        error_setg(errp, "Device '%s' not found", backup->device);
> -        return;
> -    }
> -
> -    if (!blk_is_available(blk)) {
> -        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device);
> +    bs = qmp_get_root_bs(backup->device, errp);
> +    if (!bs) {
>          return;
>      }
>  
> -    target = blk_by_name(backup->target);
> +    target = qmp_get_root_bs(backup->target, errp);

As of recently (0d978913), do_blockdev_backup() uses bdrv_lookup_bs()
for the target. While I think it was a mistake for that commit not to
change this place to bdrv_lookup_bs() as well, I think this patch should
either not touch this place at all, or it should fix it by using
bdrv_lookup_bs().

Max

>      if (!target) {
> -        error_setg(errp, "Device '%s' not found", backup->target);
>          return;
>      }
>  
>      /* AioContext is released in .clean() */
> -    state->aio_context = blk_get_aio_context(blk);
> -    if (state->aio_context != blk_get_aio_context(target)) {
> +    state->aio_context = bdrv_get_aio_context(bs);
> +    if (state->aio_context != bdrv_get_aio_context(target)) {
>          state->aio_context = NULL;
>          error_setg(errp, "Backup between two IO threads is not implemented");
>          return;
>      }
>      aio_context_acquire(state->aio_context);
> -    state->bs = blk_bs(blk);
> +    state->bs = bs;
>      bdrv_drained_begin(state->bs);
>  
>      do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL,
> @@ -3361,7 +3354,6 @@ void do_blockdev_backup(const char *job_id, const char *device,
>                           BlockdevOnError on_target_error,
>                           BlockJobTxn *txn, Error **errp)
>  {
> -    BlockBackend *blk;
>      BlockDriverState *bs;
>      BlockDriverState *target_bs;
>      Error *local_err = NULL;
> @@ -3377,21 +3369,14 @@ void do_blockdev_backup(const char *job_id, const char *device,
>          on_target_error = BLOCKDEV_ON_ERROR_REPORT;
>      }
>  
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        error_setg(errp, "Device '%s' not found", device);
> +    bs = qmp_get_root_bs(device, errp);
> +    if (!bs) {
>          return;
>      }
>  
> -    aio_context = blk_get_aio_context(blk);
> +    aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> -    if (!blk_is_available(blk)) {
> -        error_setg(errp, "Device '%s' has no medium", device);
> -        goto out;
> -    }
> -    bs = blk_bs(blk);
> -
>      target_bs = bdrv_lookup_bs(target, target, errp);
>      if (!target_bs) {
>          goto out;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 61f8fd7..728b5f7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -921,7 +921,7 @@
>  # @job-id: #optional identifier for the newly-created block job. If
>  #          omitted, the device name will be used. (Since 2.7)
>  #
> -# @device: the name of the device which should be copied.
> +# @device: the device name or node-name of a root node which should be copied.
>  #
>  # @target: the name of the backup target device.
>  #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 047b6f1..e7608f2 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1288,7 +1288,7 @@ Arguments:
>  
>  - "job-id": Identifier for the newly-created block job. If omitted,
>              the device name will be used. (json-string, optional)
> -- "device": the name of the device which should be copied.
> +- "device": the device name or node-name of a root node which should be copied.
>              (json-string)
>  - "target": the name of the backup target device. (json-string)
>  - "sync": what parts of the disk image should be copied to the destination;
>

Am 18.07.2016 um 15:59 hat Max Reitz geschrieben:
> On 14.07.2016 15:28, Kevin Wolf wrote:
> > In order to remove the necessity to use BlockBackend names in the
> > external API, we want to allow node-names everywhere. This converts
> > blockdev-backup and the corresponding transaction action to accept a
> > node-name without lifting the restriction that we're operating at a root
> > node.
> > 
> > In case of an invalid device name, the command returns the GenericError
> > error class now instead of DeviceNotFound, because this is what
> > qmp_get_root_bs() returns.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c           | 35 ++++++++++-------------------------
> >  qapi/block-core.json |  2 +-
> >  qmp-commands.hx      |  2 +-
> >  3 files changed, 12 insertions(+), 27 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 22bf03d..e5a5642 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1954,38 +1954,31 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
> >  {
> >      BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
> >      BlockdevBackup *backup;
> > -    BlockBackend *blk, *target;
> > +    BlockDriverState *bs, *target;
> >      Error *local_err = NULL;
> >  
> >      assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
> >      backup = common->action->u.blockdev_backup.data;
> >  
> > -    blk = blk_by_name(backup->device);
> > -    if (!blk) {
> > -        error_setg(errp, "Device '%s' not found", backup->device);
> > -        return;
> > -    }
> > -
> > -    if (!blk_is_available(blk)) {
> > -        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device);
> > +    bs = qmp_get_root_bs(backup->device, errp);
> > +    if (!bs) {
> >          return;
> >      }
> >  
> > -    target = blk_by_name(backup->target);
> > +    target = qmp_get_root_bs(backup->target, errp);
> 
> As of recently (0d978913), do_blockdev_backup() uses bdrv_lookup_bs()
> for the target. While I think it was a mistake for that commit not to
> change this place to bdrv_lookup_bs() as well, I think this patch should
> either not touch this place at all, or it should fix it by using
> bdrv_lookup_bs().

I'll first fix target in a separate patch and also update the
documentation which 0d978913 neglected to do, and then I'll leave target
alone in this patch.

Kevin