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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.