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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

> +     */
> +    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?

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


>      device_hotplug_done(egc, aodev);
>      return;
>  }
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 89bbf7d..21bb774 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2086,6 +2086,10 @@ struct libxl__ao_device {
>      libxl__device_action action;
>      libxl__device *dev;
>      int force;
> +    /* If force removal is successful, force_removed=1. The device is
> +     * gone from guest's PoV
> +     */
> +    int force_removed;
>      libxl__device_callback *callback;
>      /* return value, zeroed by user on entry, is valid on callback */
>      int rc;



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