[PATCH v2 0/2] fix 'xl block-detach'

[PATCH v2 0/2] fix 'xl block-detach'

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This series makes it behave as the documentation states it should

Paul Durrant (2):
  libxl: provide a mechanism to define a device 'safe remove'
    function...
  xl: implement documented '--force' option for block-detach

 docs/man/xl.1.pod.in         |  4 ++--
 tools/libxl/libxl.h          | 33 +++++++++++++++++++++++++--------
 tools/libxl/libxl_device.c   |  9 +++++----
 tools/libxl/libxl_disk.c     |  4 +++-
 tools/libxl/libxl_domain.c   |  2 +-
 tools/libxl/libxl_internal.h | 30 +++++++++++++++++++++++-------
 tools/xl/xl_block.c          | 21 ++++++++++++++++-----
 tools/xl/xl_cmdtable.c       |  3 ++-
 8 files changed, 77 insertions(+), 29 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove' function...

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

... and use it to define libxl_device_disk_safe_remove().

This patch builds on the existent macro magic by using a new value of the
'force' field in in libxl__ao_device.
It is currently defined as an int but is used in a boolean manner where
1 means the operation is forced and 0 means it is not (but is actually forced
after a 10s time-out). In adding a third value, this patch re-defines 'force'
as a struct type (libxl__force) with a single 'flag' field taking an
enumerated value:

LIBXL__FORCE_AUTO - corresponding to the old 0 value
LIBXL__FORCE_ON   - corresponding to the old 1 value
LIBXL__FORCE_OFF  - the new value

The LIBXL_DEFINE_DEVICE_REMOVE() macro is then modified to define the
libxl_device_<type>_remove() and libxl_device_<type>_destroy() functions,
setting LIBXL__FORCE_AUTO and LIBXL__FORCE_ON (respectively) in the
libxl__ao_device passed to libxl__initiate_device_generic_remove() and a
new macro, LIBXL_DEFINE_DEVICE_SAFE_REMOVE(), is defined that sets
LIBXL__FORCE_OFF instead. This macro is used to define the new
libxl_device_disk_safe_remove() function.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v2:
 - New in v2
---
 tools/libxl/libxl.h          | 33 +++++++++++++++++++++++++--------
 tools/libxl/libxl_device.c   |  9 +++++----
 tools/libxl/libxl_disk.c     |  4 +++-
 tools/libxl/libxl_domain.c   |  2 +-
 tools/libxl/libxl_internal.h | 30 +++++++++++++++++++++++-------
 5 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1cd6c38e83..1ea5b4f446 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -438,6 +438,12 @@
  */
 #define LIBXL_HAVE_CREATEINFO_PASSTHROUGH 1
 
