Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command

[Kevin Wolf]

Am 14.02.2014 um 20:17 hat Ian Main geschrieben:
> On Thu, Feb 13, 2014 at 09:59:40AM +0100, Kevin Wolf wrote:
> > Am 12.02.2014 um 18:36 hat Ian Main geschrieben:
> > > This is the sister command to blockdev-add.  In Fam's example he uses
> > > the drive_del HMP command to clean up but it would be much nicer to
> > > have a way to do this via QMP.
> > > 
> > > Signed-off-by: Ian Main <address@hidden>
> > 
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index d22651c..01186cd 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -4469,3 +4469,14 @@
> > >  # Since: 1.7
> > >  ##
> > >  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> > > +
> > > +##
> > > +# @blockdev-del:
> > > +#
> > > +# Delete a block device.
> > > +#
> > > +# @device: Identifier for the block device to be deleted.
> > > +#
> > > +# Since: 2.0
> > > +##
> > > +{ 'command': 'blockdev-del', 'data': { 'device': 'str' } }
> > 
> > I believe the full documentation should go here as well, not just in
> > qmp-commands.hx.
> > 
> > > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > > index c3ee46a..f08045d 100644
> > > --- a/qmp-commands.hx
> > > +++ b/qmp-commands.hx
> > > @@ -3442,6 +3442,36 @@ Example (2):
> > >  EQMP
> > >  
> > >      {
> > > +        .name       = "blockdev-del",
> > > +        .args_type  = "device:s",
> > > +        .mhandler.cmd_new = qmp_marshal_input_blockdev_del,
> > > +    },
> > > +
> > > +SQMP
> > > +blockdev-del
> > > +------------
> > > +
> > > +Remove host block device.  The result is that guest generated IO is no
> > > +longer submitted against the host device underlying the disk.  Once a
> > > +drive has been deleted, the QEMU Block layer returns -EIO which results
> > > +in IO errors in the guest for applications that are reading/writing to
> > > +the device.  These errors are always reported to the guest, regardless
> > > +of the drive's error actions (drive options rerror, werror).
> > 
> > I think we wanted to have different semantics for blockdev-del.
> > Specifically, it is a backend command that should have no effect on
> > users of that backend.
> > 
> > Let me paste and comment on some notes I made in a previous blockdev
> > discussion:
> > 
> >     * Make sure that an explicit blockdev-del is needed to remove the
> >       BDS; it shouldn't happen automagically just because the guest
> >       device was unplugged
> >       [done]
> > 
> >     * By default, return an error for blockdev-del if reference count > 1
> >       [ The assumption is here that one reference is held by the
> >         monitor/external user, which isn't true today to my knowledge ]
> > 
> >       - But have a force option that closes the image file, even if it
> >         breaks the remaining users (e.g. uncooperative guest that doesn't
> >         release its PCI device)
> >         [ Here we need working refcounting including the external user,
> >           because otherwise we don't free the (closed) BDS even when the
> >           guest device is unplugged. It is an open question whether and
> >           how BDSes without an external reference are shown in the
> >           monitor. ]
> > 
> >     * Prevent mixing blockdev-add with drive_del and vice versa
> > 
> >       - Ideally drive_add BDSes are exactly those with a DriveInfo
> >         [ not true today, but DriveInfo.enable_auto_del can be used to
> >           distinguish them ]
> > 
> > So I believe we still have some design work to do before we can actually
> > implement this. I would prefer not to merge this for 2.0.
> >
> > Kevin
> 
> Are we only changing the semantics/implementation of the API, or is the
> API itself going to change with these improvements?  If that were the
> case wouldn't it make some sense to get people using blockdev-del now
> and update the semantics later?  As it is now consumers will just end
> up using blockdev-add/drive_del.

It should be obvious that you can't change the semantics after the fact.
The API is not just a command name and a set of arguments, but clearly
also what happens when you invoke the command.

Doing one thing now and changing it later to behave differently will
break any management tool because it wouldn't be able to know what the
command means for the specific qemu version it's talking to.

Kevin