Re: [RFC PATCH 2/2] libxl: prototype libxl_device_nic_add/remove with IDL

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

On Fri, Aug 14, 2020 at 11:57:47AM +0100, Anthony PERARD wrote:
> On Mon, Jul 27, 2020 at 09:26:33AM -0400, Nick Rosbrook wrote:
> > Add a DeviceFunction class and describe prototypes for
> > libxl_device_nic_add/remove in libxl_types.idl.
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> > --
> > This is mostly to serve as an example of how the first patch would be
> > used for function support in the IDL.
> > ---
> >  tools/libxl/idl.py          | 8 ++++++++
> >  tools/libxl/libxl_types.idl | 6 ++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
> > index 1839871f86..15085af8c7 100644
> > --- a/tools/libxl/idl.py
> > +++ b/tools/libxl/idl.py
> > @@ -386,6 +386,14 @@ class CtxFunction(Function):
> >  
> >          Function.__init__(self, name, params, return_type, 
> > return_is_status)
> >  
> > +class DeviceFunction(CtxFunction):
> > +    """ A function that modifies a device. """
> 
> I guess that meant to be used by all function generated with the C macro
> LIBXL_DEFINE_DEVICE_ADD() and LIBXL_DEFINE_DEVICE_REMOVE(), isn't it?
Yes, I think this could be used in place of those macros.
> 
> I wonder if if we could get away with the type of device ("nic") and the
> type of the parameter (`libxl_device_nic`) and have DeviceFunction been
> a generator for both `add` and `remove` functions (and `destroy`).

We could do that, but I think for clarity it might be valuable to
explicitly define each of them. Actually, as I look at this patch again
I wonder if it would be better to define `Device{Add,Remove,Destroy}`
class definitions?

> Also there are functions like libxl_devid_to_device_nic() aren't those
> of type DeviceFunction as well ? But they don't takes any `ao_how`.
> 
> There is also `libxl_device_nic_list{,_free}`, but it is to handle a
> list of libxl_device_*, so it could be kind of related to DeviceFunction, but
> not quite. But maybe I'm going to far :-).

I think this gives another good reason to define more specific `Device*`
classes, rather than a broad `DeviceFunction` class. What do you think?

Thanks,

-NR