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

[Wei Liu <wl@xxxxxxx>]

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@xxxxxxxxxx>
> > > Sent: 15 September 2020 15:32
> > > To: Paul Durrant <paul@xxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul 
> > > <pdurrant@xxxxxxxxxxxx>; Ian Jackson
> > > <iwj@xxxxxxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Anthony PERARD 
> > > <anthony.perard@xxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > > >
> > > > ... 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@xxxxxxxxxx>
> > > 
> > > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > 
> > 
> > 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.