+/*
+ * LIBXL_HAVE_DISK_SAFE_REMOVE indicates that the
+ * libxl_device_disk_safe_remove() function is defined.
+ */
+#define LIBXL_HAVE_DISK_SAFE_REMOVE 1
+
 /*
  * libxl ABI compatibility
  *
@@ -1936,6 +1942,15 @@ void libxl_vcpuinfo_list_free(libxl_vcpuinfo *, int nr_vcpus);
  *   structure is passed in are filled in with appropriate values for
  *   the device created.
  *
+ * libxl_device_<type>_destroy(ctx, domid, device):
+ *
+ *   Removes the given device from the specified domain without guest
+ *   co-operation. It is guest specific what affect this will have on
+ *   a running guest.
+ *
+ *   This function does not interact with the guest and therefore
+ *   cannot block on the guest.
+ *
  * libxl_device_<type>_remove(ctx, domid, device):
  *
  *   Removes the given device from the specified domain by performing
@@ -1943,16 +1958,14 @@ void libxl_vcpuinfo_list_free(libxl_vcpuinfo *, int nr_vcpus);
  *   guest is running.
  *
  *   This method is currently synchronous and therefore can block
- *   while interacting with the guest.
- *
- * libxl_device_<type>_destroy(ctx, domid, device):
+ *   while interacting with the guest. There is a time-out of 10s on
+ *   this interaction after which libxl_device_<type>_destroy()
+ *   semantics apply.
  *
- *   Removes the given device from the specified domain without guest
- *   co-operation. It is guest specific what affect this will have on
- *   a running guest.
+ * libxl_device_<type>_safe_remove(ctx, domid, device):
  *
- *   This function does not interact with the guest and therefore
- *   cannot block on the guest.
+ *   This has the same semantics as libxl_device_<type>_remove() but,
+ *   in the event of hitting the 10s time-out, this function will fail.
  *
  * Controllers
  * -----------
@@ -2033,6 +2046,10 @@ int libxl_device_disk_destroy(libxl_ctx *ctx, uint32_t domid,
                               libxl_device_disk *disk,
                               const libxl_asyncop_how *ao_how)
                               LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_disk_safe_remove(libxl_ctx *ctx, uint32_t domid,
+                                  libxl_device_disk *disk,
+                                  const libxl_asyncop_how *ao_how)
+                                  LIBXL_EXTERNAL_CALLERS_ONLY;
 
 libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx,
                                           uint32_t domid, int *num)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 0381c5d509..e081faf9a9 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -973,7 +973,7 @@ void libxl__initiate_device_generic_remove(libxl__egc *egc,
             goto out;
         }
 
-        if (aodev->force)
+        if (aodev->force.flag == LIBXL__FORCE_ON)
             libxl__xs_path_cleanup(gc, t,
                                    libxl__device_frontend_path(gc, aodev->dev));
 
@@ -1092,9 +1092,9 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
 
     if (rc == ERROR_TIMEDOUT &&
         aodev->action == LIBXL__DEVICE_ACTION_REMOVE &&
-        !aodev->force) {
+        aodev->force.flag == LIBXL__FORCE_AUTO) {
         LOGD(DEBUG, aodev->dev->domid, "Timeout reached, initiating forced remove");
-        aodev->force = 1;
+        aodev->force.flag = LIBXL__FORCE_ON;
         libxl__initiate_device_generic_remove(egc, aodev);
         return;
     }
@@ -1319,7 +1319,8 @@ static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev)
     device_hotplug_clean(gc, aodev);
 
     /* Clean xenstore if it's a disconnection */
-    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE) {
+    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE &&
+        (aodev->force.flag == LIBXL__FORCE_ON || !aodev->rc)) {
         rc = libxl__device_destroy(gc, aodev->dev);
         if (!aodev->rc)
             aodev->rc = rc;
diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index ddc1eec176..de183e0fb0 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -1277,7 +1277,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
         aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
         aodev->dev = device;
         aodev->callback = local_device_detach_cb;
-        aodev->force = 0;
+        aodev->force.flag = LIBXL__FORCE_AUTO;
         libxl__initiate_device_generic_remove(egc, aodev);
         return;
     }
@@ -1318,10 +1318,12 @@ out:
  * libxl__add_disks
  * libxl_device_disk_remove
  * libxl_device_disk_destroy
+ * libxl_device_disk_safe_remove
  */
 LIBXL_DEFINE_DEVICE_ADD(disk)
 LIBXL_DEFINE_DEVICES_ADD(disk)
 LIBXL_DEFINE_DEVICE_REMOVE(disk)
+LIBXL_DEFINE_DEVICE_SAFE_REMOVE(disk)
 
 static int libxl_device_disk_compare(const libxl_device_disk *d1,
                                      const libxl_device_disk *d2)
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 39f08a6519..5d4ec90711 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1279,7 +1279,7 @@ static void dm_destroy_cb(libxl__egc *egc,
     dis->drs.ao = ao;
     dis->drs.domid = domid;
     dis->drs.callback = devices_destroy_cb;
-    dis->drs.force = 1;
+    dis->drs.force.flag = LIBXL__FORCE_ON;
     libxl__devices_destroy(egc, &dis->drs);
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e16ae9630b..1fcf85c3e2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2730,12 +2730,20 @@ _hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev);
 /* generic callback for devices that only need to set ao_complete */
 _hidden void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev);
 
