> -----Original Message-----
> From: Roger Pau Monné <[email protected]>
> Sent: 15 September 2020 15:32
> To: Paul Durrant <[email protected]>
> Cc: [email protected]; Durrant, Paul <[email protected]>;
> Ian Jackson
> <[email protected]>; Wei Liu <[email protected]>; Anthony PERARD
> <[email protected]>
> 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 <[email protected]>
> >
> > ... 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 <[email protected]>
>
> Reviewed-by: Roger Pau Monné <[email protected]>
>
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.
