[RFC v2 0/7] add function support to IDL

[RFC v2 0/7] 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.

The first version of this RFC is quite old [1]. I did address comments
on the original RFC, but also expanded the scope a bit. As a way to
evaluate function support, I worked on using this addition to the IDL to
generate device add/remove/destroy functions, and removing the
corresponding macros in libxl_internal.h. However, I stopped short of
actually completing a build with this in place, as I thought it made
sense to get feedback on the idea before working on the next step.

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

Nick Rosbrook (7):
  libxl: remove extra whitespace from gentypes.py
  libxl: add Function class to IDL
  libxl: add PASS_BY_CONST_REFERENCE to idl
  libxl: add DeviceFunction classes to IDL
  libxl: add device function definitions to libxl_types.idl
  libxl: implement device add/remove/destroy functions generation
  libxl: replace LIBXL_DEFINE_DEVICE* macro usage with generated code

 tools/golang/xenlight/gengotypes.py |   2 +-
 tools/libs/light/gentypes.py        | 107 +++++++++++++--
 tools/libs/light/idl.py             |  69 +++++++++-
 tools/libs/light/libxl_9pfs.c       |   2 -
 tools/libs/light/libxl_console.c    |   2 -
 tools/libs/light/libxl_disk.c       |   2 -
 tools/libs/light/libxl_nic.c        |   2 -
 tools/libs/light/libxl_pvcalls.c    |   2 -
 tools/libs/light/libxl_types.idl    | 202 ++++++++++++++++++++++++++++
 tools/libs/light/libxl_usb.c        |   3 -
 tools/libs/light/libxl_vdispl.c     |   2 -
 tools/libs/light/libxl_vkb.c        |   1 -
 tools/libs/light/libxl_vsnd.c       |   2 -
 tools/libs/light/libxl_vtpm.c       |   2 -
 14 files changed, 367 insertions(+), 33 deletions(-)

-- 
2.17.1

[RFC v2 1/7] libxl: remove extra whitespace from gentypes.py

[Nick Rosbrook]

No functional change, just remove the extra whitespace from gentypes.py.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/libs/light/gentypes.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libs/light/gentypes.py b/tools/libs/light/gentypes.py
index 9a45e45acc..37de0f68b3 100644
--- a/tools/libs/light/gentypes.py
+++ b/tools/libs/light/gentypes.py
@@ -40,7 +40,7 @@ def libxl_C_type_define(ty, indent = ""):
     elif isinstance(ty, idl.Aggregate):
         if isinstance(ty, idl.KeyedUnion):
             s += libxl_C_instance_of(ty.keyvar.type, ty.keyvar.name) + ";\n"
-            
+
         if ty.typename is None:
             s += "%s {\n" % ty.kind
         else:
@@ -48,7 +48,7 @@ def libxl_C_type_define(ty, indent = ""):
 
         for f in ty.fields:
             if isinstance(ty, idl.KeyedUnion) and f.type is None: continue
-            
+
             x = libxl_C_instance_of(f.type, f.name)
             if f.const:
                 x = "const " + x
@@ -164,7 +164,7 @@ def libxl_init_members(ty, nesting = 0):
         return [f for f in ty.fields if not f.const and isinstance(f.type,idl.KeyedUnion)]
     else:
         return []
-    
+
 def libxl_C_type_do_init(ty, pass_arg, need_zero=True, indent="    "):
     s=indent
     if ty.init_val is not None:
@@ -229,20 +229,20 @@ def libxl_C_type_member_init(ty, field):
         raise Exception("Only KeyedUnion is supported for member init")
 
     ku = field.type
-    
+
     s = ""
     s += "void %s(%s, %s)\n" % (ty.init_fn + "_" + ku.keyvar.name,
                                 ty.make_arg("p", passby=idl.PASS_BY_REFERENCE),
                                 ku.keyvar.type.make_arg(ku.keyvar.name))
     s += "{\n"
-    
+
     if ku.keyvar.init_val is not None:
         init_val = ku.keyvar.init_val
     elif ku.keyvar.type.init_val is not None:
         init_val = ku.keyvar.type.init_val
     else:
         init_val = None
-        
+
     (nparent,fexpr) = ty.member(ty.pass_arg("p"), ku.keyvar, isref=True)
     if init_val is not None:
         s += "    assert(%s == %s);\n" % (fexpr, init_val)
@@ -732,7 +732,7 @@ if __name__ == '__main__':
         f.write(libxl_C_type_copy(ty, "dst", "src"))
         f.write("}\n")
         f.write("\n")
