Re: [Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Fam Zheng]

On Thu, 06/27 10:15, Stefan Hajnoczi wrote:
> On Wed, Jun 26, 2013 at 11:59:19AM +0800, Fam Zheng wrote:
> > Add target-id (optional) to drive-backup command, to make the target bs
> > a named drive so that we can operate on it (e.g. export with NBD).
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  blockdev.c       | 4 +++-
> >  qapi-schema.json | 7 +++++--
> >  qmp-commands.hx  | 3 ++-
> >  3 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index b3a57e0..5e694f3 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -935,6 +935,7 @@ static void drive_backup_prepare(BlkTransactionState 
> > *common, Error **errp)
> >      backup = common->action->drive_backup;
> >  
> >      qmp_drive_backup(backup->device, backup->target,
> > +                     backup->has_target_id, backup->target_id,
> >                       backup->has_format, backup->format,
> >                       backup->has_mode, backup->mode,
> >                       backup->has_speed, backup->speed,
> > @@ -1420,6 +1421,7 @@ void qmp_block_commit(const char *device,
> >  }
> >  
> >  void qmp_drive_backup(const char *device, const char *target,
> > +                      bool has_target_id, const char *target_id,
> >                        bool has_format, const char *format,
> >                        bool has_mode, enum NewImageMode mode,
> >                        bool has_speed, int64_t speed,
> > @@ -1494,7 +1496,7 @@ void qmp_drive_backup(const char *device, const char 
> > *target,
> >          return;
> >      }
> >  
> > -    target_bs = bdrv_new("");
> > +    target_bs = bdrv_new(has_target_id ? target_id : "");
> 
> This raises a new issue:
> 
> Now that the target can be named, what happens when the user issues a
> monitor command, e.g. drive-del, block-resize, or drive-backup :)?
> 
> We have a clumsy form of protection with bdrv_set_in_use().  It makes
> several monitor commands refuse with -EBUSY.
> 
> Perhaps we should have a command permission set so it's possible to
> allow/deny specific commands.
> 

Yes, this makes me realize that ref count it not a solution to retire
bs->in_use, because we can't tell if drive-del or block-resize is safe
with only reference number. But I can't think of two situations to deny
different subsets of commands, shouldn't a general blocker, like in_use
does, be good enough?

-- 
Fam