[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: "Durrant, Paul" <pdurrant@xxxxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 15 Sep 2020 16:48:14 +0200
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Paul Durrant <paul@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Delivery-date: Tue, 15 Sep 2020 14:48:50 +0000
  • Ironport-sdr: G07L0U+CiI5SMhn/IIM959eXnZT2tfzGFQnlS9mIBsKqOlsML8yjBRy/4ADh/gao1fBXlhWoo1 XWb+fpn8O09PWX1mAYjdbOJxkaORkGR+Br1lS+D8KosMk+6z9lnMyAJLX2n7OEhIJ7Ao+ByF7y ezf3WCc9RXTejwtIUun1lkBVArhUWvC2vIYe90fuiq2maTeM5Q8zz8dlX+YcSsVVxwn6b9K3UK PnTB3lRLg2VJXx8QhbhC1o9zQEclwjjB4FbwHlL9s2BC71jdiY4+8iT91g2on0Er4d7UDGUC8/ VhA=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.