+typedef struct {
+    enum {
+        LIBXL__FORCE_AUTO, /* Re-execute with FORCE_ON if op times out */
+        LIBXL__FORCE_ON,
+        LIBXL__FORCE_OFF,
+    } flag;
+} libxl__force;
+
 struct libxl__ao_device {
     /* filled in by user */
     libxl__ao *ao;
     libxl__device_action action;
     libxl__device *dev;
-    int force;
+    libxl__force force;
     libxl__device_callback *callback;
     /* return value, zeroed by user on entry, is valid on callback */
     int rc;
@@ -3787,7 +3795,7 @@ _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
         aodev->action = LIBXL__DEVICE_ACTION_REMOVE;                    \
         aodev->dev = device;                                            \
         aodev->callback = device_addrm_aocomplete;                      \
-        aodev->force = f;                                               \
+        aodev->force.flag = f;                                          \
         libxl__initiate_device_##remtype##_remove(egc, aodev);          \
                                                                         \
     out:                                                                \
@@ -3862,12 +3870,20 @@ _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
 
 
 #define LIBXL_DEFINE_DEVICE_REMOVE(type)                                \
-    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, generic, remove, 0)            \
-    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, generic, destroy, 1)
+    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, generic, remove,               \
+                                   LIBXL__FORCE_AUTO)                   \
+    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, generic, destroy,              \
+                                   LIBXL__FORCE_ON)
 
 #define LIBXL_DEFINE_DEVICE_REMOVE_CUSTOM(type)                         \
-    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, type, remove, 0)               \
-    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, type, destroy, 1)
+    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, type, remove,                  \
+                                   LIBXL__FORCE_AUTO)                   \
+    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, type, destroy,                 \
+                                   LIBXL__FORCE_ON)
+
+#define LIBXL_DEFINE_DEVICE_SAFE_REMOVE(type)                           \
+    LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, generic, safe_remove,          \
+                                   LIBXL__FORCE_OFF)
 
 #define LIBXL_DEFINE_DEVICE_LIST(type)                                  \
     libxl_device_##type *libxl_device_##type##_list(libxl_ctx *ctx,     \
@@ -4028,7 +4044,7 @@ struct libxl__devices_remove_state {
     libxl__ao *ao;
     uint32_t domid;
     libxl__devices_remove_callback *callback;
-    int force; /* libxl_device_TYPE_destroy rather than _remove */
+    libxl__force force; /* libxl_device_TYPE_destroy rather than _remove */
     /* private */
     libxl__multidev multidev;
     int num_devices;
-- 
2.20.1

Re: [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove' function...

[Roger Pau Monné]

On Tue, Sep 15, 2020 at 03:10:06PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> ... and use it to define libxl_device_disk_safe_remove().
> 
> This patch builds on the existent macro magic by using a new value of the
> 'force' field in in libxl__ao_device.
> It is currently defined as an int but is used in a boolean manner where
> 1 means the operation is forced and 0 means it is not (but is actually forced
> after a 10s time-out). In adding a third value, this patch re-defines 'force'
> as a struct type (libxl__force) with a single 'flag' field taking an
> enumerated value:
> 
> LIBXL__FORCE_AUTO - corresponding to the old 0 value
> LIBXL__FORCE_ON   - corresponding to the old 1 value
> LIBXL__FORCE_OFF  - the new value
> 
> The LIBXL_DEFINE_DEVICE_REMOVE() macro is then modified to define the
> libxl_device_<type>_remove() and libxl_device_<type>_destroy() functions,
> setting LIBXL__FORCE_AUTO and LIBXL__FORCE_ON (respectively) in the
> libxl__ao_device passed to libxl__initiate_device_generic_remove() and a
> new macro, LIBXL_DEFINE_DEVICE_SAFE_REMOVE(), is defined that sets
> LIBXL__FORCE_OFF instead. This macro is used to define the new
> libxl_device_disk_safe_remove() function.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one nit.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index e16ae9630b..1fcf85c3e2 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2730,12 +2730,20 @@ _hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev);
>  /* generic callback for devices that only need to set ao_complete */
>  _hidden void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev);
>  
> +typedef struct {
> +    enum {
> +        LIBXL__FORCE_AUTO, /* Re-execute with FORCE_ON if op times out */
> +        LIBXL__FORCE_ON,
> +        LIBXL__FORCE_OFF,
> +    } flag;
> +} libxl__force;

Couldn't you just use the typedef against the union directly instead
of wrapping it around a struct?

Thanks, Roger.

