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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Sent: 04 September 2020 09:53
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; Ian 
> Jackson
> <iwj@xxxxxxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Anthony PERARD 
> <anthony.perard@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> > Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > ---
> >  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.