Re: [RFC v2 5/7] libxl: add device function definitions to libxl_types.idl

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

On Tue, May 04, 2021 at 04:43:27PM +0100, Anthony PERARD wrote:
> On Tue, Mar 02, 2021 at 08:46:17PM -0500, Nick Rosbrook wrote:
> > diff --git a/tools/libs/light/libxl_types.idl 
> > b/tools/libs/light/libxl_types.idl
> > index 5b85a7419f..550af7a1c7 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -666,6 +668,24 @@ libxl_device_vfb = Struct("device_vfb", [
> >      ("keymap",        string),
> >      ])
> >  
> > +libxl_device_vfb_add = DeviceAddFunction("device_vfb_add",
> > +    device_param=("vfb", libxl_device_vfb),
> > +    extra_params=[("ao_how", libxl_asyncop_how)],
> > +    return_type=libxl_error
> > +)
> > +
> > +libxl_device_vfb_remove = DeviceRemoveFunction("device_vfb_remove",
> > +    device_param=("vfb", libxl_device_vfb),
> > +    extra_params=[("ao_how", libxl_asyncop_how)],
> > +    return_type=libxl_error
> > +)
> > +
> > +libxl_device_vfb_destroy = DeviceDestroyFunction("device_vfb_destroy",
> > +    device_param=("vfb", libxl_device_vfb),
> > +    extra_params=[("ao_how", libxl_asyncop_how)],
> > +    return_type=libxl_error
> > +)
> > +
> >  libxl_device_vkb = Struct("device_vkb", [
> >      ("backend_domid", libxl_domid),
> >      ("backend_domname", string),
> 
> In future version of the series that is deem ready, I think it would be
> useful to have this change in libxl_types.idl and the change that remove
> the macro call from the C file in the same patch. It would make it
> possible to review discrepancies.
> 
> The change in the idl for vfb is different that the change in the C
> file:
> 
> > --- a/tools/libs/light/libxl_console.c
> > +++ b/tools/libs/light/libxl_console.c
> > @@ -723,8 +723,6 @@ static LIBXL_DEFINE_UPDATE_DEVID(vfb)
> >  static LIBXL_DEFINE_DEVICE_FROM_TYPE(vfb)
> > 
> >  /* vfb */
> > -LIBXL_DEFINE_DEVICE_REMOVE(vfb)
> > -
> >  DEFINE_DEVICE_TYPE_STRUCT(vfb, VFB, vfbs,
> >      .skip_attach = 1,
> >      .set_xenstore_config = (device_set_xenstore_config_fn_t)
> 
> No add function ;-)
> 

Good catch, thanks. I will consolidate these two patches in the next
version.

> And libxl doesn't build anymore with the last patch applied. They are
> maybe also issues with functions that are static and thus are not
> accessible from other c files.

Yes, I wanted to receive a bit of feedback on the code generation
approach before mangling things to build. As you say, there are
currently static functions in libxl_<device>.c files that will need to
be accessible from the generated C file. I will address this in v3.

Thanks,
NR