-        
+
     for ty in [t for t in types if t.copy_deprecated_fn]:
         f.write("int %s(libxl_ctx *ctx, %s)\n" % (ty.copy_deprecated_fn,
             ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
-- 
2.17.1

[RFC v2 2/7] 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,
and return type. The CtxFunction class extends this by indicating that a
libxl_ctx is a required parmeter.

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/libs/light/gentypes.py        |  2 +-
 tools/libs/light/idl.py             | 36 ++++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/tools/golang/xenlight/gengotypes.py b/tools/golang/xenlight/gengotypes.py
index 3e40c3d5dc..484e121746 100644
--- a/tools/golang/xenlight/gengotypes.py
+++ b/tools/golang/xenlight/gengotypes.py
@@ -725,7 +725,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/libs/light/gentypes.py b/tools/libs/light/gentypes.py
index 37de0f68b3..f9957b79a2 100644
--- a/tools/libs/light/gentypes.py
+++ b/tools/libs/light/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/libs/light/idl.py b/tools/libs/light/idl.py
index d7367503b4..20278c272a 100644
--- a/tools/libs/light/idl.py
+++ b/tools/libs/light/idl.py
@@ -347,6 +347,35 @@ 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.
+    """
+    def __init__(self, name=None, params=None, return_type=None, namespace=None):
+
+        if namespace is None:
+            self.namespace = _get_default_namespace()
+        else:
+            self.namespace = namespace
+
+        self.name = self.namespace + name
+        self.rawname = name
+        self.params = params
+        self.return_type = return_type
+
+class CtxFunction(Function):
+    """ A function that requires a libxl_ctx. """
+    def __init__(self, name=None, params=None, return_type=None):
+        ctx = Builtin("ctx", passby=PASS_BY_REFERENCE)
+        params.insert(0, ("ctx", ctx))
+
+        Function.__init__(self, name, params, return_type)
+
 def parse(f):
     print("Parsing %s" % f, file=sys.stderr)
 
@@ -358,6 +387,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 +403,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

[RFC v2 3/7] libxl: add PASS_BY_CONST_REFERENCE to idl

[Nick Rosbrook]

Currently, there is only support for PASS_BY_{REFERENCE,VALUE} in the IDL.
As a piece of adding function support, add logic for PASS_BY_CONST_REFERENCE
so that function generation code can use Type.make_arg() for function
signatures that require const reference parameters.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/libs/light/idl.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/libs/light/idl.py b/tools/libs/light/idl.py
index 20278c272a..a8a4768efc 100644
--- a/tools/libs/light/idl.py
+++ b/tools/libs/light/idl.py
@@ -4,6 +4,7 @@ import sys
 
 PASS_BY_VALUE = 1
 PASS_BY_REFERENCE = 2
+PASS_BY_CONST_REFERENCE = 3
 
 DIR_NONE = 0
 DIR_IN   = 1
@@ -40,7 +41,7 @@ class Type(object):
             raise ValueError
 
         self.passby = kwargs.setdefault('passby', PASS_BY_VALUE)
-        if self.passby not in [PASS_BY_VALUE, PASS_BY_REFERENCE]:
+        if self.passby not in [PASS_BY_VALUE, PASS_BY_REFERENCE, PASS_BY_CONST_REFERENCE]:
             raise ValueError
 
         self.private = kwargs.setdefault('private', False)
@@ -109,6 +110,8 @@ class Type(object):
 
         if passby == PASS_BY_REFERENCE:
             return "%s *%s" % (self.typename, n)
+        elif passby == PASS_BY_CONST_REFERENCE:
+            return "const %s *%s" % (self.typename, n)
         else:
             return "%s %s" % (self.typename, n)
 
@@ -116,7 +119,7 @@ class Type(object):
         if passby is None: passby = self.passby
         if isref is None: isref = self.passby == PASS_BY_REFERENCE
 
-        if passby == PASS_BY_REFERENCE:
+        if passby in [PASS_BY_REFERENCE, PASS_BY_CONST_REFERENCE]:
             if isref:
                 return "%s" % (n)
             else:
@@ -272,7 +275,7 @@ class KeyedUnion(Aggregate):
             raise ValueError
 
         kv_kwargs = dict([(x.lstrip('keyvar_'),y) for (x,y) in kwargs.items() if x.startswith('keyvar_')])
-        
+
         self.keyvar = Field(keyvar_type, keyvar_name, **kv_kwargs)
 
         for f in fields:
@@ -392,6 +395,7 @@ def parse(f):
         elif isinstance(t,type(object)) and issubclass(t, Function):
             globs[n] = t
         elif n in ['PASS_BY_REFERENCE', 'PASS_BY_VALUE',
+                   'PASS_BY_CONST_REFERENCE',
                    'DIR_NONE', 'DIR_IN', 'DIR_OUT', 'DIR_BOTH',
                    'namespace', 'hidden']:
             globs[n] = t
-- 
2.17.1

[RFC v2 4/7] libxl: add DeviceFunction classes to IDL

[Nick Rosbrook]

Add a DeviceFunction base class, and Device{Add,Remove,Destroy}Function
classes to idl.py. These classes will allow the device functions to be
generated later in gentypes.py.

The base class, DeviceFunction, extends CtxFunction and adds a domid
parameter to the function's parameter list. When creating a DeviceFunction,
the device parameter must be specified, and extra parameters can be specified
if necessary.

Rather than indicating specific device actions in the DeviceFunction
class, child classes are created for each device function type. Right
now, DeviceAddFunction does not extend the base class. DeviceRemoveFunction
adds the option of specifying 'custom_remove' parameter when custom remove
functions are needed. DeviceDestroyFunction is a child of DeviceRemoveFunction
to inherit the custom_remove attribute.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/libs/light/idl.py | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/libs/light/idl.py b/tools/libs/light/idl.py
index a8a4768efc..570b168079 100644
--- a/tools/libs/light/idl.py
+++ b/tools/libs/light/idl.py
@@ -379,6 +379,29 @@ class CtxFunction(Function):
 
         Function.__init__(self, name, params, return_type)
 
+class DeviceFunction(CtxFunction):
+    """ A function that modifies a domain's devices. """
+    def __init__(self, name=None, device_param=None, extra_params=None,
+                 return_type=None):
+        self.device_param = device_param
+
+        params = [ ("domid", uint32), device_param ] + extra_params
+
+        CtxFunction.__init__(self, name, params, return_type)
+
+class DeviceAddFunction(DeviceFunction):
+    pass
+
+class DeviceRemoveFunction(DeviceFunction):
+    def __init__(self, name=None, device_param=None, extra_params=None,
+                 return_type=None, custom_remove=None):
+        self.custom_remove = custom_remove
+
+        DeviceFunction.__init__(self, name, device_param, extra_params, return_type)
+
+class DeviceDestroyFunction(DeviceRemoveFunction):
+    pass
+
 def parse(f):
     print("Parsing %s" % f, file=sys.stderr)
 
-- 
2.17.1

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

[Nick Rosbrook]

Add definitions to libxl_type.idl for the device functions required by
each device. In addition, add a Builtin definition for libxl_asyncop_how,
since each device function requires this as a parameter.

This is still prepatory work, and a later commit will make use of these
new definitions to generate the code from gentypes.py.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/libs/light/libxl_types.idl | 202 +++++++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

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
@@ -33,6 +33,8 @@ libxl_hwcap = Builtin("hwcap", passby=PASS_BY_REFERENCE, json_parse_type="JSON_A
 libxl_ms_vm_genid = Builtin("ms_vm_genid", passby=PASS_BY_REFERENCE, check_default_fn="libxl_ms_vm_genid_is_zero",
                             copy_fn="libxl_ms_vm_genid_copy")
 
+libxl_asyncop_how = Builtin("asyncop_how", passby=PASS_BY_CONST_REFERENCE)
+
 #
 # Specific integer types
 #
@@ -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),
@@ -684,6 +704,24 @@ libxl_device_vkb = Struct("device_vkb", [
     ("multi_touch_num_contacts", uint32)
     ])
 
+libxl_device_vkb_add = DeviceAddFunction("device_vkb_add",
+    device_param=("vkb", libxl_device_vkb),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vkb_remove = DeviceRemoveFunction("device_vkb_remove",
+    device_param=("vkb", libxl_device_vkb),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vkb_destroy = DeviceAddFunction("device_vkb_destroy",
+    device_param=("vkb", libxl_device_vkb),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_disk = Struct("device_disk", [
     ("backend_domid", libxl_domid),
     ("backend_domname", string),
@@ -708,6 +746,24 @@ libxl_device_disk = Struct("device_disk", [
     ("hidden_disk", string)
     ])
 
+libxl_device_disk_add = DeviceAddFunction("device_disk_add",
+    device_param=("disk", libxl_device_disk),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_disk_remove = DeviceRemoveFunction("device_disk_remove",
+    device_param=("disk", libxl_device_disk),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_disk_destroy = DeviceDestroyFunction("device_disk_destroy",
+    device_param=("disk", libxl_device_disk),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_nic = Struct("device_nic", [
     ("backend_domid", libxl_domid),
     ("backend_domname", string),
@@ -776,6 +832,24 @@ libxl_device_nic = Struct("device_nic", [
     ("colo_checkpoint_port", string)
     ])
 
+libxl_device_nic_add = DeviceAddFunction("device_nic_add",
+    device_param=("nic", libxl_device_nic),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_nic_remove = DeviceRemoveFunction("device_nic_remove",
+    device_param=("nic", libxl_device_nic),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_nic_destroy = DeviceDestroyFunction("device_nic_destroy",
+    device_param=("nic", libxl_device_nic),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_pci = Struct("device_pci", [
     ("func", uint8),
     ("dev", uint8),
@@ -791,6 +865,24 @@ libxl_device_pci = Struct("device_pci", [
     ("name", string),
     ])
 
+libxl_device_pci_add = DeviceAddFunction("device_pci_add",
+    device_param=("pci", libxl_device_pci),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_pci_remove = DeviceRemoveFunction("device_pci_remove",
+    device_param=("pci", libxl_device_pci),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_pci_destroy = DeviceDestroyFunction("device_pci_destroy",
+    device_param=("pci", libxl_device_pci),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_rdm = Struct("device_rdm", [
     ("start", uint64),
     ("size", uint64),
@@ -817,6 +909,26 @@ libxl_device_usbctrl = Struct("device_usbctrl", [
     ("backend_domname", string),
    ])
 
+libxl_device_usbctrl_add = DeviceAddFunction("device_usbctrl_add",
+    device_param=("usbctrl", libxl_device_usbctrl),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_usbctrl_remove = DeviceRemoveFunction("device_usbctrl_remove",
+    device_param=("usbctrl", libxl_device_usbctrl),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error,
+    custom_remove="usbctrl"
+)
+
+libxl_device_usbctrl_destroy = DeviceDestroyFunction("device_usbctrl_destroy",
+    device_param=("usbctrl", libxl_device_usbctrl),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error,
+    custom_remove="usbctrl"
+)
+
 libxl_device_usbdev = Struct("device_usbdev", [
     ("ctrl", libxl_devid),
     ("port", integer),
@@ -827,6 +939,18 @@ libxl_device_usbdev = Struct("device_usbdev", [
            ])),
     ])
 
+libxl_device_usbdev_add = DeviceAddFunction("device_usbdev_add",
+    device_param=("usbdev", libxl_device_usbdev),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_usbdev_remove = DeviceRemoveFunction("device_usbdev_remove",
+    device_param=("usbdev", libxl_device_usbdev),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_dtdev = Struct("device_dtdev", [
     ("path", string),
     ])
@@ -838,6 +962,24 @@ libxl_device_vtpm = Struct("device_vtpm", [
     ("uuid",             libxl_uuid),
 ])
 
+libxl_device_vtpm_add = DeviceAddFunction("device_vtpm_add",
+    device_param=("vtpm", libxl_device_vtpm),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vtpm_remove = DeviceRemoveFunction("device_vtpm_remove",
+    device_param=("vtpm", libxl_device_vtpm),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vtpm_destroy = DeviceDestroyFunction("device_vtpm_destroy",
+    device_param=("vtpm", libxl_device_vtpm),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_p9 = Struct("device_p9", [
     ("backend_domid",    libxl_domid),
     ("backend_domname",  string),
@@ -847,12 +989,36 @@ libxl_device_p9 = Struct("device_p9", [
     ("devid",            libxl_devid),
 ])
 
+libxl_device_p9_remove = DeviceRemoveFunction("device_p9_remove",
+    device_param=("p9", libxl_device_p9),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_p9_destroy = DeviceDestroyFunction("device_p9_destroy",
+    device_param=("p9", libxl_device_p9),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_pvcallsif = Struct("device_pvcallsif", [
     ("backend_domid",    libxl_domid),
     ("backend_domname",  string),
     ("devid",            libxl_devid),
 ])
 
+libxl_device_pvcallsif_remove = DeviceRemoveFunction("device_pvcallsif_remove",
+    device_param=("pvcallsif", libxl_device_pvcallsif),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_pvcallsif_destroy = DeviceDestroyFunction("device_pvcallsif_destroy",
+    device_param=("pvcallsif", libxl_device_pvcallsif),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_device_channel = Struct("device_channel", [
     ("backend_domid", libxl_domid),
     ("backend_domname", string),
@@ -879,6 +1045,24 @@ libxl_device_vdispl = Struct("device_vdispl", [
     ("connectors", Array(libxl_connector_param, "num_connectors"))
     ])
 
+libxl_device_vdispl_add = DeviceAddFunction("device_vdispl_add",
+    device_param=("vdispl", libxl_device_vdispl),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vdispl_remove = DeviceRemoveFunction("device_vdispl_remove",
+    device_param=("vdispl", libxl_device_vdispl),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vdispl_destroy = DeviceDestroyFunction("device_vdispl_destroy",
+    device_param=("vdispl", libxl_device_vdispl),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_vsnd_pcm_format = Enumeration("vsnd_pcm_format", [
     (1,  "S8"),
     (2,  "U8"),
@@ -942,6 +1126,24 @@ libxl_device_vsnd = Struct("device_vsnd", [
     ("pcms", Array(libxl_vsnd_pcm, "num_vsnd_pcms"))
     ])
 
+libxl_device_vsnd_add = DeviceAddFunction("device_vsnd_add",
+    device_param=("vsnd", libxl_device_vsnd),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vsnd_remove = DeviceRemoveFunction("device_vsnd_remove",
+    device_param=("vsnd", libxl_device_vsnd),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
+libxl_device_vsnd_destroy = DeviceDestroyFunction("device_vsnd_destroy",
+    device_param=("vsnd", libxl_device_vsnd),
+    extra_params=[("ao_how", libxl_asyncop_how)],
+    return_type=libxl_error
+)
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
-- 
2.17.1

[RFC v2 6/7] libxl: implement device add/remove/destroy functions generation

[Nick Rosbrook]

Use the newly added function support in the IDL to generate the function
definitions for the device add, remove, and destroy functions. The
content of the generated functions is taken from the device fuction
macro framework in libxl_internal.h.

For now, the definitions are not actually written out to a .c file, but
are invoked to ensure there is no build regression introduced. A later
commit will replace the existing macros with this generated code.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/libs/light/gentypes.py | 91 +++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/tools/libs/light/gentypes.py b/tools/libs/light/gentypes.py
index f9957b79a2..9f1856399a 100644
--- a/tools/libs/light/gentypes.py
+++ b/tools/libs/light/gentypes.py
@@ -584,6 +584,85 @@ def libxl_C_enum_from_string(ty, str, e, indent = "    "):
         s = indent + s
     return s.replace("\n", "\n%s" % indent).rstrip(indent)
 
+def libxl_func_define_device_add(func):
+    s = ''
+
+    return_type = func.return_type.typename
+    if isinstance(func.return_type, idl.Enumeration):
+        return_type = idl.integer.typename
+
+    params = ', '.join([ ty.make_arg(name) for (name,ty) in func.params ])
+
+    s += '{0} {1}({2})\n'.format(return_type, func.name, params)
+    s += '{\n'
+    s += '\tAO_CREATE(ctx, domid, ao_how);\n'
+    s += '\tlibxl__ao_device *aodev;\n\n'
+    s += '\tGCNEW(aodev);\n'
+    s += '\tlibxl__prepare_ao_device(ao, aodev);\n'
+    s += '\taodev->action = LIBXL__DEVICE_ACTION_ADD;\n'
+    s += '\taodev->callback = device_addrm_aocomplete;\n'
+    s += '\taodev->update_json = true;\n'
+    s += '\tlibxl__{0}(egc, domid, type, aodev);\n\n'.format(func.rawname)
+    s += '\treturn AO_INPROGRESS;\n'
+    s += '}\n'
+
+    return s
+
+def libxl_func_define_device_remove_ext(func, action=None):
+    s = ''
+
+    flag = None
+    if action == 'remove':
+        flag = 'LIBXL__FORCE_AUTO'
+    elif action == 'destroy':
+        flag = 'LIBXL__FORCE_ON'
+    else:
+        raise Exception('Unsupported action %s' % action)
+
+    # This is used to formulate the function name libxl__device_from_<devtype>
+    devtype = func.device_param[1].rawname.replace('device_','')
+
+    remtype = 'generic'
+    if func.custom_remove is not None:
+        remtype = func.custom_remove
+
+    return_type = func.return_type.typename
+    if isinstance(func.return_type, idl.Enumeration):
+        return_type = idl.integer.typename
+
+    params = ', '.join([ ty.make_arg(name) for (name,ty) in func.params ])
+
+    s += '{0} {1}({2})\n'.format(return_type, func.name, params)
+    s += '{\n'
+    s += '\tAO_CREATE(ctx, domid, ao_how);\n'
+    s += '\tlibxl__device *device;\n'
+    s += '\tlibxl__ao_device *aodev;\n'
+    s += '\tint rc;\n'
+    s += '\n'
+    s += '\tGCNEW(device);\n'
+    s += '\trc = libxl__device_from_{0}(gc, domid, type, device);\n'.format(devtype)
+    s += '\tif (rc != 0) goto out;\n'
+    s += '\n'
+    s += '\tGCNEW(aodev);\n'
+    s += '\tlibxl__prepare_ao_device(ao, aodev);\n'
+    s += '\taodev->action = LIBXL__DEVICE_ACTION_REMOVE;\n'
+    s += '\taodev->dev = device;\n'
+    s += '\taodev->callback = device_addrm_aocomplete;\n'
+    s += '\taodev->force.flag = {0};\n'.format(flag)
+    s += '\tlibxl__initiate_device_{0}_remove(egc, aodev);\n'.format(remtype)
+    s += '\n'
+    s += 'out:\n'
+    s += '\tif (rc) return AO_CREATE_FAIL(rc);\n'
+    s += '\treturn AO_INPROGRESS;\n'
+    s += '}\n'
+
+    return s
+
+def libxl_func_define_device_remove(func):
+    return libxl_func_define_device_remove_ext(func, action='remove')
+
+def libxl_func_define_device_destroy(func):
+    return libxl_func_define_device_remove_ext(func, action='destroy')
 
 if __name__ == '__main__':
     if len(sys.argv) != 6:
@@ -592,7 +671,7 @@ if __name__ == '__main__':
 
     (_, idlname, header, header_private, header_json, impl) = sys.argv
 
-    (builtins,types,_) = idl.parse(idlname)
+    (builtins,types,funcs) = idl.parse(idlname)
 
     print("outputting libxl type definitions to %s" % header)
 
@@ -794,4 +873,14 @@ if __name__ == '__main__':
         f.write("}\n")
         f.write("\n")
 
+    for func in funcs:
+        if type(func) is idl.DeviceAddFunction:
+            _ = libxl_func_define_device_add(func)
+        elif type(func) is idl.DeviceRemoveFunction:
+            _ = libxl_func_define_device_remove(func)
+        elif type(func) is idl.DeviceDestroyFunction:
+            _ = libxl_func_define_device_destroy(func)
+        else:
+            raise Exception("Unexpected Function class %s" % type(func))
+
     f.close()
-- 
2.17.1

[RFC v2 7/7] libxl: replace LIBXL_DEFINE_DEVICE* macro usage with generated code

[Nick Rosbrook]

Allow the gentypes.py script to write generated function output to
_libxl_types.c, and remove the LIBXL_DEFINE_DEVICE* macro calls in the
appropriate locations.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---

Note: This commit does not build; there is more work to be done,
particularly around accessing libxl__device_from* functions, as well as
libxl__device_<type>_add functions.
---
 tools/libs/light/gentypes.py     | 8 +++++---
 tools/libs/light/libxl_9pfs.c    | 2 --
 tools/libs/light/libxl_console.c | 2 --
 tools/libs/light/libxl_disk.c    | 2 --
 tools/libs/light/libxl_nic.c     | 2 --
 tools/libs/light/libxl_pvcalls.c | 2 --
 tools/libs/light/libxl_usb.c     | 3 ---
 tools/libs/light/libxl_vdispl.c  | 2 --
 tools/libs/light/libxl_vkb.c     | 1 -
 tools/libs/light/libxl_vsnd.c    | 2 --
 tools/libs/light/libxl_vtpm.c    | 2 --
 11 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/tools/libs/light/gentypes.py b/tools/libs/light/gentypes.py
index 9f1856399a..5b72b4613e 100644
--- a/tools/libs/light/gentypes.py
+++ b/tools/libs/light/gentypes.py
@@ -874,12 +874,14 @@ if __name__ == '__main__':
         f.write("\n")
 
     for func in funcs:
+        f.write("\n")
+
         if type(func) is idl.DeviceAddFunction:
-            _ = libxl_func_define_device_add(func)
+            f.write(libxl_func_define_device_add(func))
         elif type(func) is idl.DeviceRemoveFunction:
-            _ = libxl_func_define_device_remove(func)
+            f.write(libxl_func_define_device_remove(func))
         elif type(func) is idl.DeviceDestroyFunction:
-            _ = libxl_func_define_device_destroy(func)
+            f.write(libxl_func_define_device_destroy(func))
         else:
             raise Exception("Unexpected Function class %s" % type(func))
 
diff --git a/tools/libs/light/libxl_9pfs.c b/tools/libs/light/libxl_9pfs.c
index 5ab0d3aa21..f4875ea996 100644
--- a/tools/libs/light/libxl_9pfs.c
+++ b/tools/libs/light/libxl_9pfs.c
@@ -43,8 +43,6 @@ static int libxl__set_xenstore_p9(libxl__gc *gc, uint32_t domid,
 static LIBXL_DEFINE_UPDATE_DEVID(p9)
 static LIBXL_DEFINE_DEVICE_FROM_TYPE(p9)
 
-LIBXL_DEFINE_DEVICE_REMOVE(p9)
-
 DEFINE_DEVICE_TYPE_STRUCT(p9, 9PFS, p9s,
     .skip_attach = 1,
     .set_xenstore_config = (device_set_xenstore_config_fn_t)
diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index d8b2bc5465..726bee3b16 100644
--- 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)
diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
index 411ffeaca6..16d2667d3a 100644
--- a/tools/libs/light/libxl_disk.c
+++ b/tools/libs/light/libxl_disk.c
@@ -1320,9 +1320,7 @@ out:
  * libxl_device_disk_destroy
  * libxl_device_disk_safe_remove
  */
-LIBXL_DEFINE_DEVICE_ADD(disk)
 LIBXL_DEFINE_DEVICES_ADD(disk)
-LIBXL_DEFINE_DEVICE_REMOVE(disk)
 LIBXL_DEFINE_DEVICE_SAFE_REMOVE(disk)
 
 static int libxl_device_disk_compare(const libxl_device_disk *d1,
diff --git a/tools/libs/light/libxl_nic.c b/tools/libs/light/libxl_nic.c
index 0b45469dca..f77e1a07a5 100644
--- a/tools/libs/light/libxl_nic.c
+++ b/tools/libs/light/libxl_nic.c
@@ -525,9 +525,7 @@ static LIBXL_DEFINE_UPDATE_DEVID(nic)
 static LIBXL_DEFINE_DEVICE_FROM_TYPE(nic)
 
 LIBXL_DEFINE_DEVID_TO_DEVICE(nic)
-LIBXL_DEFINE_DEVICE_ADD(nic)
 LIBXL_DEFINE_DEVICES_ADD(nic)
-LIBXL_DEFINE_DEVICE_REMOVE(nic)
 
 DEFINE_DEVICE_TYPE_STRUCT(nic, VIF, nics,
     .update_config = libxl_device_nic_update_config,
diff --git a/tools/libs/light/libxl_pvcalls.c b/tools/libs/light/libxl_pvcalls.c
index 1fbedf651c..6816cc3d4d 100644
--- a/tools/libs/light/libxl_pvcalls.c
+++ b/tools/libs/light/libxl_pvcalls.c
@@ -32,6 +32,4 @@ static LIBXL_DEFINE_DEVICE_FROM_TYPE(pvcallsif)
 #define libxl_device_pvcallsif_list NULL
 #define libxl_device_pvcallsif_compare NULL
 
-LIBXL_DEFINE_DEVICE_REMOVE(pvcallsif)
-
 DEFINE_DEVICE_TYPE_STRUCT(pvcallsif, PVCALLS, pvcallsifs);
diff --git a/tools/libs/light/libxl_usb.c b/tools/libs/light/libxl_usb.c
index c5ae59681c..af5230b261 100644
--- a/tools/libs/light/libxl_usb.c
+++ b/tools/libs/light/libxl_usb.c
@@ -547,9 +547,7 @@ static void device_usbctrl_add_done(libxl__egc *egc,
     aodev->callback(egc, aodev);
 }
 
-LIBXL_DEFINE_DEVICE_ADD(usbctrl)
 static LIBXL_DEFINE_DEVICES_ADD(usbctrl)
-LIBXL_DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl)
 
 static int libxl__device_usbdev_list_for_usbctrl(libxl__gc *gc, uint32_t domid,
                                                  libxl_devid usbctrl,
@@ -1865,7 +1863,6 @@ static void device_usbdev_add_done(libxl__egc *egc,
     aodev->callback(egc, aodev);
 }
 
-LIBXL_DEFINE_DEVICE_ADD(usbdev)
 static LIBXL_DEFINE_DEVICES_ADD(usbdev)
 
 static void device_usbdev_remove_timeout(libxl__egc *egc,
diff --git a/tools/libs/light/libxl_vdispl.c b/tools/libs/light/libxl_vdispl.c
index 60427c76c2..13b6c2be99 100644
--- a/tools/libs/light/libxl_vdispl.c
+++ b/tools/libs/light/libxl_vdispl.c
@@ -202,8 +202,6 @@ static LIBXL_DEFINE_UPDATE_DEVID(vdispl)
 static LIBXL_DEFINE_DEVICES_ADD(vdispl)
 
 LIBXL_DEFINE_DEVID_TO_DEVICE(vdispl)
-LIBXL_DEFINE_DEVICE_ADD(vdispl)
-LIBXL_DEFINE_DEVICE_REMOVE(vdispl)
 LIBXL_DEFINE_DEVICE_LIST(vdispl)
 
 DEFINE_DEVICE_TYPE_STRUCT(vdispl, VDISPL, vdispls,
diff --git a/tools/libs/light/libxl_vkb.c b/tools/libs/light/libxl_vkb.c
index bb88059f93..5b552c262f 100644
--- a/tools/libs/light/libxl_vkb.c
+++ b/tools/libs/light/libxl_vkb.c
@@ -334,7 +334,6 @@ static LIBXL_DEFINE_UPDATE_DEVID(vkb)
 #define libxl_device_vkb_compare NULL
 
 LIBXL_DEFINE_DEVICE_LIST(vkb)
-LIBXL_DEFINE_DEVICE_REMOVE(vkb)
 
 DEFINE_DEVICE_TYPE_STRUCT(vkb, VKBD, vkbs,
     .skip_attach = 1,
diff --git a/tools/libs/light/libxl_vsnd.c b/tools/libs/light/libxl_vsnd.c
index bb7942bbc9..16f448c74e 100644
--- a/tools/libs/light/libxl_vsnd.c
+++ b/tools/libs/light/libxl_vsnd.c
@@ -666,8 +666,6 @@ out:
 static LIBXL_DEFINE_UPDATE_DEVID(vsnd)
 static LIBXL_DEFINE_DEVICES_ADD(vsnd)
 
-LIBXL_DEFINE_DEVICE_ADD(vsnd)
-LIBXL_DEFINE_DEVICE_REMOVE(vsnd)
 LIBXL_DEFINE_DEVICE_LIST(vsnd)
 
 DEFINE_DEVICE_TYPE_STRUCT(vsnd, VSND, vsnds,
diff --git a/tools/libs/light/libxl_vtpm.c b/tools/libs/light/libxl_vtpm.c
index 0148c572d4..8dcc965860 100644
--- a/tools/libs/light/libxl_vtpm.c
+++ b/tools/libs/light/libxl_vtpm.c
@@ -227,8 +227,6 @@ static LIBXL_DEFINE_UPDATE_DEVID(vtpm)
 static LIBXL_DEFINE_DEVICE_FROM_TYPE(vtpm)
 static LIBXL_DEFINE_DEVICES_ADD(vtpm)
 
-LIBXL_DEFINE_DEVICE_ADD(vtpm)
-LIBXL_DEFINE_DEVICE_REMOVE(vtpm)
 LIBXL_DEFINE_DEVICE_LIST(vtpm)
 
 DEFINE_DEVICE_TYPE_STRUCT(vtpm, VTPM, vtpms,
-- 
2.17.1

Re: [RFC v2 0/7] add function support to IDL

[Ian Jackson]

Nick Rosbrook writes ("[RFC v2 0/7] add function support to IDL"):
> 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.
> 
> The first version of this RFC is quite old [1]. I did address comments
> on the original RFC, but also expanded the scope a bit. As a way to
> evaluate function support, I worked on using this addition to the IDL to
> generate device add/remove/destroy functions, and removing the
> corresponding macros in libxl_internal.h. However, I stopped short of
> actually completing a build with this in place, as I thought it made
> sense to get feedback on the idea before working on the next step.

This is exciting!  I hope to find time to look at it, but I'm the
release manager for Xen 4.15 and that's taking most of my time right
now.

Regards,
Ian.

Re: [RFC v2 0/7] add function support to IDL

[Nick Rosbrook]

On Wed, Mar 03, 2021 at 09:48:02AM +0000, Ian Jackson wrote:
> Nick Rosbrook writes ("[RFC v2 0/7] add function support to IDL"):
> > 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.
> > 
> > The first version of this RFC is quite old [1]. I did address comments
> > on the original RFC, but also expanded the scope a bit. As a way to
> > evaluate function support, I worked on using this addition to the IDL to
> > generate device add/remove/destroy functions, and removing the
> > corresponding macros in libxl_internal.h. However, I stopped short of
> > actually completing a build with this in place, as I thought it made
> > sense to get feedback on the idea before working on the next step.
> 
> This is exciting!  I hope to find time to look at it, but I'm the
> release manager for Xen 4.15 and that's taking most of my time right
> now.

Of course, I understand. Thank you for expressing interest, I look
forward to hearing your thoughts when time permits.

Thanks,
NR

Re: [RFC v2 0/7] add function support to IDL

[Nick Rosbrook]

On Wed, Mar 3, 2021 at 8:41 AM Nick Rosbrook <rosbrookn@gmail.com> wrote:
>
> On Wed, Mar 03, 2021 at 09:48:02AM +0000, Ian Jackson wrote:
> > Nick Rosbrook writes ("[RFC v2 0/7] add function support to IDL"):
> > > 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.
> > >
> > > The first version of this RFC is quite old [1]. I did address comments
> > > on the original RFC, but also expanded the scope a bit. As a way to
> > > evaluate function support, I worked on using this addition to the IDL to
> > > generate device add/remove/destroy functions, and removing the
> > > corresponding macros in libxl_internal.h. However, I stopped short of
> > > actually completing a build with this in place, as I thought it made
> > > sense to get feedback on the idea before working on the next step.
> >
> > This is exciting!  I hope to find time to look at it, but I'm the
> > release manager for Xen 4.15 and that's taking most of my time right
> > now.
>
> Of course, I understand. Thank you for expressing interest, I look
> forward to hearing your thoughts when time permits.
>
Hi,

Just thought I would send a ping to see if anyone has time to review.

Thanks,
NR

Re: [RFC v2 1/7] libxl: remove extra whitespace from gentypes.py

[Anthony PERARD]

On Tue, Mar 02, 2021 at 08:46:13PM -0500, Nick Rosbrook wrote:
> No functional change, just remove the extra whitespace from gentypes.py.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [RFC v2 6/7] libxl: implement device add/remove/destroy functions generation

[Anthony PERARD]

On Tue, Mar 02, 2021 at 08:46:18PM -0500, Nick Rosbrook wrote:
> +def libxl_func_define_device_add(func):
> +    s = ''
> +
> +    return_type = func.return_type.typename
> +    if isinstance(func.return_type, idl.Enumeration):
> +        return_type = idl.integer.typename
> +
> +    params = ', '.join([ ty.make_arg(name) for (name,ty) in func.params ])
> +
> +    s += '{0} {1}({2})\n'.format(return_type, func.name, params)
> +    s += '{\n'
> +    s += '\tAO_CREATE(ctx, domid, ao_how);\n'
> +    s += '\tlibxl__ao_device *aodev;\n\n'
> +    s += '\tGCNEW(aodev);\n'
> +    s += '\tlibxl__prepare_ao_device(ao, aodev);\n'
> +    s += '\taodev->action = LIBXL__DEVICE_ACTION_ADD;\n'
> +    s += '\taodev->callback = device_addrm_aocomplete;\n'
> +    s += '\taodev->update_json = true;\n'
> +    s += '\tlibxl__{0}(egc, domid, type, aodev);\n\n'.format(func.rawname)
> +    s += '\treturn AO_INPROGRESS;\n'
> +    s += '}\n'

That's kind of hard to read, I think we could use python's triple-quote
(or triple double-quote) ''' or """ to have a multi-line string and
remove all those \t \n
Something like:

    s = '''
    {ret} {func}({params})
    {{
        return ERROR_FAIL;
        libxl__{rawname}(gc);
    }}
    '''.format(ret=return_type, func=func.name, params=params,
               rawname=func.rawname)

That would produce some extra indentation in the generated C file, but
that doesn't matter to me. They could be removed with textwrap.dedent()
if needed.

Thanks,

-- 
Anthony PERARD

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

[Anthony PERARD]

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 ;-)

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.

Cheers,

-- 
Anthony PERARD

Re: [RFC v2 0/7] add function support to IDL

[Anthony PERARD]

On Tue, Mar 02, 2021 at 08:46:12PM -0500, Nick Rosbrook wrote:
> 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.
> 
> The first version of this RFC is quite old [1]. I did address comments
> on the original RFC, but also expanded the scope a bit. As a way to
> evaluate function support, I worked on using this addition to the IDL to
> generate device add/remove/destroy functions, and removing the
> corresponding macros in libxl_internal.h. However, I stopped short of
> actually completing a build with this in place, as I thought it made
> sense to get feedback on the idea before working on the next step.

The series looks good to me, beside a few detail.

Cheers,

-- 
Anthony PERARD

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

[Nick Rosbrook]

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

Re: [RFC v2 6/7] libxl: implement device add/remove/destroy functions generation

[Nick Rosbrook]

On Tue, May 04, 2021 at 04:02:55PM +0100, Anthony PERARD wrote:
> On Tue, Mar 02, 2021 at 08:46:18PM -0500, Nick Rosbrook wrote:
> > +def libxl_func_define_device_add(func):
> > +    s = ''
> > +
> > +    return_type = func.return_type.typename
> > +    if isinstance(func.return_type, idl.Enumeration):
> > +        return_type = idl.integer.typename
> > +
> > +    params = ', '.join([ ty.make_arg(name) for (name,ty) in func.params ])
> > +
> > +    s += '{0} {1}({2})\n'.format(return_type, func.name, params)
> > +    s += '{\n'
> > +    s += '\tAO_CREATE(ctx, domid, ao_how);\n'
> > +    s += '\tlibxl__ao_device *aodev;\n\n'
> > +    s += '\tGCNEW(aodev);\n'
> > +    s += '\tlibxl__prepare_ao_device(ao, aodev);\n'
> > +    s += '\taodev->action = LIBXL__DEVICE_ACTION_ADD;\n'
> > +    s += '\taodev->callback = device_addrm_aocomplete;\n'
> > +    s += '\taodev->update_json = true;\n'
> > +    s += '\tlibxl__{0}(egc, domid, type, aodev);\n\n'.format(func.rawname)
> > +    s += '\treturn AO_INPROGRESS;\n'
> > +    s += '}\n'
> 
> That's kind of hard to read, I think we could use python's triple-quote
> (or triple double-quote) ''' or """ to have a multi-line string and
> remove all those \t \n
> Something like:
> 
>     s = '''
>     {ret} {func}({params})
>     {{
>         return ERROR_FAIL;
>         libxl__{rawname}(gc);
>     }}
>     '''.format(ret=return_type, func=func.name, params=params,
>                rawname=func.rawname)
> 
> That would produce some extra indentation in the generated C file, but
> that doesn't matter to me. They could be removed with textwrap.dedent()
> if needed.
> 
That sounds good to me.

Thanks,
NR

Re: [RFC v2 0/7] add function support to IDL

[Nick Rosbrook]

On Tue, May 04, 2021 at 04:46:52PM +0100, Anthony PERARD wrote:
> On Tue, Mar 02, 2021 at 08:46:12PM -0500, Nick Rosbrook wrote:
> > 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.
> > 
> > The first version of this RFC is quite old [1]. I did address comments
> > on the original RFC, but also expanded the scope a bit. As a way to
> > evaluate function support, I worked on using this addition to the IDL to
> > generate device add/remove/destroy functions, and removing the
> > corresponding macros in libxl_internal.h. However, I stopped short of
> > actually completing a build with this in place, as I thought it made
> > sense to get feedback on the idea before working on the next step.
> 
> The series looks good to me, beside a few detail.
> 
> Cheers,
> 
> -- 
> Anthony PERARD

Thanks for reviewing!

-NR