[RFC PATCH 0/2] add function support to IDL

[RFC PATCH 0/2] add function support to IDL

[Nick Rosbrook]

At a Xen Summit design session for the golang bindings (see [1]), we
agreed that it would be beneficial to expand the libxl IDL with function
support. In addition to benefiting libxl itself, this would allow other
language bindings to easily generate function wrappers.

These RFC patches outline a potential strategy for accomplishing this
goal. The first patch adds the Function and CtxFunction classes to
libxl/idl.py, introducing the idea of functions to the IDL. The second
patch adds a DeviceFunction class and adds some sample definitions to
libxl/libxl_types.idl for example purposes.

[1] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg00964.html

Nick Rosbrook (2):
  libxl: add Function class to IDL
  libxl: prototype libxl_device_nic_add/remove with IDL

 tools/golang/xenlight/gengotypes.py |  2 +-
 tools/libxl/gentypes.py             |  2 +-
 tools/libxl/idl.py                  | 54 ++++++++++++++++++++++++++++-
 tools/libxl/libxl_types.idl         |  6 ++++
 4 files changed, 61 insertions(+), 3 deletions(-)

-- 
2.17.1

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

[Nick Rosbrook]

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@ainfosec.com>
---
 tools/golang/xenlight/gengotypes.py |  2 +-
 tools/libxl/gentypes.py             |  2 +-
 tools/libxl/idl.py                  | 46 ++++++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
index 557fecd07b..bd3663c9ea 100644
--- a/tools/golang/xenlight/gengotypes.py
+++ b/tools/golang/xenlight/gengotypes.py
@@ -718,7 +718,7 @@ def xenlight_golang_fmt_name(name, exported = True):
 if __name__ == '__main__':
     idlname = sys.argv[1]
 
-    (builtins, types) = idl.parse(idlname)
+    (builtins, types, _) = idl.parse(idlname)
 
     for b in builtins:
         name = b.typename
diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 9a45e45acc..ac7306f3f7 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -592,7 +592,7 @@ if __name__ == '__main__':
 
     (_, idlname, header, header_private, header_json, impl) = sys.argv
 
-    (builtins,types) = idl.parse(idlname)
+    (builtins,types,_) = idl.parse(idlname)
 
     print("outputting libxl type definitions to %s" % header)
 
diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
index d7367503b4..1839871f86 100644
--- a/tools/libxl/idl.py
+++ b/tools/libxl/idl.py
@@ -347,6 +347,45 @@ class OrderedDict(dict):
     def ordered_items(self):
         return [(x,self[x]) for x in self.__ordered]
 
+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.
+    """
+    def __init__(self, name=None, params=None, return_type=None,
+                 return_is_status=False, namespace=None):
+
+        if namespace is None:
+            self.namespace = _get_default_namespace()
+        else:
+            self.namespace = namespace
+
+        self.name = self.namespace + name
+        self.params = params
+        self.return_type = return_type
+        self.return_is_status = return_is_status
+
+class CtxFunction(Function):
+    """
+    A function that requires a libxl_ctx.
+
+    Attributes:
+      is_asyncop (bool): indicates that the function accepts a
+                         libxl_asyncop_how parameter.
+    """
+    def __init__(self, name=None, params=None, return_type=None,
+                 return_is_status=False, is_asyncop=False):
+
+        self.is_asyncop = is_asyncop
+
+        Function.__init__(self, name, params, return_type, return_is_status)
+
 def parse(f):
     print("Parsing %s" % f, file=sys.stderr)
 
@@ -358,6 +397,10 @@ def parse(f):
             globs[n] = t
         elif isinstance(t,type(object)) and issubclass(t, Type):
             globs[n] = t
+        elif isinstance(t, Function):
+            globs[n] = t
+        elif isinstance(t,type(object)) and issubclass(t, Function):
+            globs[n] = t
         elif n in ['PASS_BY_REFERENCE', 'PASS_BY_VALUE',
                    'DIR_NONE', 'DIR_IN', 'DIR_OUT', 'DIR_BOTH',
                    'namespace', 'hidden']:
@@ -370,8 +413,9 @@ def parse(f):
                           % (e.lineno, f, e.text))
 
     types = [t for t in locs.ordered_values() if isinstance(t,Type)]
+    funcs = [f for f in locs.ordered_values() if isinstance(f,Function)]
 
     builtins = [t for t in types if isinstance(t,Builtin)]
     types = [t for t in types if not isinstance(t,Builtin)]
 
-    return (builtins,types)
+    return (builtins,types,funcs)
-- 
2.17.1

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

[Anthony PERARD]

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@ainfosec.com>
> ---
>  
> +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?

> +    """
> +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?

Thanks,

-- 
Anthony PERARD

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

[Nick Rosbrook]

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@ainfosec.com>
> > ---
> >  
> > +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

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

[Nick Rosbrook]

Add a DeviceFunction class and describe prototypes for
libxl_device_nic_add/remove in libxl_types.idl.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
--
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. """
+    def __init__(self, name=None, device_param=None):
+        params = [ ("domid", uint32), device_param ]
+
+        CtxFunction.__init__(self, name=name, params=params, return_type=integer,
+                             return_is_status=True, is_asyncop=True)
+
 def parse(f):
     print("Parsing %s" % f, file=sys.stderr)
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9d3f05f399..22ba93ee4b 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -769,6 +769,12 @@ libxl_device_nic = Struct("device_nic", [
     ("colo_checkpoint_port", string)
     ])
 
+libxl_device_nic_add = DeviceFunction("device_nic_add",
+    ("nic", libxl_device_nic))
+
+libxl_device_nic_remove = DeviceFunction("device_nic_remove",
+    ("nic", libxl_device_nic))
+
 libxl_device_pci = Struct("device_pci", [
     ("func",      uint8),
     ("dev",       uint8),
-- 
2.17.1

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

[Anthony PERARD]

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@ainfosec.com>
> --
> 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?

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`).

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 :-).

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> +libxl_device_nic_add = DeviceFunction("device_nic_add",
> +    ("nic", libxl_device_nic))
> +
> +libxl_device_nic_remove = DeviceFunction("device_nic_remove",
> +    ("nic", libxl_device_nic))
> +

Thanks,

-- 
Anthony PERARD

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

[Nick Rosbrook]

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@ainfosec.com>
> > --
> > 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