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 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>

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?

Thanks, Roger.