RE: [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove' function...

[Durrant, Paul]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 15 September 2020 15:32
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Ian Jackson
> <iwj@xenproject.org>; Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove'
> function...
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Tue, Sep 15, 2020 at 03:10:06PM +0100, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > ... and use it to define libxl_device_disk_safe_remove().
> >
> > This patch builds on the existent macro magic by using a new value of the
> > 'force' field in in libxl__ao_device.
> > It is currently defined as an int but is used in a boolean manner where
> > 1 means the operation is forced and 0 means it is not (but is actually forced
> > after a 10s time-out). In adding a third value, this patch re-defines 'force'
> > as a struct type (libxl__force) with a single 'flag' field taking an
> > enumerated value:
> >
> > LIBXL__FORCE_AUTO - corresponding to the old 0 value
> > LIBXL__FORCE_ON   - corresponding to the old 1 value
> > LIBXL__FORCE_OFF  - the new value
> >
> > The LIBXL_DEFINE_DEVICE_REMOVE() macro is then modified to define the
> > libxl_device_<type>_remove() and libxl_device_<type>_destroy() functions,
> > setting LIBXL__FORCE_AUTO and LIBXL__FORCE_ON (respectively) in the
> > libxl__ao_device passed to libxl__initiate_device_generic_remove() and a
> > new macro, LIBXL_DEFINE_DEVICE_SAFE_REMOVE(), is defined that sets
> > LIBXL__FORCE_OFF instead. This macro is used to define the new
> > libxl_device_disk_safe_remove() function.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Thanks.

> Just one nit.
> 
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index e16ae9630b..1fcf85c3e2 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -2730,12 +2730,20 @@ _hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device
> *aodev);
> >  /* generic callback for devices that only need to set ao_complete */
> >  _hidden void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev);
> >
> > +typedef struct {
> > +    enum {
> > +        LIBXL__FORCE_AUTO, /* Re-execute with FORCE_ON if op times out */
> > +        LIBXL__FORCE_ON,
> > +        LIBXL__FORCE_OFF,
> > +    } flag;
> > +} libxl__force;
> 
> Couldn't you just use the typedef against the union directly instead
> of wrapping it around a struct?

You mean s/union/enum?

I could have done that, but it helped find all the places where aodev->force is used and I liked the abstraction. I don't mind changing if there are strong opinions against it.

  Paul

> 
> Thanks, Roger.

Re: [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove' function...

[Roger Pau Monné]

On Tue, Sep 15, 2020 at 02:40:09PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: 15 September 2020 15:32
> > To: Paul Durrant <paul@xen.org>
> > Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Ian Jackson
> > <iwj@xenproject.org>; Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>
> > Subject: RE: [EXTERNAL] [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove'
> > function...
> > 
> > CAUTION: This email originated from outside of the organization. Do not click links or open
> > attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Tue, Sep 15, 2020 at 03:10:06PM +0100, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > >
> > > ... and use it to define libxl_device_disk_safe_remove().
> > >
> > > This patch builds on the existent macro magic by using a new value of the
> > > 'force' field in in libxl__ao_device.
> > > It is currently defined as an int but is used in a boolean manner where
> > > 1 means the operation is forced and 0 means it is not (but is actually forced
> > > after a 10s time-out). In adding a third value, this patch re-defines 'force'
> > > as a struct type (libxl__force) with a single 'flag' field taking an
> > > enumerated value:
> > >
> > > LIBXL__FORCE_AUTO - corresponding to the old 0 value
> > > LIBXL__FORCE_ON   - corresponding to the old 1 value
> > > LIBXL__FORCE_OFF  - the new value
> > >
> > > The LIBXL_DEFINE_DEVICE_REMOVE() macro is then modified to define the
> > > libxl_device_<type>_remove() and libxl_device_<type>_destroy() functions,
> > > setting LIBXL__FORCE_AUTO and LIBXL__FORCE_ON (respectively) in the
> > > libxl__ao_device passed to libxl__initiate_device_generic_remove() and a
> > > new macro, LIBXL_DEFINE_DEVICE_SAFE_REMOVE(), is defined that sets
> > > LIBXL__FORCE_OFF instead. This macro is used to define the new
> > > libxl_device_disk_safe_remove() function.
> > >
> > > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> 
> Thanks.
> 
> > Just one nit.
> > 
> > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > index e16ae9630b..1fcf85c3e2 100644
> > > --- a/tools/libxl/libxl_internal.h
> > > +++ b/tools/libxl/libxl_internal.h
> > > @@ -2730,12 +2730,20 @@ _hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device
> > *aodev);
> > >  /* generic callback for devices that only need to set ao_complete */
> > >  _hidden void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev);
> > >
> > > +typedef struct {
> > > +    enum {
> > > +        LIBXL__FORCE_AUTO, /* Re-execute with FORCE_ON if op times out */
> > > +        LIBXL__FORCE_ON,
> > > +        LIBXL__FORCE_OFF,
> > > +    } flag;
> > > +} libxl__force;
> > 
> > Couldn't you just use the typedef against the union directly instead
> > of wrapping it around a struct?
> 
> You mean s/union/enum?

