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

[PATCH 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):
  xl: implement documented --force option for block-detach
  libxl: do not automatically force detach of block devices

 docs/man/xl.1.pod.in       |  4 ++--
 tools/libxl/libxl_device.c |  9 ++++++---
 tools/xl/xl_block.c        | 21 ++++++++++++++++-----
 tools/xl/xl_cmdtable.c     |  3 ++-
 4 files changed, 26 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH 1/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.
This option, however, is not implemented. This patch implements the option.

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>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 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..05696643bf 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_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 ? "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

[PATCH 2/2] libxl: do not automatically force detach of block devices

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

The manpage for 'xl' documents that guest co-operation is required for a (non-
forced) block-detach operation and that it may consequently fail. Currently,
however, the implementation of generic device removal means that a time-out
of a block-detach is being automatically re-tried with the force flag set
rather than failing. This patch stops such behaviour.

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>
---
 tools/libxl/libxl_device.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 0381c5d509..d17ca78848 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1092,7 +1092,8 @@ 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 &&
+        aodev->dev->kind != LIBXL__DEVICE_KIND_VBD) {
         LOGD(DEBUG, aodev->dev->domid, "Timeout reached, initiating forced remove");
         aodev->force = 1;
         libxl__initiate_device_generic_remove(egc, aodev);
@@ -1103,7 +1104,8 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
         LOGD(ERROR, aodev->dev->domid, "unable to %s device with path %s",
                     libxl__device_action_to_string(aodev->action),
                     libxl__device_backend_path(gc, aodev->dev));
-        goto out;
+        if (!aodev->force)
+            goto out;
     }
 
     device_hotplug(egc, aodev);
@@ -1319,7 +1321,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 || !aodev->rc)) {
         rc = libxl__device_destroy(gc, aodev->dev);
         if (!aodev->rc)
             aodev->rc = rc;
-- 
2.20.1

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Roger Pau Monné]

On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> The manpage for 'xl' documents that guest co-operation is required for a (non-
> forced) block-detach operation and that it may consequently fail. Currently,
> however, the implementation of generic device removal means that a time-out
> of a block-detach is being automatically re-tried with the force flag set
> rather than failing. This patch stops such behaviour.

Won't this break cleanup on domain shutdown if the guest doesn't close
the devices itself?

I think we need some special-casing on shutdown that keeps the current
behavior on that case.

> 
> 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>
> ---
>  tools/libxl/libxl_device.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 0381c5d509..d17ca78848 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -1092,7 +1092,8 @@ 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 &&
> +        aodev->dev->kind != LIBXL__DEVICE_KIND_VBD) {

Doing this differentiation for block only seems weird, we should treat
all devices equally.

Thanks, Roger.

RE: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 04 September 2020 09:53
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Ian Jackson
> <iwj@xenproject.org>; Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>
> Subject: Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
> 
> On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > The manpage for 'xl' documents that guest co-operation is required for a (non-
> > forced) block-detach operation and that it may consequently fail. Currently,
> > however, the implementation of generic device removal means that a time-out
> > of a block-detach is being automatically re-tried with the force flag set
> > rather than failing. This patch stops such behaviour.
> 
> Won't this break cleanup on domain shutdown if the guest doesn't close
> the devices itself?
> 

I don't think so... AFAIK that's a destroy i.e. it's always forced (since there's no way the guest can co-operate at the point).

> I think we need some special-casing on shutdown that keeps the current
> behavior on that case.
> 
> >
> > 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>
> > ---
> >  tools/libxl/libxl_device.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > index 0381c5d509..d17ca78848 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -1092,7 +1092,8 @@ 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 &&
> > +        aodev->dev->kind != LIBXL__DEVICE_KIND_VBD) {
> 
> Doing this differentiation for block only seems weird, we should treat
> all devices equally.
> 

That is not how things are documented in the xl manpage though; block-detach is the only one to have a force option. I assume that was deliberate.

  Paul

> Thanks, Roger.

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Roger Pau Monné]

