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

[Ian Main]

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.

        Ian