Yes :) sorry for that.

> I could have done that, but it helped find all the places where aodev->force is used and I liked the abstraction. I don't mind changing if there are strong opinions against it.

While it's indeed helpful to find the users to fixup, it just makes
the lines longer in the final patch IMO. Let's see what opinions
others have however.

Thanks, Roger.

Re: [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove' function...

[Wei Liu]

On Tue, Sep 15, 2020 at 04:48:14PM +0200, Roger Pau Monné wrote:
> On Tue, Sep 15, 2020 at 02:40:09PM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > Sent: 15 September 2020 15:32
> > > To: Paul Durrant <paul@xen.org>
> > > Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Ian Jackson
> > > <iwj@xenproject.org>; Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>
> > > Subject: RE: [EXTERNAL] [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove'
> > > function...
> > > 
> > > CAUTION: This email originated from outside of the organization. Do not click links or open
> > > attachments unless you can confirm the sender and know the content is safe.
> > > 
> > > 
> > > 
> > > On Tue, Sep 15, 2020 at 03:10:06PM +0100, Paul Durrant wrote:
> > > > From: Paul Durrant <pdurrant@amazon.com>
> > > >
> > > > ... and use it to define libxl_device_disk_safe_remove().
> > > >
> > > > This patch builds on the existent macro magic by using a new value of the
> > > > 'force' field in in libxl__ao_device.
> > > > It is currently defined as an int but is used in a boolean manner where
> > > > 1 means the operation is forced and 0 means it is not (but is actually forced
> > > > after a 10s time-out). In adding a third value, this patch re-defines 'force'
> > > > as a struct type (libxl__force) with a single 'flag' field taking an
> > > > enumerated value:
> > > >
> > > > LIBXL__FORCE_AUTO - corresponding to the old 0 value
> > > > LIBXL__FORCE_ON   - corresponding to the old 1 value
> > > > LIBXL__FORCE_OFF  - the new value
> > > >
> > > > The LIBXL_DEFINE_DEVICE_REMOVE() macro is then modified to define the
> > > > libxl_device_<type>_remove() and libxl_device_<type>_destroy() functions,
> > > > setting LIBXL__FORCE_AUTO and LIBXL__FORCE_ON (respectively) in the
> > > > libxl__ao_device passed to libxl__initiate_device_generic_remove() and a
> > > > new macro, LIBXL_DEFINE_DEVICE_SAFE_REMOVE(), is defined that sets
> > > > LIBXL__FORCE_OFF instead. This macro is used to define the new
> > > > libxl_device_disk_safe_remove() function.
> > > >
> > > > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > > 
> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > > 
> > 
> > Thanks.
> > 
> > > Just one nit.
> > > 
> > > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > > index e16ae9630b..1fcf85c3e2 100644
> > > > --- a/tools/libxl/libxl_internal.h
> > > > +++ b/tools/libxl/libxl_internal.h
> > > > @@ -2730,12 +2730,20 @@ _hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device
> > > *aodev);
> > > >  /* generic callback for devices that only need to set ao_complete */
> > > >  _hidden void device_addrm_aocomplete(libxl__egc *egc, libxl__ao_device *aodev);
> > > >
> > > > +typedef struct {
> > > > +    enum {
> > > > +        LIBXL__FORCE_AUTO, /* Re-execute with FORCE_ON if op times out */
> > > > +        LIBXL__FORCE_ON,
> > > > +        LIBXL__FORCE_OFF,
> > > > +    } flag;
> > > > +} libxl__force;
> > > 
> > > Couldn't you just use the typedef against the union directly instead
> > > of wrapping it around a struct?
> > 
> > You mean s/union/enum?
> 
> Yes :) sorry for that.
> 
> > I could have done that, but it helped find all the places where aodev->force is used and I liked the abstraction. I don't mind changing if there are strong opinions against it.
> 
> While it's indeed helpful to find the users to fixup, it just makes
> the lines longer in the final patch IMO. Let's see what opinions
> others have however.