On Fri, Sep 04, 2020 at 10:47:37AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: 04 September 2020 09:53
> > To: Paul Durrant <paul@xen.org>
> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Ian Jackson
> > <iwj@xenproject.org>; Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>
> > Subject: Re: [PATCH 2/2] libxl: do not automatically force detach of block devices
> > 
> > On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > >
> > > The manpage for 'xl' documents that guest co-operation is required for a (non-
> > > forced) block-detach operation and that it may consequently fail. Currently,
> > > however, the implementation of generic device removal means that a time-out
> > > of a block-detach is being automatically re-tried with the force flag set
> > > rather than failing. This patch stops such behaviour.
> > 
> > Won't this break cleanup on domain shutdown if the guest doesn't close
> > the devices itself?
> > 
> 
> I don't think so... AFAIK that's a destroy i.e. it's always forced (since there's no way the guest can co-operate at the point).

Urgh, yes, sorry.

> > I think we need some special-casing on shutdown that keeps the current
> > behavior on that case.
> > 
> > >
> > > 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>
> > > ---
> > >  tools/libxl/libxl_device.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > > index 0381c5d509..d17ca78848 100644
> > > --- a/tools/libxl/libxl_device.c
> > > +++ b/tools/libxl/libxl_device.c
> > > @@ -1092,7 +1092,8 @@ 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 &&
> > > +        aodev->dev->kind != LIBXL__DEVICE_KIND_VBD) {
> > 
> > Doing this differentiation for block only seems weird, we should treat
> > all devices equally.
> > 
> 
> That is not how things are documented in the xl manpage though; block-detach is the only one to have a force option. I assume that was deliberate.

Oh right, that's weird. I guess removing a block device without guest
cooperation will likely result in a guest crash, while the same it's
not true for other devices.

Thanks, Roger.

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

[Paul Durrant]

Ping? Can I get a toolstack maintainer opinion on this?

Thanks,

  Paul

> -----Original Message-----
> From: Paul Durrant <paul@xen.org>
> Sent: 03 September 2020 11:06
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>
> Subject: [PATCH 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):
>   xl: implement documented --force option for block-detach
>   libxl: do not automatically force detach of block devices
> 
>  docs/man/xl.1.pod.in       |  4 ++--
>  tools/libxl/libxl_device.c |  9 ++++++---
>  tools/xl/xl_block.c        | 21 ++++++++++++++++-----
>  tools/xl/xl_cmdtable.c     |  3 ++-
>  4 files changed, 26 insertions(+), 11 deletions(-)
> 
> --
> 2.20.1

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

[Wei Liu]

On Tue, Sep 08, 2020 at 11:52:48AM +0100, Paul Durrant wrote:
> Ping? Can I get a toolstack maintainer opinion on this?

This series landed in my @xen.org inbox just fine but I haven't got
around reviewing it.

Wei.

Re: [PATCH 1/2] xl: implement documented --force option for block-detach

[Wei Liu]

On Thu, Sep 03, 2020 at 11:05:36AM +0100, Paul Durrant wrote:
> 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.
> This option, however, is not implemented. This patch implements the option.
> 
> 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>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  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

Missing "." at the end. This can be fixed while committing.

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

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Wei Liu]

On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> The manpage for 'xl' documents that guest co-operation is required for a (non-
> forced) block-detach operation and that it may consequently fail. Currently,
> however, the implementation of generic device removal means that a time-out
> of a block-detach is being automatically re-tried with the force flag set
> rather than failing. This patch stops such behaviour.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

I'm two-minded here.

On the one hand, special-casing VBD in libxl to conform to xl's
behaviour seems wrong to me.

On the other hand, if we want to treat all devices the same in libxl,
libxl should drop its force-removal behaviour all together, and the
retry behaviour would need to be implemented in xl for all devices
excepts for VBD. This requires a bit of code churn and, more
importantly, changes how those device removal APIs behave. In the end
this effort may not worth it.

If we go with the patch here, we should document this special case on
libxl level somehow.

Anthony and Ian, do you have any opinion on this?

Wei.

> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_device.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 0381c5d509..d17ca78848 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -1092,7 +1092,8 @@ 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 &&
> +        aodev->dev->kind != LIBXL__DEVICE_KIND_VBD) {
>          LOGD(DEBUG, aodev->dev->domid, "Timeout reached, initiating forced remove");
>          aodev->force = 1;
>          libxl__initiate_device_generic_remove(egc, aodev);
> @@ -1103,7 +1104,8 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
>          LOGD(ERROR, aodev->dev->domid, "unable to %s device with path %s",
>                      libxl__device_action_to_string(aodev->action),
>                      libxl__device_backend_path(gc, aodev->dev));
> -        goto out;
> +        if (!aodev->force)
> +            goto out;
>      }
>  
>      device_hotplug(egc, aodev);
> @@ -1319,7 +1321,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 || !aodev->rc)) {
>          rc = libxl__device_destroy(gc, aodev->dev);
>          if (!aodev->rc)
>              aodev->rc = rc;
> -- 
> 2.20.1
> 

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Roger Pau Monné]

On Tue, Sep 08, 2020 at 02:19:03PM +0000, Wei Liu wrote:
> On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> > 
> > The manpage for 'xl' documents that guest co-operation is required for a (non-
> > forced) block-detach operation and that it may consequently fail. Currently,
> > however, the implementation of generic device removal means that a time-out
> > of a block-detach is being automatically re-tried with the force flag set
> > rather than failing. This patch stops such behaviour.
> > 
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> I'm two-minded here.
> 
> On the one hand, special-casing VBD in libxl to conform to xl's
> behaviour seems wrong to me.
> 
> On the other hand, if we want to treat all devices the same in libxl,
> libxl should drop its force-removal behaviour all together, and the
> retry behaviour would need to be implemented in xl for all devices
> excepts for VBD. This requires a bit of code churn and, more
> importantly, changes how those device removal APIs behave. In the end
> this effort may not worth it.

I would be worried about those changes, as we would likely have to
also change libvirt or any other downstreams?

> If we go with the patch here, we should document this special case on
> libxl level somehow.
> 
> Anthony and Ian, do you have any opinion on this?

Maybe a new function should be introduced instead, that attempts to
remove a device gracefully and fail otherwise?

Then none of the current APIs would change, and xl could use this new
function to handle VBD removal?

Roger.

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Wei Liu]

On Mon, Sep 14, 2020 at 12:41:09PM +0200, Roger Pau Monné wrote:
> On Tue, Sep 08, 2020 at 02:19:03PM +0000, Wei Liu wrote:
> > On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > > 
> > > The manpage for 'xl' documents that guest co-operation is required for a (non-
> > > forced) block-detach operation and that it may consequently fail. Currently,
> > > however, the implementation of generic device removal means that a time-out
> > > of a block-detach is being automatically re-tried with the force flag set
> > > rather than failing. This patch stops such behaviour.
> > > 
> > > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > 
> > I'm two-minded here.
> > 
> > On the one hand, special-casing VBD in libxl to conform to xl's
> > behaviour seems wrong to me.
> > 
> > On the other hand, if we want to treat all devices the same in libxl,
> > libxl should drop its force-removal behaviour all together, and the
> > retry behaviour would need to be implemented in xl for all devices
> > excepts for VBD. This requires a bit of code churn and, more
> > importantly, changes how those device removal APIs behave. In the end
> > this effort may not worth it.
> 
> I would be worried about those changes, as we would likely have to
> also change libvirt or any other downstreams?
> 
> > If we go with the patch here, we should document this special case on
> > libxl level somehow.
> > 
> > Anthony and Ian, do you have any opinion on this?
> 
> Maybe a new function should be introduced instead, that attempts to
> remove a device gracefully and fail otherwise?
> 
> Then none of the current APIs would change, and xl could use this new
> function to handle VBD removal?

This sounds fine to me.

Wei.

> 
> Roger.

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 2/2] libxl: do not automatically force detach of block devices"):
> On Mon, Sep 14, 2020 at 12:41:09PM +0200, Roger Pau Monné wrote:
> > Maybe a new function should be introduced instead, that attempts to
> > remove a device gracefully and fail otherwise?
> > 
> > Then none of the current APIs would change, and xl could use this new
> > function to handle VBD removal?
> 
> This sounds fine to me.

I agree.

If there is going to be different default policy for different devices
it ought to be in xl, not libxl, but frankly I think this is an
anomaly.

I suggest we have a new entrypoint that specifies the fallback
behaviour explicitly.  ISTM that there are three possible behaviours:
 - fail if the guest does not cooperate
 - fall back to force remove
 - rip the device out immediately

The last of these would be useful only in rare situations.  IDK if the
length of the timeout (for the first two cases) ought to be a
parameter too.

Ian.