Re: [Qemu-devel] [PATCH v4 03/11] block: Accept node-name for blockdev-backup

[Kevin Wolf]

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 <address@hidden>
> > ---
> >  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

pgpsRUwTjdMYw.pgp
Description: PGP signature