I don't feel strongly about this either way.

Wei.

> 
> Thanks, Roger.

Re: [PATCH v2 1/2] libxl: provide a mechanism to define a device 'safe remove' function...

[Wei Liu]

On Tue, Sep 15, 2020 at 03:10:06PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> ... and use it to define libxl_device_disk_safe_remove().
> 
> This patch builds on the existent macro magic by using a new value of the
> 'force' field in in libxl__ao_device.
> It is currently defined as an int but is used in a boolean manner where
> 1 means the operation is forced and 0 means it is not (but is actually forced
> after a 10s time-out). In adding a third value, this patch re-defines 'force'
> as a struct type (libxl__force) with a single 'flag' field taking an
> enumerated value:
> 
> LIBXL__FORCE_AUTO - corresponding to the old 0 value
> LIBXL__FORCE_ON   - corresponding to the old 1 value
> LIBXL__FORCE_OFF  - the new value
> 
> The LIBXL_DEFINE_DEVICE_REMOVE() macro is then modified to define the
> libxl_device_<type>_remove() and libxl_device_<type>_destroy() functions,
> setting LIBXL__FORCE_AUTO and LIBXL__FORCE_ON (respectively) in the
> libxl__ao_device passed to libxl__initiate_device_generic_remove() and a
> new macro, LIBXL_DEFINE_DEVICE_SAFE_REMOVE(), is defined that sets
> LIBXL__FORCE_OFF instead. This macro is used to define the new
> libxl_device_disk_safe_remove() function.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Reviewed-by: Wei Liu <wl@xen.org>

[PATCH v2 2/2] xl: implement documented '--force' option for block-detach

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

The manpage for 'xl' documents an option to force a block device to be
released even if the domain to which it is attached does not co-operate.
The documentation also states that, if the force flag is not specified, the
block-detach operation should fail.

Currently the force option is not implemented and a non-forced block-detach
will auto-force after a time-out of 10s. This patch implements the force
option and also stops auto-forcing a non-forced block-detach by calling
libxl_device_disk_safe_remove() rather than libxl_device_disk_remove(),
allowing the operation to fail cleanly as per the documented behaviour.

NOTE: The documentation is also adjusted since the normal positioning of
      options is before compulsory parameters. It is also noted that use of
      the --force option may lead to a guest crash.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v2:
 - Add missing '.'
 - Use the new libxl_device_disk_safe_remove() function
 - Keep Wei's A-b as the modifications are trivial
---
 docs/man/xl.1.pod.in   |  4 ++--
 tools/xl/xl_block.c    | 21 ++++++++++++++++-----
 tools/xl/xl_cmdtable.c |  3 ++-
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 52a47a6fbd..5f7d3a7134 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -1389,7 +1389,7 @@ Note that only PV block devices are supported by block-attach.
 Requests to attach emulated devices (eg, vdev=hdc) will result in only
 the PV view being available to the guest.
 
-=item B<block-detach> I<domain-id> I<devid> [I<OPTIONS>]
+=item B<block-detach> [I<OPTIONS>] I<domain-id> I<devid>
 
 Detach a domain's virtual block device. I<devid> may be the symbolic
 name or the numeric device id given to the device by domain 0.  You
@@ -1406,7 +1406,7 @@ B<OPTIONS>
 =item B<--force>
 
 If this parameter is specified the device will be forcefully detached, which
-may cause IO errors in the domain.
+may cause IO errors in the domain and possibly a guest crash
 
 =back
 
