Re: [Xen-devel] [PATCH V4 23/24] libxl: consider force removal of device successful

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-06 at 17:04 +0100, Wei Liu wrote:
> On Tue, May 06, 2014 at 04:48:40PM +0100, Ian Campbell wrote:
> > On Thu, 2014-05-01 at 13:58 +0100, Wei Liu wrote:
> > > If a device is force removed and the force removal successes, from
> > 
> > "succeeds".
> > 
> > > guest's PoV this device is gone. So the toolstack should consider this a
> > > successful case as well.
> > 
> > This seems rather self evident (consider success as success), were you
> > perhaps trying to say something opposite, which is that if a force
> > remove fails for some reason that we should (e.g. for the purposes of a
> > later migration etc) consider the device to have actually been removed?
> > 
> 
> No.
> 
> The commit message is written like this due to the current behavior.
> 
> If you remove a device, and the backend times out, libxl will try to
> force-remove (destroy) this device. Then it returns "fail to remove a
> device" even if the force removal is successful.
> 
> This is rather confusing because libxl tells you the removal fails, but
> actually the device is long gone from guest's PoV.

I see. I'm not sure how sensible I think this automatic fallback to
forcing is, but given that is the behaviour I think your change does
make sense. I'd be in favour of pasting most of your explanation above
into the commit message though and/or comments.

> > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > ---
> > >  tools/libxl/libxl_device.c   |   12 ++++++++++--
> > >  tools/libxl/libxl_internal.h |    4 ++++
> > >  2 files changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > > index fa99f77..7a37778 100644
> > > --- a/tools/libxl/libxl_device.c
> > > +++ b/tools/libxl/libxl_device.c
> > > @@ -845,6 +845,11 @@ void libxl__initiate_device_remove(libxl__egc *egc,
> > >          if (rc < 0) goto out;
> > >      }
> > >  
> > > +    /* At this point the XS transaction is commited. So check if we
> > > +     * force removal of the device.
> > 
> > "check if we were force removing the device". I think committed has two
> > t-s too (although I confess I'm not 100% sure...).
> > 
> 
> You're right. I have lots of typos in this series. :-(

FWIW I only spot other people's, not my own ;-)

> 
> > > +     */
> > > +    aodev->force_removed = aodev->force;
> > 
> > How and when can aodev->force_removed be distinct from aodev->force?
> > 
> 
> The removal of xenstore entries can fail. So we are only sure if the
> device is force removed if the xenstore transaction is committed.

If the xenstore commit fails the force_removed and force can differ?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel