Re: [Xen-devel] [PATCH V5 30/32] libxl: consider force removal of device successful

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 20, 2014 at 03:26:59PM +0100, Ian Campbell wrote:
> On Tue, 2014-05-13 at 22:54 +0100, Wei Liu wrote:
> > The current behavior of libxl to remove a device is, if the backend
> > times out, libxl will initiate a force removal (destroy) of that device.
> > However, libxl still returns failure in that case, even if the force
> > removal was successful.
> > 
> > If a device is force removed and the force removal succeeds, from
> > guest's PoV this device is gone. Libxl should consider this a
> > successful case as well.
> 
> This seems reasonable.
> 
> However the implementation confuses me. Perhaps this is one for Ian J,
> but let me see if I can work it out.
> 
> > 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..ce0b358 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 committed. So check if we were
> > +     * force removing the device.
> 
> Why does the fact that we were or not force committing relate to the XS
> transaction?
> 

Force removal is in fact removing xenstore entries. If XS transcation
fails then force removal (if aodev->force == 1) also fails.

> > +     */
> > +    aodev->force_removed = aodev->force;
> > +
> >      rc = libxl__ev_devstate_wait(gc, &aodev->backend_ds,
> >                                   device_backend_callback,
> >                                   state_path, XenbusStateClosed,
> > @@ -928,7 +933,7 @@ static void device_backend_callback(libxl__egc *egc, 
> > libxl__ev_devstate *ds,
> >          return;
> >      }
> >  
> > -    if (rc) {
> > +    if (rc && !aodev->force_removed) {
> >          LOG(ERROR, "unable to %s device with path %s",
> >                     libxl__device_action_to_string(aodev->action),
> >                     libxl__device_backend_path(gc, aodev->dev));
> > @@ -939,7 +944,10 @@ static void device_backend_callback(libxl__egc *egc, 
> > libxl__ev_devstate *ds,
> >      return;
> >  
> >  out:
> > -    aodev->rc = rc;
> > +    /* If force removal is successful, the device is gone from guest's
> > +     * PoV. Libxl should return success, too.
> > +     */
> > +    aodev->rc = aodev->force_removed ? 0 : rc;
> 
> This is the bit which confuses me most. If the forced removal was
> successful, why was rc not 0 already in that case?
> 

rc is ERROR_TIMEOUT in the senario stated in commit message IIRC.

> I'm also not sure where the case that the force remove also fails is.
> Perhaps it cannot possibly fail?
> 

XS transaction can fail.

Wei.


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