Re: [RFC PATCH 1/2] libxl: add Function class to IDL

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

On Fri, Aug 14, 2020 at 11:52:33AM +0100, Anthony PERARD wrote:
> On Mon, Jul 27, 2020 at 09:26:32AM -0400, Nick Rosbrook wrote:
> > Add a Function and CtxFunction classes to idl.py to allow generator
> > scripts to generate wrappers which are repetitive and straight forward
> > when doing so by hand. Examples of such functions are the
> > device_add/remove functions.
> > 
> > To start, a Function has attributes for namespace, name, parameters,
> > return type, and an indication if the return value should be interpreted as
> > a status code. The CtxFunction class extends this by indicating that a
> > libxl_ctx is a required parmeter, and can optionally be an async
> > function.
> > 
> > Also, add logic to idl.parse to return the list of functions found in an
> > IDL file. For now, have users of idl.py -- i.e. libxl/gentypes.py and
> > golang/xenlight/gengotypes.py -- ignore the list of functions returned.
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> > ---
> >  
> > +class Function(object):
> > +    """
> > +    A general description of a function signature.
> > +
> > +    Attributes:
> > +      name (str): name of the function, excluding namespace.
> > +      params (list of (str,Type)): list of function parameters.
> > +      return_type (Type): the Type (if any), returned by the function.
> > +      return_is_status (bool): Indicates that the return value should be
> > +                               interpreted as an error/status code.
> 
> Can we get away without `return_is_status`? Couldn't we try to have
> return_type=libxl_error to indicate that return is a kind of status?
> 
Yes, I think that is much better.

> > +    """
> > +class CtxFunction(Function):
> > +    """
> > +    A function that requires a libxl_ctx.
> > +
> > +    Attributes:
> > +      is_asyncop (bool): indicates that the function accepts a
> > +                         libxl_asyncop_how parameter.
> 
> While CtxFunction can be a function that takes `libxl_ctx` as first
> parameter, I don't think `is_asyncop` can be used. We can't know if
> `ao_how` will be last or not. For some function, `ao_how` is second to
> last. So, I guess `ao_how` might need to be listed in `params`
> 
> What do you think?
That's a good point. Do you think it would make sense to add `Builtin`
definitions to libxl_types.idl for `libxl_asyncop_how`,
`libxl_asyncprogress_how`, etc.? That way the generation scripts could
work with those types more easily. But, I guess since those definitions
aren't known until parse time we couldn't use them in the
`DeviceFunction` class definition (but maybe that's not a big deal).

Thank you for the feedback.

-NR