[libvirt] [PATCH] qemu: call drive_unplug in DetachPciDiskDevice

[Ryan Harper]

* Daniel P. Berrange <berrange at redhat.com> [2010-10-21 11:56]:
> On Thu, Oct 21, 2010 at 11:48:33AM -0500, Anthony Liguori wrote:
> > On 10/21/2010 11:45 AM, Daniel P. Berrange wrote:
> > >On Thu, Oct 21, 2010 at 08:50:35AM -0500, Ryan Harper wrote:
> > >   
> > >>Currently libvirt doesn't confirm whether the guest has responded to the
> > >>disk removal request.  In some cases this can leave the guest with
> > >>continued access to the device while the mgmt layer believes that it has
> > >>been removed.  With a recent qemu monitor command[1] we can
> > >>deterministically revoke a guests access to the disk (on the QEMU side)
> > >>to ensure no futher access is permitted.
> > >>
> > >>This patch adds support for the drive_unplug() command and introduces it
> > >>in the disk removal paths.  There is some discussion to be had about how
> > >>to handle the case where the guest is running in a QEMU without this
> > >>command (and the fact that we currently don't have a way of detecting
> > >>what monitor commands are available).
> > >>     
> > >Basically we try to run the command and then catch the failure.
> > >
> > >For QMP, we can check for a error with a class of 'CommandNotFound',
> > >
> > >For HMP, QEMU will print 'unknown command' in the reply.
> > >
> > >Neither is ideal, since neither is a guarenteed part of the monitor
> > >interface, but it is all we have to go on, and ensure other critical
> > >errors will still be treated as fatal by libvirt.
> > >
> > >   
> > >>My current implementation assumes that if you don't have a QEMU with
> > >>this capability that we should fail the device removal.  This is a
> > >>strong statement around hotplug that isn't consistent with previous
> > >>releases so I'm open to other approachs, but given the potential data
> > >>leakage problem hot-remove can lead to without drive_unplug, I think
> > >>it's the right thing to do.
> > >>     
> > >I don't think we can do this, since it obviously breaks every single
> > >existing deployment out there. Users who have sVirt enabled will
> > >have a level of protection from the data leakage, so I don't think
> > >it is a severe problem.
> > >   
> > 
> > I think that's reasonable but if sVirt is disabled, libvirt at least 
> > should log something somewhere to indicate that something may be wrong.
> 
> We can syslog a warning anytime we try to run  drive_unplug and it is not
> present.

Let me know your preference here and I'll respin the patch.

> 
> Regards,
> Daniel
> -- 
> |: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
> |: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
> |: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh at us.ibm.com