diff --git a/tools/xl/xl_block.c b/tools/xl/xl_block.c
index acaf9b96b8..70eed431e4 100644
--- a/tools/xl/xl_block.c
+++ b/tools/xl/xl_block.c
@@ -96,12 +96,21 @@ int main_blocklist(int argc, char **argv)
 
 int main_blockdetach(int argc, char **argv)
 {
+    static struct option opts[] = {
+        {"force", 0, 0, 'f'},
+        COMMON_LONG_OPTS
+    };
     uint32_t domid;
     int opt, rc = 0;
     libxl_device_disk disk;
-
-    SWITCH_FOREACH_OPT(opt, "", NULL, "block-detach", 2) {
-        /* No options */
+    bool force = false;
+
+    SWITCH_FOREACH_OPT(opt, "f", opts, "block-detach", 2) {
+    case 'f':
+        force = true;
+        break;
+    default:
+        break;
     }
 
     domid = find_domain(argv[optind]);
@@ -110,9 +119,11 @@ int main_blockdetach(int argc, char **argv)
         fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
         return 1;
     }
-    rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
+    rc = !force ? libxl_device_disk_safe_remove(ctx, domid, &disk, 0) :
+        libxl_device_disk_destroy(ctx, domid, &disk, 0);
     if (rc) {
-        fprintf(stderr, "libxl_device_disk_remove failed.\n");
+        fprintf(stderr, "libxl_device_disk_%s failed.\n",
+                !force ? "safe_remove" : "destroy");
         return 1;
     }
     libxl_device_disk_dispose(&disk);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 2b8e1b321a..7da6c1b927 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -368,7 +368,8 @@ struct cmd_spec cmd_table[] = {
     { "block-detach",
       &main_blockdetach, 0, 1,
       "Destroy a domain's virtual block device",
-      "<Domain> <DevId>",
+      "[option] <Domain> <DevId>",
+      "-f, --force        do not wait for the domain to release the device"
     },
     { "vtpm-attach",
       &main_vtpmattach, 1, 1,
-- 
2.20.1

RE: [PATCH v2 0/2] fix 'xl block-detach'

[Paul Durrant]

Ping? AFAICT this series is fully acked. Can it be committed soon?

  Paul

> -----Original Message-----
> From: Paul Durrant <paul@xen.org>
> Sent: 15 September 2020 15:10
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>
> Subject: [PATCH v2 0/2] fix 'xl block-detach'
> 
> From: Paul Durrant <pdurrant@amazon.com>
> 
> This series makes it behave as the documentation states it should
> 
> Paul Durrant (2):
>   libxl: provide a mechanism to define a device 'safe remove'
>     function...
>   xl: implement documented '--force' option for block-detach
> 
>  docs/man/xl.1.pod.in         |  4 ++--
>  tools/libxl/libxl.h          | 33 +++++++++++++++++++++++++--------
>  tools/libxl/libxl_device.c   |  9 +++++----
>  tools/libxl/libxl_disk.c     |  4 +++-
>  tools/libxl/libxl_domain.c   |  2 +-
>  tools/libxl/libxl_internal.h | 30 +++++++++++++++++++++++-------
>  tools/xl/xl_block.c          | 21 ++++++++++++++++-----
>  tools/xl/xl_cmdtable.c       |  3 ++-
>  8 files changed, 77 insertions(+), 29 deletions(-)
> 
> --
> 2.20.1

RE: [PATCH v2 0/2] fix 'xl block-detach'

[Paul Durrant]

Toolstack maintainers...

Ping+1

> -----Original Message-----
> From: Paul Durrant <xadimgnik@gmail.com>
> Sent: 25 September 2020 11:39
> To: xen-devel@lists.xenproject.org
> Cc: 'Paul Durrant' <pdurrant@amazon.com>
> Subject: RE: [PATCH v2 0/2] fix 'xl block-detach'
> 
> Ping? AFAICT this series is fully acked. Can it be committed soon?
> 
>   Paul
> 
> > -----Original Message-----
> > From: Paul Durrant <paul@xen.org>
> > Sent: 15 September 2020 15:10
> > To: xen-devel@lists.xenproject.org
> > Cc: Paul Durrant <pdurrant@amazon.com>
> > Subject: [PATCH v2 0/2] fix 'xl block-detach'
> >
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > This series makes it behave as the documentation states it should
> >
> > Paul Durrant (2):
> >   libxl: provide a mechanism to define a device 'safe remove'
> >     function...
> >   xl: implement documented '--force' option for block-detach
> >
> >  docs/man/xl.1.pod.in         |  4 ++--
> >  tools/libxl/libxl.h          | 33 +++++++++++++++++++++++++--------
> >  tools/libxl/libxl_device.c   |  9 +++++----
> >  tools/libxl/libxl_disk.c     |  4 +++-
> >  tools/libxl/libxl_domain.c   |  2 +-
> >  tools/libxl/libxl_internal.h | 30 +++++++++++++++++++++++-------
> >  tools/xl/xl_block.c          | 21 ++++++++++++++++-----
> >  tools/xl/xl_cmdtable.c       |  3 ++-
> >  8 files changed, 77 insertions(+), 29 deletions(-)
> >
> > --
> > 2.20.1
>