[PATCH 00 of 10] libxl python binding updates

[PATCH 00 of 10] libxl python binding updates

[Gianni Tedesco]

This patch-series brings the libxl python binding up to the state where it can
create domains. It also includes several bug-fixes and exporting of additional
libxl functionality such as PCI passthrough.

The changes are all contained within the tools/python tree except for changes
to the libxl IDL and associated processing code. The final patch in the series
exports libxl_domain_config structure via the IDL and removes the redundant
(identical) definitions and declarations from libxl.

Gianni

[PATCH 01 of 10] pyxl: Fix reference counting of Py_(None|True|False)

[Gianni Tedesco]

 tools/python/genwrap.py           |   5 ++++-
 tools/python/xen/lowlevel/xl/xl.c |  12 +++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761665 0
# Node ID 9ae3901ab42fd0458cf6150014e1f376a25493b5
# Parent  719ba64edad1d7226cd927bcfa65fff087544995
pyxl: Fix reference counting of Py_(None|True|False)

The incorrect refcounting causes the python interpreter to crash

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 719ba64edad1 -r 9ae3901ab42f tools/python/genwrap.py
--- a/tools/python/genwrap.py	Tue Jan 11 16:01:05 2011 +0000
+++ b/tools/python/genwrap.py	Tue Jan 11 16:01:05 2011 +0000
@@ -51,7 +51,10 @@ def py_attrib_get(ty, f):
     l.append('static PyObject *py_%s_%s_get(Py_%s *self, void *priv)'%(ty.rawname, f.name, ty.rawname))
     l.append('{')
     if t == TYPE_BOOL:
-        l.append('    return (self->obj.%s) ? Py_True : Py_False;'%f.name)
+        l.append('    PyObject *ret;')
+        l.append('    ret = (self->obj.%s) ? Py_True : Py_False;'%f.name)
+        l.append('    Py_INCREF(ret);')
+        l.append('    return ret;')
     elif t == TYPE_INT:
         l.append('    return genwrap__ll_get(self->obj.%s);'%f.name)
     elif t == TYPE_UINT:
diff -r 719ba64edad1 -r 9ae3901ab42f tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:05 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:05 2011 +0000
@@ -97,8 +97,10 @@ int genwrap__string_set(PyObject *v, cha
 
 PyObject *genwrap__string_get(char **str)
 {
-    if ( NULL == *str )
+    if ( NULL == *str ) {
+        Py_INCREF(Py_None);
         return Py_None;
+    }
     return PyString_FromString(*str);
 }
 
@@ -377,6 +379,7 @@ static PyObject *pyxl_list_domains(XlObj
         if ( NULL == di )
             goto err_mem;
         memcpy(&di->obj, cur, sizeof(di->obj));
+        /* SetItem steals a reference */
         PyList_SetItem(list, i, (PyObject *)di);
     }
 
@@ -413,6 +416,7 @@ static PyObject *pyxl_domain_shutdown(Xl
         PyErr_SetString(xl_error_obj, "cannot shutdown domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -425,6 +429,7 @@ static PyObject *pyxl_domain_destroy(XlO
         PyErr_SetString(xl_error_obj, "cannot destroy domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -437,6 +442,7 @@ static PyObject *pyxl_domain_pause(XlObj
         PyErr_SetString(xl_error_obj, "cannot pause domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -449,6 +455,7 @@ static PyObject *pyxl_domain_unpause(XlO
         PyErr_SetString(xl_error_obj, "cannot unpause domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -462,6 +469,7 @@ static PyObject *pyxl_domain_rename(XlOb
         PyErr_SetString(xl_error_obj, "cannot rename domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -481,6 +489,7 @@ static PyObject *pyxl_pci_add(XlObject *
         PyErr_SetString(xl_error_obj, "cannot add pci device");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -501,6 +510,7 @@ static PyObject *pyxl_pci_del(XlObject *
         PyErr_SetString(xl_error_obj, "cannot remove pci device");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }

[PATCH 02 of 10] pyxl: Un-muddle libxl_domain_(pause|unpause) wrappers

[Gianni Tedesco]

 tools/python/xen/lowlevel/xl/xl.c |  4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761666 0
# Node ID b06b8e4174e1660b775bfee70f132542afe0a62e
# Parent  9ae3901ab42fd0458cf6150014e1f376a25493b5
pyxl: Un-muddle libxl_domain_(pause|unpause) wrappers

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 9ae3901ab42f -r b06b8e4174e1 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:05 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:06 2011 +0000
@@ -546,9 +546,9 @@ static PyMethodDef pyxl_methods[] = {
          "Shutdown a domain"},
     {"domain_destroy", (PyCFunction)pyxl_domain_destroy, METH_VARARGS,
          "Destroy a domain"},
-    {"domain_pause", (PyCFunction)pyxl_domain_unpause, METH_VARARGS,
+    {"domain_pause", (PyCFunction)pyxl_domain_pause, METH_VARARGS,
          "Pause a domain"},
-    {"domain_unpause", (PyCFunction)pyxl_domain_pause, METH_VARARGS,
+    {"domain_unpause", (PyCFunction)pyxl_domain_unpause, METH_VARARGS,
          "Unpause a domain"},
     {"domain_rename", (PyCFunction)pyxl_domain_rename, METH_VARARGS,
          "Rename a domain"},

[PATCH 03 of 10] pyxl: Export relevant integer constants from python wrapper

[Gianni Tedesco]

 tools/python/xen/lowlevel/xl/xl.c |  32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761666 0
# Node ID 26fc86ef80278a10cd9954d7ce7e4221f5015baf
# Parent  b06b8e4174e1660b775bfee70f132542afe0a62e
pyxl: Export relevant integer constants from python wrapper

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r b06b8e4174e1 -r 26fc86ef8027 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:06 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:06 2011 +0000
@@ -651,6 +651,8 @@ static PyTypeObject PyXlType = {
 
 static PyMethodDef xl_methods[] = { { NULL } };
 
+#define  _INT_CONST(m, c) PyModule_AddIntConstant(m, #c, c)
+#define  _INT_CONST_LIBXL(m, c) PyModule_AddIntConstant(m, #c, LIBXL_ ## c)
 PyMODINIT_FUNC initxl(void)
 {
     PyObject *m;
@@ -671,6 +673,36 @@ PyMODINIT_FUNC initxl(void)
     Py_INCREF(xl_error_obj);
     PyModule_AddObject(m, "Error", xl_error_obj);
 
+    _INT_CONST(m, SHUTDOWN_poweroff);
+    _INT_CONST(m, SHUTDOWN_reboot);
+    _INT_CONST(m, SHUTDOWN_suspend);
+    _INT_CONST(m, SHUTDOWN_crash);
+    _INT_CONST(m, SHUTDOWN_watchdog);
+
+    _INT_CONST(m, XENFV);
+    _INT_CONST(m, XENPV);
+
+    _INT_CONST_LIBXL(m, CONSTYPE_SERIAL);
+    _INT_CONST_LIBXL(m, CONSTYPE_PV);
+
+    _INT_CONST_LIBXL(m, CONSBACK_XENCONSOLED);
+    _INT_CONST_LIBXL(m, CONSBACK_IOEMU);
+
+    _INT_CONST(m, PHYSTYPE_QCOW);
+    _INT_CONST(m, PHYSTYPE_QCOW2);
+    _INT_CONST(m, PHYSTYPE_VHD);
+    _INT_CONST(m, PHYSTYPE_AIO);
+    _INT_CONST(m, PHYSTYPE_FILE);
+    _INT_CONST(m, PHYSTYPE_PHY);
+
+    _INT_CONST(m, NICTYPE_IOEMU);
+    _INT_CONST(m, NICTYPE_VIF);
+
+    _INT_CONST_LIBXL(m, EVENT_DOMAIN_DEATH);
+    _INT_CONST_LIBXL(m, EVENT_DISK_EJECT);
+
+    _INT_CONST(m, POWER_BUTTON);
+    _INT_CONST(m, SLEEP_BUTTON);
     genwrap__init(m);
 }

[PATCH 04 of 10] pyxl: Allow subclassing of auto-generated python types

[Gianni Tedesco]

 tools/python/genwrap.py           |  2 +-
 tools/python/xen/lowlevel/xl/xl.c |  2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761667 0
# Node ID 791046eea8ded39ad09f1cf4b8482580e2a42e6a
# Parent  26fc86ef80278a10cd9954d7ce7e4221f5015baf
pyxl: Allow subclassing of auto-generated python types

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 26fc86ef8027 -r 791046eea8de tools/python/genwrap.py
--- a/tools/python/genwrap.py	Tue Jan 11 16:01:06 2011 +0000
+++ b/tools/python/genwrap.py	Tue Jan 11 16:01:07 2011 +0000
@@ -151,7 +151,7 @@ static PyTypeObject Py%s_Type= {
     NULL,                         /* tp_getattro       */
     NULL,                         /* tp_setattro       */
     NULL,                         /* tp_as_buffer      */
-    Py_TPFLAGS_DEFAULT,           /* tp_flags          */
+    Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE, /* tp_flags          */
     "%s",                         /* tp_doc            */
     NULL,                         /* tp_traverse       */
     NULL,                         /* tp_clear          */
diff -r 26fc86ef8027 -r 791046eea8de tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:06 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:07 2011 +0000
@@ -628,7 +628,7 @@ static PyTypeObject PyXlType = {
     NULL,                         /* tp_getattro       */
     NULL,                         /* tp_setattro       */
     NULL,                         /* tp_as_buffer      */
-    Py_TPFLAGS_DEFAULT,           /* tp_flags          */
+    Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE, /* tp_flags          */
     "libxenlight connection",     /* tp_doc            */
     NULL,                         /* tp_traverse       */
     NULL,                         /* tp_clear          */

[PATCH 05 of 10] pyxl: Export PCI passthrough related libxl functions

[Gianni Tedesco]

 tools/python/xen/lowlevel/xl/xl.c |  74 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761667 0
# Node ID fb64e8d377d84f99b0a0bac3d7f04dd62fea25b9
# Parent  791046eea8ded39ad09f1cf4b8482580e2a42e6a
pyxl: Export PCI passthrough related libxl functions

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 791046eea8de -r fb64e8d377d8 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:07 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:07 2011 +0000
@@ -537,6 +537,75 @@ static PyObject *pyxl_pci_parse(XlObject
     return (PyObject *)pci;
 }
 
+static PyObject *pyxl_pci_list_assignable(XlObject *self, PyObject *args)
+{
+    libxl_device_pci *dev;
+    PyObject *list;
+    int nr_dev, i;
+
+    if ( libxl_device_pci_list_assignable(&self->ctx, &dev, &nr_dev) ) {
+        PyErr_SetString(xl_error_obj, "Cannot list assignable devices");
+        return NULL;
+    }
+
+    list = PyList_New(nr_dev);
+    if ( NULL == list )
+        return NULL;
+
+    for(i = 0; i < nr_dev; i++) {
+        Py_device_pci *pd;
+        pd = Pydevice_pci_New();
+        if ( NULL == pd )
+            goto err_mem;
+        memcpy(&pd->obj, &dev[i], sizeof(pd->obj));
+        /* SetItem steals a reference */
+        PyList_SetItem(list, i, (PyObject *)pd);
+    }
+
+    free(dev);
+    return list;
+err_mem:
+    Py_DECREF(list);
+    PyErr_SetString(PyExc_MemoryError, "Allocating PCI device list");
+    return NULL;
+}
+
+static PyObject *pyxl_pci_list(XlObject *self, PyObject *args)
+{
+    libxl_device_pci *dev;
+    PyObject *list;
+    int nr_dev, i, domid;
+
+    if ( !PyArg_ParseTuple(args, "i", &domid) )
+        return NULL;
+
+    if ( libxl_device_pci_list_assigned(&self->ctx, &dev, domid, &nr_dev) ) {
+        PyErr_SetString(xl_error_obj, "Cannot list assignable devices");
+        return NULL;
+    }
+
+    list = PyList_New(nr_dev);
+    if ( NULL == list )
+        return NULL;
+
+    for(i = 0; i < nr_dev; i++) {
+        Py_device_pci *pd;
+        pd = Pydevice_pci_New();
+        if ( NULL == pd )
+            goto err_mem;
+        memcpy(&pd->obj, &dev[i], sizeof(pd->obj));
+        /* SetItem steals a reference */
+        PyList_SetItem(list, i, (PyObject *)pd);
+    }
+
+    free(dev);
+    return list;
+err_mem:
+    Py_DECREF(list);
+    PyErr_SetString(PyExc_MemoryError, "Allocating PCI device list");
+    return NULL;
+}
+
 static PyMethodDef pyxl_methods[] = {
     {"list_domains", (PyCFunction)pyxl_list_domains, METH_NOARGS,
          "List domains"},
@@ -558,6 +627,11 @@ static PyMethodDef pyxl_methods[] = {
          "Remove a pass-through PCI device"},
     {"device_pci_parse_bdf", (PyCFunction)pyxl_pci_parse, METH_VARARGS,
          "Parse pass-through PCI device spec (BDF)"},
+    {"device_pci_list", (PyCFunction)pyxl_pci_list, METH_VARARGS,
+        "List PCI devices assigned to a domain"},
+    {"device_pci_list_assignable",
+        (PyCFunction)pyxl_pci_list_assignable, METH_NOARGS,
+        "List assignable PCI devices"},
     { NULL, NULL, 0, NULL }
 };

[PATCH 06 of 10] pyxl: Updates to builtin-type marshalling functions

[Gianni Tedesco]

 tools/python/xen/lowlevel/xl/xl.c |  23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761667 0
# Node ID f0cb6ba7a4cf52b63cca5595a68568657976d57d
# Parent  fb64e8d377d84f99b0a0bac3d7f04dd62fea25b9
pyxl: Updates to builtin-type marshalling functions

Allow setting a string field to None as a way to zero it out.
Implement setting/getting libx_file_references as strings.
Produce relevant Exceptions marshallers which remain unimplemented.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r fb64e8d377d8 -r f0cb6ba7a4cf tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:07 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:07 2011 +0000
@@ -76,7 +76,7 @@ int genwrap__obj_init(PyObject *self, Py
 int genwrap__string_set(PyObject *v, char **str)
 {
     char *tmp;
-    if ( NULL == v ) {
+    if ( NULL == v || Py_None == v ) {
         free(*str);
         *str = NULL;
         return 0;
@@ -211,6 +211,7 @@ static PyObject *fixed_bytearray_get(con
 
 int attrib__libxl_cpuid_policy_list_set(PyObject *v, libxl_cpuid_policy_list *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Setting cpuid_policy_list");
     return -1;
 }
 
@@ -233,21 +234,29 @@ int attrib__libxl_cpuarray_set(PyObject 
 
 int attrib__libxl_domain_build_state_ptr_set(PyObject *v, libxl_domain_build_state **pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Setting domain_build_state_ptr");
     return -1;
 }
 
 int attrib__libxl_file_reference_set(PyObject *v, libxl_file_reference *pptr)
 {
-    return -1;
+    return genwrap__string_set(v, &pptr->path);
 }
 
 int attrib__libxl_hwcap_set(PyObject *v, libxl_hwcap *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Setting hwcap");
     return -1;
 }
 
 int attrib__libxl_key_value_list_set(PyObject *v, libxl_key_value_list *pptr)
 {
+    if ( *pptr ) {
+        libxl_key_value_list_destroy(pptr);
+        *pptr = NULL;
+    }
+    if ( v == Py_None )
+        return 0;
     return -1;
 }
 
@@ -258,6 +267,7 @@ int attrib__libxl_mac_set(PyObject *v, l
 
 int attrib__libxl_string_list_set(PyObject *v, libxl_string_list *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Setting string_list");
     return -1;
 }
 
@@ -268,11 +278,13 @@ int attrib__libxl_uuid_set(PyObject *v, 
 
 int attrib__struct_in_addr_set(PyObject *v, struct in_addr *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Setting in_addr");
     return -1;
 }
 
 PyObject *attrib__libxl_cpuid_policy_list_get(libxl_cpuid_policy_list *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Getting cpuid_policy_list");
     return NULL;
 }
 
@@ -314,21 +326,24 @@ PyObject *attrib__libxl_cpuarray_get(lib
 
 PyObject *attrib__libxl_domain_build_state_ptr_get(libxl_domain_build_state **pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Getting domain_build_state_ptr");
     return NULL;
 }
 
 PyObject *attrib__libxl_file_reference_get(libxl_file_reference *pptr)
 {
-    return NULL;
+    return genwrap__string_get(&pptr->path);
 }
 
 PyObject *attrib__libxl_hwcap_get(libxl_hwcap *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Getting hwcap");
     return NULL;
 }
 
 PyObject *attrib__libxl_key_value_list_get(libxl_key_value_list *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Getting key_value_list");
     return NULL;
 }
 
@@ -339,6 +354,7 @@ PyObject *attrib__libxl_mac_get(libxl_ma
 
 PyObject *attrib__libxl_string_list_get(libxl_string_list *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Getting string_list");
     return NULL;
 }
 
@@ -349,6 +365,7 @@ PyObject *attrib__libxl_uuid_get(libxl_u
 
 PyObject *attrib__struct_in_addr_get(struct in_addr *pptr)
 {
+    PyErr_SetString(PyExc_NotImplementedError, "Getting in_addr");
     return NULL;
 }

[PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Gianni Tedesco]

 tools/libxl/libxl.idl   |    2 +-
 tools/python/genwrap.py |  184 ++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 155 insertions(+), 31 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761668 0
# Node ID 735df86eab9a758431f9a3927db64e02baae82d6
# Parent  f0cb6ba7a4cf52b63cca5595a68568657976d57d
pyxl: Recursively scan type-tree to produce complete binding boilerplate

Currently the python wrapper generator ignores non-toplevel types in the IDL.
The KeyedUnion implementation is trivial but for structures embedded in
structures the code becomes more complex. The problem is that when assigning to
a structure (a.b = x) either we:
 - copy the c structure from x in to a.b which is unexpected for python
   programmers since everything is by reference not by value.
 - keep a reference to x which 'shadows' a.b and do the copy just before we
   pass the wrapped C structure across the libxl C API boundary.

The latter approach is chosen. The tree of shadow references is maintained in
the auto-generated code and Py_foo_Unshadow() functions are also generated and
exposed for the hand-written part of the xl wrapper to use.

Finally, for anonymous types, a name mangling system is used to translate names
like x.u.hvm.acpi. To support such constructs in python we would need to export
python types for x.u.hvm such as 'xl.anonymous_hvm_0'. Instead of dealing with
this we simply flatten the type tree so that such a field is named
x.u_hvm_apic.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r f0cb6ba7a4cf -r 735df86eab9a tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Tue Jan 11 16:01:07 2011 +0000
+++ b/tools/libxl/libxl.idl	Tue Jan 11 16:01:08 2011 +0000
@@ -115,7 +115,7 @@ libxl_domain_build_info = Struct("domain
                                         ("bootloader_args", string),
                                         ("cmdline", string),
                                         ("ramdisk", libxl_file_reference),
-                                        ("features", string, True),
+                                        ("features", string),
                                         ])),
                  ])),
     ],
diff -r f0cb6ba7a4cf -r 735df86eab9a tools/python/genwrap.py
--- a/tools/python/genwrap.py	Tue Jan 11 16:01:07 2011 +0000
+++ b/tools/python/genwrap.py	Tue Jan 11 16:01:08 2011 +0000
@@ -4,7 +4,7 @@ import sys,os
 
 import libxltypes
 
-(TYPE_BOOL, TYPE_INT, TYPE_UINT, TYPE_STRING) = range(4)
+(TYPE_BOOL, TYPE_INT, TYPE_UINT, TYPE_STRING) = xrange(4)
 
 def py_type(ty):
     if ty == libxltypes.bool or isinstance(ty, libxltypes.BitField) and ty.width == 1:
@@ -18,11 +18,16 @@ def py_type(ty):
         return TYPE_STRING
     return None
 
+def shadow_fields(ty):
+    return filter(lambda x:x.shadow_type is True, ty.fields)
+
 def py_wrapstruct(ty):
     l = []
     l.append('typedef struct {')
     l.append('    PyObject_HEAD;')
     l.append('    %s obj;'%ty.typename);
+    for f in shadow_fields(ty):
+        l.append('    Py_%s *%s_ref;'%(f.type.rawname, f.python_name))
     l.append('}Py_%s;'%ty.rawname)
     l.append('')
     return "\n".join(l) + "\n"
@@ -36,33 +41,59 @@ def py_decls(ty):
     l = []
     l.append('_hidden Py_%s *Py%s_New(void);\n'%(ty.rawname, ty.rawname))
     l.append('_hidden int Py%s_Check(PyObject *self);\n'%ty.rawname)
+    if len(shadow_fields(ty)) > 0:
+        l.append('_hidden void Py%s_Unshadow(Py_%s *self);\n'%(ty.rawname, ty.rawname))
     for f in ty.fields:
         if py_type(f.type) is not None:
             continue
-        l.append('_hidden PyObject *attrib__%s_get(%s *%s);'%(\
-                 fsanitize(f.type.typename), f.type.typename, f.name))
-        l.append('_hidden int attrib__%s_set(PyObject *v, %s *%s);'%(\
-                 fsanitize(f.type.typename), f.type.typename, f.name))
+        l.append('_hidden PyObject *attrib__%s_get(%s *val);'%(\
+                 fsanitize(f.type.typename), f.type.typename))
+        l.append('_hidden int attrib__%s_set(PyObject *v, %s *val);'%(\
+                 fsanitize(f.type.typename), f.type.typename))
     return '\n'.join(l) + "\n"
 
+def union_check(f, ret, prefix = 'self->obj.'):
+    u_check = []
+    if len(f.condlist):
+        cond = '||'.join(map(lambda (expr,name):('!(' + expr + ')')%('%s%s'%(prefix,name)), f.condlist))
+        u_check.append('    if ( %s ) {'%cond)
+        u_check.append('        PyErr_SetString(PyExc_AttributeError, "Union key not selected");')
+        u_check.append('        return %s;'%ret)
+        u_check.append('    }')
+    return u_check
+
 def py_attrib_get(ty, f):
     t = py_type(f.type)
     l = []
-    l.append('static PyObject *py_%s_%s_get(Py_%s *self, void *priv)'%(ty.rawname, f.name, ty.rawname))
+    l.append('static PyObject *py_%s_%s_get(Py_%s *self, void *priv)'%(ty.rawname, f.python_name, ty.rawname))
     l.append('{')
+
+    u_check = union_check(f, 'NULL')
+
     if t == TYPE_BOOL:
         l.append('    PyObject *ret;')
+        l.extend(u_check)
         l.append('    ret = (self->obj.%s) ? Py_True : Py_False;'%f.name)
         l.append('    Py_INCREF(ret);')
         l.append('    return ret;')
     elif t == TYPE_INT:
+        l.extend(u_check)
         l.append('    return genwrap__ll_get(self->obj.%s);'%f.name)
     elif t == TYPE_UINT:
+        l.extend(u_check)
         l.append('    return genwrap__ull_get(self->obj.%s);'%f.name)
     elif t == TYPE_STRING:
+        l.extend(u_check)
         l.append('    return genwrap__string_get(&self->obj.%s);'%f.name)
+    elif f.shadow_type is True:
+        l.append('    PyObject *ret;')
+        l.extend(u_check)
+        l.append('    ret = (self->%s_ref == NULL) ? Py_None : (PyObject *)self->%s_ref;'%(f.python_name, f.python_name))
+        l.append('    Py_INCREF(ret);')
+        l.append('    return ret;')
     else:
         tn = f.type.typename
+        l.extend(u_check)
         l.append('    return attrib__%s_get((%s *)&self->obj.%s);'%(fsanitize(tn), tn, f.name))
     l.append('}')
     return '\n'.join(l) + "\n\n"
@@ -70,14 +101,17 @@ def py_attrib_get(ty, f):
 def py_attrib_set(ty, f):
     t = py_type(f.type)
     l = []
-    l.append('static int py_%s_%s_set(Py_%s *self, PyObject *v, void *priv)'%(ty.rawname, f.name, ty.rawname))
+    l.append('static int py_%s_%s_set(Py_%s *self, PyObject *v, void *priv)'%(ty.rawname, f.python_name, ty.rawname))
     l.append('{')
+    u_check = union_check(f, '-1')
     if t == TYPE_BOOL:
+        l.extend(u_check)
         l.append('    self->obj.%s = (NULL == v || Py_None == v || Py_False == v) ? 0 : 1;'%f.name)
         l.append('    return 0;')
     elif t == TYPE_UINT or t == TYPE_INT:
         l.append('    %slong long tmp;'%(t == TYPE_UINT and 'unsigned ' or ''))
         l.append('    int ret;')
+        l.extend(u_check)
         if t == TYPE_UINT:
             l.append('    ret = genwrap__ull_set(v, &tmp, (%s)~0);'%f.type.typename)
         else:
@@ -86,47 +120,87 @@ def py_attrib_set(ty, f):
         l.append('        self->obj.%s = tmp;'%f.name)
         l.append('    return ret;')
     elif t == TYPE_STRING:
+        l.extend(u_check)
         l.append('    return genwrap__string_set(v, &self->obj.%s);'%f.name)
+    elif f.shadow_type is True:
+        l.extend(u_check)
+        l.append('    if ( !Py%s_Check(v) ) {'%f.type.rawname)
+        l.append('        PyErr_SetString(PyExc_TypeError, "Expected xl.%s");'%f.type.rawname)
+        l.append('        return -1;')
+        l.append('    }')
+        l.append('    if ( self->%s_ref ) {'%f.python_name)
+        l.append('        Py_DECREF(self->%s_ref);'%f.python_name)
+        l.append('    }')
+        l.append('    self->%s_ref = (Py_%s *)v;'%(f.python_name, f.type.rawname))
+        l.append('    Py_INCREF(self->%s_ref);'%f.python_name)
+        l.append('    return 0;')
     else:
         tn = f.type.typename
+        l.extend(u_check)
         l.append('    return attrib__%s_set(v, (%s *)&self->obj.%s);'%(fsanitize(tn), tn, f.name))
     l.append('}')
     return '\n'.join(l) + "\n\n"
 
+def sync_func(ty):
+    pf = shadow_fields(ty)
+    if len(pf) == 0:
+        return ''
+    l = []
+
+    l.append('void Py%s_Unshadow(Py_%s*self)\n'%(ty.rawname, ty.rawname))
+    l.append('{')
+    for f in pf:
+        l.append('    if ( self->%s_ref ) {'%f.python_name)
+        l.append('        Py_%s *x = (Py_%s *)self->%s_ref;'%(f.type.rawname, f.type.rawname, f.python_name))
+        if len(shadow_fields(f.type)):
+            l.append('        Py%s_Unshadow(x);'%f.type.rawname)
+        l.append('        memcpy(&self->obj.%s, &x->obj, sizeof(self->obj.%s));'%(f.name, f.name))
+        l.append('    }else{')
+        l.append('        memset(&self->obj.%s, 0, sizeof(self->obj.%s));'%(f.name, f.name))
+        l.append('    }')
+    l.append('}')
+    l.append('')
+    return '\n'.join(l)
+
 def py_object_def(ty):
     l = []
-    if ty.destructor_fn is not None:
-        dtor = '    %s(&self->obj);\n'%ty.destructor_fn
-    else:
-        dtor = ''
-
-    funcs="""static void Py%(rawname)s_dealloc(Py_%(rawname)s *self)
-{
-%(dtor)s    self->ob_type->tp_free((PyObject *)self);
-}
-
-static int Py%(rawname)s_init(Py_%(rawname)s *self, PyObject *args, PyObject *kwds)
+    funcs="""static int Py%s_init(Py_%s *self, PyObject *args, PyObject *kwds)
 {
     memset(&self->obj, 0, sizeof(self->obj));
     return genwrap__obj_init((PyObject *)self, args, kwds);
 }
 
-static PyObject *Py%(rawname)s_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
+static PyObject *Py%s_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
 {
-    Py_%(rawname)s *self = (Py_%(rawname)s *)type->tp_alloc(type, 0);
+    Py_%s *self = (Py_%s *)type->tp_alloc(type, 0);
     if (self == NULL)
         return NULL;
     memset(&self->obj, 0, sizeof(self->obj));
     return (PyObject *)self;
 }
 
-"""%{'rawname': ty.rawname, 'dtor': dtor}
+"""%tuple(ty.rawname for x in range(5))
+
+    funcs += sync_func(ty)
+
+
+    l.append('static void Py%s_dealloc(Py_%s *self)'%(ty.rawname, ty.rawname))
+    l.append('{')
+    if ty.destructor_fn is not None:
+        l.append('    %s(&self->obj);'%ty.destructor_fn)
+    for f in shadow_fields(ty):
+        l.append('    if ( self->%s_ref ) {'%f.python_name)
+        l.append('        Py_DECREF(self->%s_ref);'%f.python_name)
+        l.append('    }')
+    l.append('self->ob_type->tp_free((PyObject *)self);')
+    l.append('}')
+    l.append('')
 
     l.append('static PyGetSetDef Py%s_getset[] = {'%ty.rawname)
     for f in ty.fields:
-        l.append('    { .name = "%s", '%f.name)
-        l.append('      .get = (getter)py_%s_%s_get, '%(ty.rawname, f.name))
-        l.append('      .set = (setter)py_%s_%s_set },'%(ty.rawname, f.name))
+        l.append('    { .name = "%s", '%f.python_name)
+        l.append('      .get = (getter)py_%s_%s_get, '%(ty.rawname, f.python_name))
+        l.append('      .set = (setter)py_%s_%s_set },'%(ty.rawname, f.python_name))
     l.append('    { .name = NULL }')
     l.append('};')
     struct="""
@@ -196,12 +270,62 @@ def py_initfuncs(types):
     l.append('}')
     return '\n'.join(l) + "\n\n"
 
-def tree_frob(types):
-    ret = types[:]
-    for ty in ret:
-        ty.fields = filter(lambda f:f.name is not None and f.type.typename is not None, ty.fields)
+def dbg_tree(str, indent=0):
+    do_dbg = True
+    if not do_dbg:
+        return
+    print '%s%s'%(''.join('  ' for i in xrange(indent)), str)
+
+# We don't have a good translation for anonymous structures so we just
+# flatten them out recursively and replace '.' with '_'. For example
+# domain_build_info.u.hvm.pae becomes domain_build_info.u_hvm_pae
+def flatten_type(ty, path = None, depth = 0, condvar = []):
+    if not isinstance(ty, libxltypes.Aggregate):
+        return ty.fields
+
+    if path is None:
+        path = ''
+    else:
+        path = path + '.'
+
+    ret = []
+    for f in ty.fields:
+        f.name = path + f.name
+        f.python_name = f.name.replace('.', '_')
+        if isinstance(f.type, libxltypes.Aggregate) and \
+                f.type.typename is not None:
+            f.shadow_type = True
+        else:
+            f.shadow_type = False
+
+        if isinstance(f.type, libxltypes.KeyedUnion):
+            f.type.keyvar_name = path + f.type.keyvar_name
+
+        f.condlist = condvar[:]
+
+        if isinstance(f.type, libxltypes.Aggregate) and \
+                f.type.typename is None:
+            dbg_tree('(%s)'%(f.name), depth + 1)
+            if isinstance(ty, libxltypes.KeyedUnion):
+                condvar.append((f.keyvar_expr, ty.keyvar_name))
+            ret.extend(flatten_type(f.type, f.name, depth + 1, condvar))
+            if isinstance(ty, libxltypes.KeyedUnion):
+                condvar.pop()
+        else:
+            dbg_tree('%s - %s'%(f.name, f.python_name), depth + 1)
+            ret.append(f)
+
+
     return ret
 
+def frob_type(type):
+    dbg_tree('[%s]'%type.typename)
+    type.fields = flatten_type(type)
+    return type
+
+def frob_types(types):
+    return map(frob_type, types)
+
 if __name__ == '__main__':
     if len(sys.argv) < 4:
         print >>sys.stderr, "Usage: genwrap.py <idl> <decls> <defns>"
@@ -210,7 +334,7 @@ if __name__ == '__main__':
     idl = sys.argv[1]
     (_,types) = libxltypes.parse(idl)
 
-    types = tree_frob(types)
+    types = frob_types(types)
 
     decls = sys.argv[2]
     f = open(decls, 'w')
@@ -250,7 +374,7 @@ _hidden int genwrap__ll_set(PyObject *v,
 
 """ % " ".join(sys.argv))
     for ty in types:
-        f.write('/* Internal APU for %s wrapper */\n'%ty.typename)
+        f.write('/* Internal API for %s wrapper */\n'%ty.typename)
         f.write(py_wrapstruct(ty))
         f.write(py_decls(ty))
         f.write('\n')

[PATCH 08 of 10] xl: support array types in IDL

[Gianni Tedesco]

 tools/libxl/gentypes.py           |   30 +++++++-
 tools/libxl/libxltypes.py         |    6 +
 tools/python/genwrap.py           |  156 +++++++++++++++++++++++++++++++------
 tools/python/xen/lowlevel/xl/xl.c |   19 ++++
 4 files changed, 183 insertions(+), 28 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761668 0
# Node ID 9c57b1622bbcc6252e59d18dca5e33673d6fc236
# Parent  735df86eab9a758431f9a3927db64e02baae82d6
xl: support array types in IDL

This is required to auto-generate language bindings for libxl_domain_config.
An Array() types is implemented which causes the IDL header generator to output
a pointer and a count variable for each instance. C destructor functions are
also correctly generated.

They python wrapper part of this patch builds on the 'shadow' references
introduced in the previous patch 'pyxl: Recursively scan type-tree to produce
complete binding boilerplate' This means that array fields remain as python
lists and present with the expected 'by-reference' semantics. The C array is
built during the 'unshadow' process before the wrapped structure is passed
accross the C API boundary in to libxl.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 735df86eab9a -r 9c57b1622bbc tools/libxl/gentypes.py
--- a/tools/libxl/gentypes.py	Tue Jan 11 16:01:08 2011 +0000
+++ b/tools/libxl/gentypes.py	Tue Jan 11 16:01:08 2011 +0000
@@ -32,6 +32,16 @@ def libxl_C_instance_of(ty, instancename
     else:
         return libxl_C_type_of(ty) + " " + instancename
 
+def flatten_arrays(ty):
+    ret = []
+    for f in ty.fields:
+        if isinstance(f.type, libxltypes.Array):
+            ret.append(libxltypes.Field(libxltypes.integer, "num_%s"%f.name))
+            ret.append(libxltypes.Field(libxltypes.Reference(f.type.array_type), f.name))
+        else:
+            ret.append(f)
+    return ret
+    
 def libxl_C_type_define(ty, indent = ""):
     s = ""
     if isinstance(ty, libxltypes.Aggregate):
@@ -43,7 +53,7 @@ def libxl_C_type_define(ty, indent = "")
         else:
             s += "typedef %s {\n" % ty.kind
 
-        for f in ty.fields:
+        for f in flatten_arrays(ty):
             if f.comment is not None:
                 s += format_comment(4, f.comment)
             x = libxl_C_instance_of(f.type, f.name)
@@ -59,6 +69,15 @@ def libxl_C_type_define(ty, indent = "")
         raise NotImplementedError("%s" % type(ty))
     return s.replace("\n", "\n%s" % indent)
 
+def contains_array_dtor(ty, parent = None):
+    if isinstance(ty, libxltypes.Struct) and (parent is None or ty.destructor_fn is None):
+        for f in [f for f in ty.fields if not f.const]:
+            if isinstance(f.type, libxltypes.Array) and f.type.array_type.destructor_fn:
+                return True
+        if contains_array_dtor(f.type, True):
+            return True
+    return False
+
 def libxl_C_type_destroy(ty, v, reference, indent = "    ", parent = None):
     if reference:
         deref = v + "->"
@@ -85,6 +104,13 @@ def libxl_C_type_destroy(ty, v, referenc
             s += "%s(%s);\n" % (ty.destructor_fn, makeref + v)
     elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.destructor_fn is None):
         for f in [f for f in ty.fields if not f.const]:
+            if isinstance(f.type, libxltypes.Array):
+                at = f.type.array_type
+                if at.destructor_fn:
+                    s += "for(i = 0; i < %s; i++)\n"%(deref + "num_" + f.name)
+                    s += "%s%s(&%s%s[i]);\n"%(indent, at.destructor_fn, deref, f.name)
+                    s += "free(%s%s);\n"%(deref, f.name)
+                continue
 
             if f.name is None: # Anonynous struct
                 s += libxl_C_type_destroy(f.type, deref, False, "", deref)
@@ -158,6 +184,8 @@ if __name__ == '__main__':
     for ty in [t for t in types if t.destructor_fn is not None and t.autogenerate_destructor]:
         f.write("void %s(%s *p)\n" % (ty.destructor_fn, ty.typename))
         f.write("{\n")
+        if contains_array_dtor(ty):
+            f.write("    int i;\n")
         f.write(libxl_C_type_destroy(ty, "p", True))
         f.write("    memset(p, LIBXL_DTOR_POISON, sizeof(*p));\n")
         f.write("}\n")
diff -r 735df86eab9a -r 9c57b1622bbc tools/libxl/libxltypes.py
--- a/tools/libxl/libxltypes.py	Tue Jan 11 16:01:08 2011 +0000
+++ b/tools/libxl/libxltypes.py	Tue Jan 11 16:01:08 2011 +0000
@@ -69,6 +69,12 @@ class Field(object):
         self.comment = kwargs.setdefault('comment', None)
         self.keyvar_expr = kwargs.setdefault('keyvar_expr', None)
 
+class Array(Type):
+    """A counted array of elements"""
+    def __init__(self, type):
+        Type.__init__(self, None)
+        self.array_type = type
+
 class Aggregate(Type):
     """A type containing a collection of other types"""
     def __init__(self, kind, typename, fields, **kwargs):
diff -r 735df86eab9a -r 9c57b1622bbc tools/python/genwrap.py
--- a/tools/python/genwrap.py	Tue Jan 11 16:01:08 2011 +0000
+++ b/tools/python/genwrap.py	Tue Jan 11 16:01:08 2011 +0000
@@ -4,9 +4,11 @@ import sys,os
 
 import libxltypes
 
-(TYPE_BOOL, TYPE_INT, TYPE_UINT, TYPE_STRING) = xrange(4)
+(TYPE_BOOL, TYPE_INT, TYPE_UINT, TYPE_STRING, TYPE_LIST) = xrange(5)
 
 def py_type(ty):
+    if isinstance(ty, libxltypes.Array):
+        return TYPE_LIST
     if ty == libxltypes.bool or isinstance(ty, libxltypes.BitField) and ty.width == 1:
         return TYPE_BOOL
     if isinstance(ty, libxltypes.Number):
@@ -27,7 +29,10 @@ def py_wrapstruct(ty):
     l.append('    PyObject_HEAD;')
     l.append('    %s obj;'%ty.typename);
     for f in shadow_fields(ty):
-        l.append('    Py_%s *%s_ref;'%(f.type.rawname, f.python_name))
+        if isinstance(f.type, libxltypes.Array):
+             l.append('    PyObject *%s_ref;'%f.python_name)
+        else:
+            l.append('    Py_%s *%s_ref;'%(f.type.rawname, f.python_name))
     l.append('}Py_%s;'%ty.rawname)
     l.append('')
     return "\n".join(l) + "\n"
@@ -42,7 +47,7 @@ def py_decls(ty):
     l.append('_hidden Py_%s *Py%s_New(void);\n'%(ty.rawname, ty.rawname))
     l.append('_hidden int Py%s_Check(PyObject *self);\n'%ty.rawname)
     if len(shadow_fields(ty)) > 0:
-        l.append('_hidden void Py%s_Unshadow(Py_%s *self);\n'%(ty.rawname, ty.rawname))
+        l.append('_hidden int Py%s_Unshadow(Py_%s *self);\n'%(ty.rawname, ty.rawname))
     for f in ty.fields:
         if py_type(f.type) is not None:
             continue
@@ -62,6 +67,15 @@ def union_check(f, ret, prefix = 'self->
         u_check.append('    }')
     return u_check
 
+def num_var(fname):
+    "Determine the name of the count variable for an array with given name"
+    # Yes, it's a bit of a hacky way to do it
+    try:
+        i = fname.rindex('.')
+    except ValueError:
+        return 'num_%s'%fname
+    return fname[:i] + 'num_' + fname[i:]
+
 def py_attrib_get(ty, f):
     t = py_type(f.type)
     l = []
@@ -124,14 +138,22 @@ def py_attrib_set(ty, f):
         l.append('    return genwrap__string_set(v, &self->obj.%s);'%f.name)
     elif f.shadow_type is True:
         l.extend(u_check)
-        l.append('    if ( !Py%s_Check(v) ) {'%f.type.rawname)
-        l.append('        PyErr_SetString(PyExc_TypeError, "Expected xl.%s");'%f.type.rawname)
+        if isinstance(f.type, libxltypes.Array):
+            at = f.type.array_type
+            l.append('    if ( !genwrap__list_check(v, Py%s_Check) ) {'%at.rawname)
+            l.append('        PyErr_SetString(PyExc_TypeError, "Expected list of xl.%s");'%at.rawname)
+        else:
+            l.append('    if ( !Py%s_Check(v) ) {'%f.type.rawname)
+            l.append('        PyErr_SetString(PyExc_TypeError, "Expected xl.%s");'%f.type.rawname)
         l.append('        return -1;')
         l.append('    }')
         l.append('    if ( self->%s_ref ) {'%f.python_name)
         l.append('        Py_DECREF(self->%s_ref);'%f.python_name)
         l.append('    }')
-        l.append('    self->%s_ref = (Py_%s *)v;'%(f.python_name, f.type.rawname))
+        if isinstance(f.type, libxltypes.Array):
+            l.append('    self->%s_ref = v;'%f.python_name)
+        else:
+            l.append('    self->%s_ref = (Py_%s *)v;'%(f.python_name, f.type.rawname))
         l.append('    Py_INCREF(self->%s_ref);'%f.python_name)
         l.append('    return 0;')
     else:
@@ -141,23 +163,88 @@ def py_attrib_set(ty, f):
     l.append('}')
     return '\n'.join(l) + "\n\n"
 
-def sync_func(ty):
+def list_unshadow_func(ty):
+    if len(shadow_fields(ty)):
+        uncall = '        Py%s_Unshadow(val)\n'%ty.rawname
+    else:
+        uncall = ''
+
+    if ty.destructor_fn:
+        fcall = """
+    for(i = 0; i < *len; i++) {
+        %s((*ret) + i);
+    }
+
+"""%ty.destructor_fn
+    else:
+        fcall = ''
+
+    return """/* list of %s */
+static int %s_list_unshadow(PyObject *list, %s **ret, int *len)
+{
+    Py_ssize_t sz = 0, i;
+    %s *arr = NULL;
+
+    if ( list == Py_None || list == NULL )
+        goto out;
+
+    if ( !PyList_Check(list) ) {
+        PyErr_SetString(PyExc_TypeError, "Expected list of xl.%s");
+        return 0;
+    }
+
+    sz = PyList_Size(list);
+    if ( sz <= 0 )
+        goto out;
+
+    arr = calloc(sz, sizeof(*arr));
+    if ( NULL == arr ) {
+        PyErr_SetString(PyExc_MemoryError, "Allocating array of %s");
+        return 0;
+    }
+
+out:
+    for(i = 0; i < sz; i++) {
+        Py_%s *val;
+        val = (Py_%s *)PyList_GetItem(list, i);
+%s        memcpy(arr + i, &val->obj, sizeof(val->obj));
+    }
+%s
+    free(*ret);
+    *ret = arr;
+    *len = sz;
+    return 1;
+}
+
+"""%tuple([ty.typename for x in xrange(4)] + [ty.rawname for x in xrange(4)] + [uncall, fcall])
+
+def unshadow_func(ty):
     pf = shadow_fields(ty)
     if len(pf) == 0:
         return ''
     l = []
 
-    l.append('void Py%s_Unshadow(Py_%s*self)\n'%(ty.rawname, ty.rawname))
+    l.append('int Py%s_Unshadow(Py_%s*self)\n'%(ty.rawname, ty.rawname))
     l.append('{')
+    l.append('    int ret = 1;')
     for f in pf:
-        l.append('    if ( self->%s_ref ) {'%f.python_name)
-        l.append('        Py_%s *x = (Py_%s *)self->%s_ref;'%(f.type.rawname, f.type.rawname, f.python_name))
-        if len(shadow_fields(f.type)):
-            l.append('        Py%s_Unshadow(x);'%f.type.rawname)
-        l.append('        memcpy(&self->obj.%s, &x->obj, sizeof(self->obj.%s));'%(f.name, f.name))
-        l.append('    }else{')
-        l.append('        memset(&self->obj.%s, 0, sizeof(self->obj.%s));'%(f.name, f.name))
-        l.append('    }')
+        if isinstance(f.type, libxltypes.Array):
+            at = f.type.array_type
+            l.append('    if ( !%s_list_unshadow(self->%s_ref, '%(at.typename, f.python_name))
+            l.append('                           &self->obj.%s, '%f.name)
+            l.append('                           &self->obj.%s) )'%num_var(f.name))
+            l.append('        ret = 0;')
+        else:
+            l.append('    if ( self->%s_ref ) {'%f.python_name)
+            l.append('        Py_%s *x = (Py_%s *)self->%s_ref;'%(f.type.rawname, f.type.rawname, f.python_name))
+            if len(shadow_fields(f.type)):
+                l.append('        if ( !Py%s_Unshadow(x) )'%f.type.rawname)
+                l.append('            ret = 0;')
+            l.append('        memcpy(&self->obj.%s, &x->obj, sizeof(self->obj.%s));'%(f.name, f.name))
+            l.append('    }else{')
+            l.append('        memset(&self->obj.%s, 0, sizeof(self->obj.%s));'%(f.name, f.name))
+            l.append('    }')
+    l.append('    return ret;')
     l.append('}')
     l.append('')
     return '\n'.join(l)
@@ -181,18 +268,24 @@ static PyObject *Py%s_new(PyTypeObject *
 
 """%tuple(ty.rawname for x in range(5))
 
-    funcs += sync_func(ty)
+    funcs += unshadow_func(ty)
 
-
+    l.append('')
     l.append('static void Py%s_dealloc(Py_%s *self)'%(ty.rawname, ty.rawname))
     l.append('{')
-    if ty.destructor_fn is not None:
-        l.append('    %s(&self->obj);'%ty.destructor_fn)
     for f in shadow_fields(ty):
         l.append('    if ( self->%s_ref ) {'%f.python_name)
         l.append('        Py_DECREF(self->%s_ref);'%f.python_name)
         l.append('    }')
-    l.append('self->ob_type->tp_free((PyObject *)self);')
+        if isinstance(f.type, libxltypes.Array):
+            l.append('    self->obj.%s = NULL;'%f.name)
+            l.append('    self->obj.%s = 0;'%num_var(f.name))
+        else:
+            l.append('    memset(&self->obj.%s, 0, sizeof(self->obj.%s));'%(
+                     f.name, f.name))
+    if ty.destructor_fn is not None:
+        l.append('    %s(&self->obj);'%ty.destructor_fn)
+    l.append('    self->ob_type->tp_free((PyObject *)self);')
     l.append('}')
     l.append('')
 
@@ -279,7 +372,7 @@ def dbg_tree(str, indent=0):
 # We don't have a good translation for anonymous structures so we just
 # flatten them out recursively and replace '.' with '_'. For example
 # domain_build_info.u.hvm.pae becomes domain_build_info.u_hvm_pae
-def flatten_type(ty, path = None, depth = 0, condvar = []):
+def flatten_type(ty, sdict, path = None, depth = 0, condvar = []):
     if not isinstance(ty, libxltypes.Aggregate):
         return ty.fields
 
@@ -295,6 +388,9 @@ def flatten_type(ty, path = None, depth 
         if isinstance(f.type, libxltypes.Aggregate) and \
                 f.type.typename is not None:
             f.shadow_type = True
+        elif isinstance(f.type, libxltypes.Array):
+            sdict[f.type.array_type] = None
+            f.shadow_type = True
         else:
             f.shadow_type = False
 
@@ -308,7 +404,7 @@ def flatten_type(ty, path = None, depth 
             dbg_tree('(%s)'%(f.name), depth + 1)
             if isinstance(ty, libxltypes.KeyedUnion):
                 condvar.append((f.keyvar_expr, ty.keyvar_name))
-            ret.extend(flatten_type(f.type, f.name, depth + 1, condvar))
+            ret.extend(flatten_type(f.type, sdict, f.name, depth + 1, condvar))
             if isinstance(ty, libxltypes.KeyedUnion):
                 condvar.pop()
         else:
@@ -318,13 +414,14 @@ def flatten_type(ty, path = None, depth 
 
     return ret
 
-def frob_type(type):
+def frob_type(type, sdict):
     dbg_tree('[%s]'%type.typename)
-    type.fields = flatten_type(type)
+    type.fields = flatten_type(type, sdict)
     return type
 
 def frob_types(types):
-    return map(frob_type, types)
+    sdict = {}
+    return (map(lambda x:frob_type(x, sdict), types), sdict)
 
 if __name__ == '__main__':
     if len(sys.argv) < 4:
@@ -334,7 +431,7 @@ if __name__ == '__main__':
     idl = sys.argv[1]
     (_,types) = libxltypes.parse(idl)
 
-    types = frob_types(types)
+    (types, sdict) = frob_types(types)
 
     decls = sys.argv[2]
     f = open(decls, 'w')
@@ -364,6 +461,9 @@ _hidden void genwrap__init(PyObject *m);
 /* Generic type initialiser */
 _hidden int genwrap__obj_init(PyObject *self, PyObject *args, PyObject *kwds);
 
+/* Generic checker for list wrapper */
+_hidden int genwrap__list_check(PyObject *list, int(*cbfn)(PyObject *obj));
+
 /* Auto-generated get/set functions for simple data-types */
 _hidden int genwrap__string_set(PyObject *v, char **str);
 _hidden PyObject *genwrap__string_get(char **str);
@@ -399,6 +499,8 @@ _hidden int genwrap__ll_set(PyObject *v,
 #include "%s"
 
 """ % tuple((' '.join(sys.argv),) + (os.path.split(decls)[-1:]),))
+    for ty in sdict.keys():
+        f.write(list_unshadow_func(ty))
     for ty in types:
         f.write('/* Attribute get/set functions for %s */\n'%ty.typename)
         for a in ty.fields:
diff -r 735df86eab9a -r 9c57b1622bbc tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:08 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:08 2011 +0000
@@ -73,6 +73,25 @@ int genwrap__obj_init(PyObject *self, Py
     return 0;
 }
 
+int genwrap__list_check(PyObject *list, int(*cbfn)(PyObject *obj))
+{
+    Py_ssize_t i, len;
+
+    if ( list == Py_None )
+        return 1;
+
+    if ( !PyList_Check(list) )
+        return 0;
+
+    len = PyList_Size(list);
+    for(i = 0; i < len; i++) {
+        if ( !(*cbfn)(PyList_GetItem(list, i)) )
+            return 0;
+    }
+
+    return 1;
+}
+
 int genwrap__string_set(PyObject *v, char **str)
 {
     char *tmp;

[PATCH 09 of 10] xl: Implement enum types in IDL

[Gianni Tedesco]

 tools/libxl/libxltypes.py |  4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761669 0
# Node ID 3bbba45fac5c7f614d75076aaf3e567799b2b4a7
# Parent  9c57b1622bbcc6252e59d18dca5e33673d6fc236
xl: Implement enum types in IDL

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 9c57b1622bbc -r 3bbba45fac5c tools/libxl/libxltypes.py
--- a/tools/libxl/libxltypes.py	Tue Jan 11 16:01:08 2011 +0000
+++ b/tools/libxl/libxltypes.py	Tue Jan 11 16:01:09 2011 +0000
@@ -44,6 +44,10 @@ class Number(Builtin):
         self.signed = kwargs['signed']
         Builtin.__init__(self, ctype, **kwargs)
 
+class Enum(Number):
+    def __init__(self, w, **kwargs):
+        Number.__init__(self, 'enum %s'%w, **kwargs)
+
 class UInt(Number):
     def __init__(self, w, **kwargs):
         kwargs.setdefault('namespace', None)

[PATCH 10 of 10] pyxl: Export libxl_domain_create_new() to python binding

[Gianni Tedesco]

 tools/libxl/libxl.h               |  45 ++++++------------------
 tools/libxl/libxl.idl             |  16 +++++++++
 tools/libxl/libxl_create.c        |  33 ------------------
 tools/python/xen/lowlevel/xl/xl.c |  68 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 94 insertions(+), 68 deletions(-)


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1294761669 0
# Node ID beded83ed66984ee588e18c6160c546895443607
# Parent  3bbba45fac5c7f614d75076aaf3e567799b2b4a7
pyxl: Export libxl_domain_create_new() to python binding

This also moves struct libxl_domain_config to be an IDL generated type.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 3bbba45fac5c -r beded83ed669 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue Jan 11 16:01:09 2011 +0000
+++ b/tools/libxl/libxl.h	Tue Jan 11 16:01:09 2011 +0000
@@ -205,6 +205,18 @@ typedef struct libxl__cpuid_policy libxl
 typedef libxl_cpuid_policy * libxl_cpuid_policy_list;
 void libxl_cpuid_destroy(libxl_cpuid_policy_list *cpuid_list);
 
+enum libxl_action_on_shutdown {
+    LIBXL_ACTION_DESTROY,
+
+    LIBXL_ACTION_RESTART,
+    LIBXL_ACTION_RESTART_RENAME,
+
+    LIBXL_ACTION_PRESERVE,
+
+    LIBXL_ACTION_COREDUMP_DESTROY,
+    LIBXL_ACTION_COREDUMP_RESTART,
+};
+
 #define LIBXL_PCI_FUNC_ALL (~0U)
 
 #include "_libxl_types.h"
@@ -241,38 +253,6 @@ enum {
 
 #define LIBXL_VERSION 0
 
-enum libxl_action_on_shutdown {
-    LIBXL_ACTION_DESTROY,
-
-    LIBXL_ACTION_RESTART,
-    LIBXL_ACTION_RESTART_RENAME,
-
-    LIBXL_ACTION_PRESERVE,
-
-    LIBXL_ACTION_COREDUMP_DESTROY,
-    LIBXL_ACTION_COREDUMP_RESTART,
-};
-
-typedef struct {
-    libxl_domain_create_info c_info;
-    libxl_domain_build_info b_info;
-    libxl_device_model_info dm_info;
-
-    int num_disks, num_vifs, num_vif2s, num_pcidevs, num_vfbs, num_vkbs;
-
-    libxl_device_disk *disks;
-    libxl_device_nic *vifs;
-    libxl_device_net2 *vif2s;
-    libxl_device_pci *pcidevs;
-    libxl_device_vfb *vfbs;
-    libxl_device_vkb *vkbs;
-
-    enum libxl_action_on_shutdown on_poweroff;
-    enum libxl_action_on_shutdown on_reboot;
-    enum libxl_action_on_shutdown on_watchdog;
-    enum libxl_action_on_shutdown on_crash;
-} libxl_domain_config;
-
 /* context functions */
 int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger*);
 int libxl_ctx_free(libxl_ctx *ctx);
@@ -286,7 +266,6 @@ void libxl_init_dm_info(libxl_device_mod
 typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
 int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
-void libxl_domain_config_destroy(libxl_domain_config *d_config);
 int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
                           uint32_t domid, int fd);
 int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid);
diff -r 3bbba45fac5c -r beded83ed669 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Tue Jan 11 16:01:09 2011 +0000
+++ b/tools/libxl/libxl.idl	Tue Jan 11 16:01:09 2011 +0000
@@ -327,3 +327,19 @@ libxl_net2info = Struct("net2info", [
     ("back_mac", libxl_mac),
     ("filter_mac", integer),
     ])
+
+libxl_domain_config = Struct("domain_config", [
+    ("c_info", libxl_domain_create_info),
+    ("b_info", libxl_domain_build_info),
+    ("dm_info", libxl_device_model_info),
+    ("disks", Array(libxl_device_disk)),
+    ("vifs", Array(libxl_device_nic)),
+    ("vif2s", Array(libxl_device_net2)),
+    ("pcidevs", Array(libxl_device_pci)),
+    ("vfbs", Array(libxl_device_vfb)),
+    ("vkbs", Array(libxl_device_vkb)),
+    ("on_poweroff", Enum("libxl_action_on_shutdown")),
+    ("on_reboot", Enum("libxl_action_on_shutdown")),
+    ("on_watchdog", Enum("libxl_action_on_shutdown")),
+    ("on_crash", Enum("libxl_action_on_shutdown")),
+    ])
diff -r 3bbba45fac5c -r beded83ed669 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Tue Jan 11 16:01:09 2011 +0000
+++ b/tools/libxl/libxl_create.c	Tue Jan 11 16:01:09 2011 +0000
@@ -27,39 +27,6 @@
 #include "libxl_internal.h"
 #include "flexarray.h"
 
-void libxl_domain_config_destroy(libxl_domain_config *d_config)
-{
-    int i;
-
-    for (i=0; i<d_config->num_disks; i++)
-        libxl_device_disk_destroy(&d_config->disks[i]);
-    free(d_config->disks);
-
-    for (i=0; i<d_config->num_vifs; i++)
-        libxl_device_nic_destroy(&d_config->vifs[i]);
-    free(d_config->vifs);
-
-    for (i=0; i<d_config->num_vif2s; i++)
-        libxl_device_net2_destroy(&d_config->vif2s[i]);
-    free(d_config->vif2s);
-
-    for (i=0; i<d_config->num_pcidevs; i++)
-        libxl_device_pci_destroy(&d_config->pcidevs[i]);
-    free(d_config->pcidevs);
-
-    for (i=0; i<d_config->num_vfbs; i++)
-        libxl_device_vfb_destroy(&d_config->vfbs[i]);
-    free(d_config->vfbs);
-
-    for (i=0; i<d_config->num_vkbs; i++)
-        libxl_device_vkb_destroy(&d_config->vkbs[i]);
-    free(d_config->vkbs);
-
-    libxl_domain_create_info_destroy(&d_config->c_info);
-    libxl_domain_build_info_destroy(&d_config->b_info);
-    libxl_device_model_info_destroy(&d_config->dm_info);
-}
-
 void libxl_init_create_info(libxl_domain_create_info *c_info)
 {
     memset(c_info, '\0', sizeof(*c_info));
diff -r 3bbba45fac5c -r beded83ed669 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:09 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Jan 11 16:01:09 2011 +0000
@@ -443,6 +443,68 @@ static PyObject *pyxl_domid_to_name(XlOb
     return ret;
 }
 
+struct cb_priv {
+    PyObject *xl_ctx, *cb, *cb_priv;
+};
+
+static int console_ready(libxl_ctx *ctx, uint32_t domid, void *priv)
+{
+    struct cb_priv *p = priv;
+    PyObject *d;
+
+    /* no callback provided, succeed */
+    if ( p->cb == Py_None )
+        return 0;
+
+    d = PyInt_FromLong(domid);
+    if ( d == NULL )
+        return -1;
+
+    if ( PyObject_CallFunctionObjArgs(p->cb, p->xl_ctx, d, p->cb_priv, NULL) ) {
+        Py_DECREF(d);
+        return 0;
+    }
+
+    Py_DECREF(d);
+    return -1;
+}
+
+static PyObject *pyxl_domain_create(XlObject *self, PyObject *args)
+{
+    PyObject *console_cb, *cb_priv, *rfile = NULL;
+    Py_domain_config *d_config;
+    struct cb_priv priv;
+    uint32_t domid;
+
+    if ( !PyArg_ParseTuple(args, "OOO|O", &d_config, &console_cb, &cb_priv, &rfile) )
+        return NULL;
+
+    if ( !Pydomain_config_Check((PyObject *)d_config) ) {
+        PyErr_SetString(PyExc_TypeError, "Expected xl.domain_config");
+        return NULL;
+    }
+
+    if ( console_cb != Py_None && !PyCallable_Check(console_cb) ) {
+        PyErr_SetString(PyExc_TypeError, "Expected callable console callback");
+        return NULL;
+    }
+
+    if ( !Pydomain_config_Unshadow(d_config) )
+        return NULL;
+
+    priv.xl_ctx = (PyObject *)self;
+    priv.cb = console_cb;
+    priv.cb_priv = cb_priv;
+
+    if ( libxl_domain_create_new(&self->ctx, &d_config->obj,
+                                    console_ready, &priv, &domid) ) {
+        PyErr_SetString(xl_error_obj, "Error creating domain");
+        return NULL;
+    }
+
+    return PyInt_FromLong(domid);
+}
+
 static PyObject *pyxl_domain_shutdown(XlObject *self, PyObject *args)
 {
     int domid, req = 0;
@@ -517,7 +579,7 @@ static PyObject *pyxl_pci_add(XlObject *
     if ( !PyArg_ParseTuple(args, "iO", &domid, &obj) )
         return NULL;
     if ( !Pydevice_pci_Check(obj) ) {
-        PyErr_SetString(PyExc_TypeError, "Xxpected xl.device_pci");
+        PyErr_SetString(PyExc_TypeError, "Expected xl.device_pci");
         return NULL;
     }
     pci = (Py_device_pci *)obj;
@@ -538,7 +600,7 @@ static PyObject *pyxl_pci_del(XlObject *
     if ( !PyArg_ParseTuple(args, "iO|i", &domid, &obj, &force) )
         return NULL;
     if ( !Pydevice_pci_Check(obj) ) {
-        PyErr_SetString(PyExc_TypeError, "Xxpected xl.device_pci");
+        PyErr_SetString(PyExc_TypeError, "Expected xl.device_pci");
         return NULL;
     }
     pci = (Py_device_pci *)obj;
@@ -649,6 +711,8 @@ static PyMethodDef pyxl_methods[] = {
          "Retrieve name from domain-id"},
     {"domain_shutdown", (PyCFunction)pyxl_domain_shutdown, METH_VARARGS,
          "Shutdown a domain"},
+    {"domain_create", (PyCFunction)pyxl_domain_create, METH_VARARGS,
+         "Create or restore a domain"},
     {"domain_destroy", (PyCFunction)pyxl_domain_destroy, METH_VARARGS,
          "Destroy a domain"},
     {"domain_pause", (PyCFunction)pyxl_domain_pause, METH_VARARGS,

Re: [PATCH 00 of 10] libxl python binding updates

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

On Tue, 2011-01-11 at 16:03 +0000, Gianni Tedesco wrote:
> This patch-series brings the libxl python binding up to the state where it can
> create domains. It also includes several bug-fixes and exporting of additional
> libxl functionality such as PCI passthrough.

Attached file is a python script which re-implements some xl commands in
python, used for testing the above patch-series.

Gianni

[-- Attachment #2: pyxl.py --]
[-- Type: text/x-python, Size: 9296 bytes --]

#!/usr/bin/python

from xen.lowlevel import xl

class PyxlError(xl.Error):
    pass
class PyxlArgError(PyxlError):
    pass
class PyxlNoDomain(PyxlError):
    pass

def superclass(self, orig, type, kwargs):
    if orig == None:
        return
    if orig.__class__ != type:
        raise TypeError
    map(lambda (k,v): kwargs.setdefault(k, v),
            filter(lambda (k,v): k[0] != '_' and not callable(v),
                    map(lambda k:(k, getattr(orig, k)), dir(orig))))
    return

class Ctx:
    class __impl(xl.ctx):
        pass

    __instance = None

    def __init__(self):
        if Ctx.__instance is None:
            Ctx.__instance = self.__impl()
        self.__dict__['Ctx_instance'] = Ctx.__instance

    def __getattr__(self, attr):
        return getattr(self.__instance, attr)
    def __setattr__(self, attr, value):
        return setattr(self.__instance, attr, value)


class DomInfo(xl.dominfo):
    def __init__(self, orig = None, **kwargs):
        superclass(self, orig, xl.dominfo, kwargs)
        xl.dominfo.__init__(self, **kwargs)
        self.name = None
    def set_name(self, name):
        self.name = str(name)
    def __repr__(self):
        if self.name is not None:
            return 'dominfo(%s:%d)'%(self.name, self.domid)
        else:
            return 'dominfo(%d)'%self.domid
    def __str__(self):
        return '%s %5d %5lu %5d     %s%s%s%s%s%s  %8.1f'%(self.name.ljust(40),
                self.domid,
                self.max_memkb / 1024,
                self.vcpu_online,
                self.running and 'r' or '-',
                self.blocked and 'b' or '-',
                self.paused and 'p' or '-',
                self.shutdown and 's' or '-',
                self.shutdown_reason == xl.SHUTDOWN_crash and 'c' or '-',
                self.dying and 'd' or '-',
                float(self.cpu_time) / 1e9)

def find_domain(arg, by_name = True, by_id = True):
    ctx = Ctx()
    domlist = map(DomInfo, ctx.list_domains())
    map(lambda x:x.set_name(ctx.domid_to_name(x.domid)), domlist)

    if by_id:
        dl_by_id = dict(map(lambda v:(v.domid, v), domlist))
        try:
            return dl_by_id[int(arg)]
        except:
            pass

    if by_name:
        dl_by_name = dict(map(lambda v:(v.name, v), domlist))
        try:
            return dl_by_name[str(arg)]
        except:
            pass

    raise PyxlNoDomain, "Domain '%s' not found"%arg

def uuid(s):
    a = ''.join(map(lambda x:'%.2x'%x, s))
    return '-'.join([a[0:8], a[8:12], a[12:16], a[16:20], a[20:]])

def do_rename(args):
    "Rename a domain"

    ctx = Ctx()
    if len(args) is not 3:
        raise PyxlArgError, 'Requires 2 arguments'
    d = find_domain(args[1])

    ctx.domain_rename(d.domid, args[2], d.name)

def do_list(args):
    "List information about all/some domains"

    verbose = '-v' in args
    ctx = Ctx()
    domlist = map(DomInfo, ctx.list_domains())
    map(lambda x:x.set_name(ctx.domid_to_name(x.domid)), domlist)
    print 'Name                                        ID   Mem VCPUs\tState\tTime(s)'
    for x in domlist:
        print '%s%s'%(x, verbose and ' %s'%uuid(x.uuid) or '')

def do_pause(args):
    "Pause execution of a domain"

    if len(args) is not 2:
        raise PyxlArgError
    ctx = Ctx()
    d = find_domain(args[1])
    ctx.domain_pause(d.domid)

def do_unpause(args):
    "Unpause a paused domain"

    if len(args) is not 2:
        raise PyxlArgError
    ctx = Ctx()
    d = find_domain(args[1])
    ctx.domain_unpause(d.domid)

def do_domid(args):
    "Convert a domain name to domain id"
    if len(args) is not 2:
        raise PyxlArgError
    ctx = Ctx()
    d = find_domain(args[1], by_id = False)
    print d.domid

def do_domname(args):
    "Convert a domain id to domain name"
    if len(args) is not 2:
        raise PyxlArgError
    ctx = Ctx()
    d = find_domain(args[1], by_name = False)
    print d.name

def do_pci_list_assignable(args):
    "List all the assignable pci devices"
    if len(args) is not 1:
        raise PyxlArgError
    ctx = Ctx()
    for x in ctx.device_pci_list_assignable():
        print "%.4x:%.2x:%.2x.%.1x"%(x.domain, x.bus, x.dev, x.func)

def do_pci_list(args):
    "List pass-through pci devices for a domain"
    if len(args) is not 2:
        raise PyxlArgError
    ctx = Ctx()
    d = find_domain(args[1])
    for x in ctx.device_pci_list(d.domid):
        print "%.4x:%.2x:%.2x.%.1x"%(x.domain, x.bus, x.dev, x.func)

def do_pci_add(args):
    "Insert a new pass-through pci device"
    if len(args) not in [3,4]:
        raise PyxlArgError
    ctx = Ctx()
    d = find_domain(args[1])
    dev = ctx.device_pci_parse_bdf(args[2])
    # TODO: virtual slot
    ctx.device_pci_add(d.domid, dev)

def do_pci_del(args):
    "Remove a domain's pass-through pci device"
    if len(args) is not 3:
        raise PyxlArgError
    ctx = Ctx()
    d = find_domain(args[1])
    dev = ctx.device_pci_parse_bdf(args[2])
    ctx.device_pci_del(d.domid, dev)

def callback(xl, domid, priv):
    print 'callback: %r %d %r'%(xl, domid, priv)

def do_destroy(args):
    if len(args) not in [2,3]:
        raise PyxlArgError
    force = (len(args) > 2 and '-f' in args)
    ctx = Ctx()
    d = find_domain(args[1])
    ctx.domain_destroy(d.domid, force)

def do_create(args):
    hvmloader_path = '/home/scara/xen-unstable-tools.hg/tools/firmware/hvmloader/hvmloader'
    dm_path = '/home/scara/xen-unstable-tools.hg/tools/ioemu-remote/i386-dm/qemu-dm'
    name = 'pyxl-test'

    # create info
    ci = xl.domain_create_info()
    ci.hap = True
    ci.hvm = True
    ci.oos = True
    ci.name = name
    ci.platformdata = None
    ci.poolid = 0
    ci.poolname = 'Pool-0'
    ci.xsdata = None
    ci.ssidref
    ci.uuid = ''.join([chr(0) for x in xrange(16)])

    # build info
    bi = xl.domain_build_info()
    #bi.cpuid = None
    bi.cur_vcpus = 1
    bi.max_vcpus = 1
    bi.disable_migrate = False
    bi.hvm = True
    bi.u_hvm_pae = True
    bi.u_hvm_apic = True
    bi.u_hvm_acpi = True
    bi.u_hvm_nx = True
    bi.u_hvm_viridian = False
    bi.u_hvm_timeoffset = None
    bi.u_hvm_hpet = True
    bi.u_hvm_vpt_align = True
    bi.u_hvm_timer_mode = 1
    bi.kernel = hvmloader_path
    bi.shadow_memkb = 3072
    bi.video_memkb = 8192
    bi.target_memkb = 262144
    bi.max_memkb = 262144
    bi.tsc_mode = 0

    # create dm
    dm = xl.device_model_info()
    dm.device_model = dm_path
    dm.videoram = bi.video_memkb >> 10
    dm.dom_name = name
    dm.boot = 'cda'
    dm.vnc = 1
    dm.vncunused = 1
    dm.vnclisten = '0.0.0.0'
    dm.apic = bi.u_hvm_apic
    dm.vcpus = bi.max_vcpus
    dm.vcpu_avail = bi.cur_vcpus

    # attach devices
    disks = []
    d = xl.device_disk()
    d.physpath = '/vm/winxp.hd'
    d.phystype = 5
    d.unpluggable = True
    d.virtpath = 'xvda'
    d.readwrite = True
    disks.append(d)

    config = xl.domain_config()
    config.c_info = ci
    config.b_info = bi
    config.dm_info = dm
    config.disks = disks

    ctx = Ctx()
    domid = ctx.domain_create(config, callback, 'test')
    print 'domid %d'%domid

    # unpause
    ctx.domain_unpause(domid)

    # daemonise maybe

def do_help(args):
    "Print this message"

    global cmdtbl
    if len(args) > 1:
        usage(args[1])
        return
    keys = cmdtbl.keys()
    keys.sort()
    print 'Usage pyxl [-v] <subcommand> [args]'
    print
    print 'pyxl full list of subcommands:'
    print
    for (k,fn,u) in map(lambda k:(k,) + cmdtbl[k], keys):
        print ' %s %s'%(k.ljust(20), fn.__doc__)

def usage(subcmd, exc = None):
    global cmdtbl
    from sys import stderr

    if not cmdtbl.has_key(subcmd):
        stderr.write('No such command %s\n'%subcmd)
        return

    if exc is not None:
        for msg in (exc.args):
            stderr.write('\'xl %s\' %s\n'%(subcmd, msg))
        if len(exc.args):
            stderr.write('\n')

    (fn, u) = cmdtbl[subcmd]
    stderr.write('Usage xl [-v] %s %s\n'%(subcmd, u))
    stderr.write('\n%s\n\n'%fn.__doc__)

cmdtbl = {
    'create' : (do_create, '<ConfigFile> [options] [vars]'),
    'destroy' : (do_destroy, '<Domain> [-f]'),
    'rename' : (do_rename, '<Domain> <NewDomainName>'),
    'pause' : (do_pause, '<Domain>'),
    'unpause' : (do_unpause, '<Domain>'),
    'domid' : (do_domid, '<DomainName>'),
    'domname' : (do_domname, '<DomainID>'),
    'pci-attach' : (do_pci_add, '<Domain> <BDF> [Virtual Slot]'),
    'pci-detach' : (do_pci_del, '<Domain> <BDF>'),
    'pci-list' : (do_pci_list, '<Domain>'),
    'pci-list-assignable-devices' : (do_pci_list_assignable,''),
    'list' : (do_list, '[Domain]'),
    'help' : (do_help, '[subcommand]')
}

if __name__ == '__main__':
    from sys import argv

    if len(argv) < 2:
        do_help(['help'])
        raise SystemExit, True
    if not cmdtbl.has_key(argv[1]):
        from sys import stderr
        do_help(['help'])
        stderr.write('No such command %s\n'%argv[1])
        raise SystemExit, True
    (fn, u) = cmdtbl[argv[1]]

    try:
        fn(argv[1:])
    except PyxlArgError, e:
        usage(argv[1], e)
        raise SystemExit,True
    except xl.Error, e:
        from sys import stderr
        stderr.write('%s\n'%'\n'.join(map(lambda x:'Error: %s'%x, e.args)))
        raise SystemExit,True

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Ian Jackson]

Gianni Tedesco writes ("[Xen-devel] [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate"):
> pyxl: Recursively scan type-tree to produce complete binding boilerplate

I applied patches 1-6.  They touch only pyxl and didn't break the
build.

However patch 7 contains this:

> --- a/tools/libxl/libxl.idl	Tue Jan 11 16:01:07 2011 +0000
> +++ b/tools/libxl/libxl.idl	Tue Jan 11 16:01:08 2011 +0000
...
> -                                        ("features", string, True),
> +                                        ("features", string),

This makes a substantive change to both the interface and the
generated code, and wasn't mentioned in the comment.  I'm not
convinced it's a change that's appropriate during feature freeze.

So I'm pushing 1-6 and the rest will have to wait for after the 4.1
release, I'm afraid.

Ian.

Re: [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Gianni Tedesco]

On Tue, 2011-01-11 at 19:26 +0000, Ian Jackson wrote:
> Gianni Tedesco writes ("[Xen-devel] [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate"):
> > pyxl: Recursively scan type-tree to produce complete binding boilerplate
> 
> I applied patches 1-6.  They touch only pyxl and didn't break the
> build.
> 
> However patch 7 contains this:
> 
> > --- a/tools/libxl/libxl.idl	Tue Jan 11 16:01:07 2011 +0000
> > +++ b/tools/libxl/libxl.idl	Tue Jan 11 16:01:08 2011 +0000
> ...
> > -                                        ("features", string, True),
> > +                                        ("features", string),
> 
> This makes a substantive change to both the interface and the
> generated code, and wasn't mentioned in the comment.  I'm not
> convinced it's a change that's appropriate during feature freeze.

We discussed this off-list at the time I was writing it. I think with
one of the other guys. We decided that const char * in the interface was
a bad idea since it imposed an allocation policy where no reason for it
actually existed in the code. I could have worked around this by casting
everything in the python bindings but we felt that was an ugly solution
and not a good precedent to set.

You are right that it changes the code and may incorrectly free the ""
allocated in libxl_dm.c - I could strdup() in that instance that but the
interface is changed so we have a problem. Alternatively I can change 2
lines in genwrap.py to emit correct code for this.

> So I'm pushing 1-6 and the rest will have to wait for after the 4.1
> release, I'm afraid.

Your chainsaw.... my baby... :(

Gianni

--
# HG changeset patch
# Parent 760d458df83d738019b404a2dba12959d79ac1dd
pyxl: Recursively scan type-tree to produce complete binding boilerplate

Currently the python wrapper generator ignores non-toplevel types in the IDL.
The KeyedUnion implementation is trivial but for structures embedded in
structures the code becomes more complex. The problem is that when assigning to
a structure (a.b = x) either we:
 - copy the c structure from x in to a.b which is unexpected for python
   programmers since everything is by reference not by value.
 - keep a reference to x which 'shadows' a.b and do the copy just before we
   pass the wrapped C structure across the libxl C API boundary.

The latter approach is chosen. The tree of shadow references is maintained in
the auto-generated code and Py_foo_Unshadow() functions are also generated and
exposed for the hand-written part of the xl wrapper to use.

Finally, for anonymous types, a name mangling system is used to translate names
like x.u.hvm.acpi. To support such constructs in python we would need to export
python types for x.u.hvm such as 'xl.anonymous_hvm_0'. Instead of dealing with
this we simply flatten the type tree so that such a field is named
x.u_hvm_apic.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 760d458df83d tools/python/genwrap.py
--- a/tools/python/genwrap.py	Wed Jan 12 12:44:18 2011 +0000
+++ b/tools/python/genwrap.py	Wed Jan 12 12:46:57 2011 +0000
@@ -4,7 +4,7 @@ import sys,os
 
 import libxltypes
 
-(TYPE_BOOL, TYPE_INT, TYPE_UINT, TYPE_STRING) = range(4)
+(TYPE_BOOL, TYPE_INT, TYPE_UINT, TYPE_STRING) = xrange(4)
 
 def py_type(ty):
     if ty == libxltypes.bool or isinstance(ty, libxltypes.BitField) and ty.width == 1:
@@ -18,11 +18,16 @@ def py_type(ty):
         return TYPE_STRING
     return None
 
+def shadow_fields(ty):
+    return filter(lambda x:x.shadow_type is True, ty.fields)
+
 def py_wrapstruct(ty):
     l = []
     l.append('typedef struct {')
     l.append('    PyObject_HEAD;')
     l.append('    %s obj;'%ty.typename);
+    for f in shadow_fields(ty):
+        l.append('    Py_%s *%s_ref;'%(f.type.rawname, f.python_name))
     l.append('}Py_%s;'%ty.rawname)
     l.append('')
     return "\n".join(l) + "\n"
@@ -36,33 +41,59 @@ def py_decls(ty):
     l = []
     l.append('_hidden Py_%s *Py%s_New(void);\n'%(ty.rawname, ty.rawname))
     l.append('_hidden int Py%s_Check(PyObject *self);\n'%ty.rawname)
+    if len(shadow_fields(ty)) > 0:
+        l.append('_hidden void Py%s_Unshadow(Py_%s *self);\n'%(ty.rawname, ty.rawname))
     for f in ty.fields:
         if py_type(f.type) is not None:
             continue
-        l.append('_hidden PyObject *attrib__%s_get(%s *%s);'%(\
-                 fsanitize(f.type.typename), f.type.typename, f.name))
-        l.append('_hidden int attrib__%s_set(PyObject *v, %s *%s);'%(\
-                 fsanitize(f.type.typename), f.type.typename, f.name))
+        l.append('_hidden PyObject *attrib__%s_get(%s *val);'%(\
+                 fsanitize(f.type.typename), f.type.typename))
+        l.append('_hidden int attrib__%s_set(PyObject *v, %s *val);'%(\
+                 fsanitize(f.type.typename), f.type.typename))
     return '\n'.join(l) + "\n"
 
+def union_check(f, ret, prefix = 'self->obj.'):
+    u_check = []
+    if len(f.condlist):
+        cond = '||'.join(map(lambda (expr,name):('!(' + expr + ')')%('%s%s'%(prefix,name)), f.condlist))
+        u_check.append('    if ( %s ) {'%cond)
+        u_check.append('        PyErr_SetString(PyExc_AttributeError, "Union key not selected");')
+        u_check.append('        return %s;'%ret)
+        u_check.append('    }')
+    return u_check
+
 def py_attrib_get(ty, f):
     t = py_type(f.type)
     l = []
-    l.append('static PyObject *py_%s_%s_get(Py_%s *self, void *priv)'%(ty.rawname, f.name, ty.rawname))
+    l.append('static PyObject *py_%s_%s_get(Py_%s *self, void *priv)'%(ty.rawname, f.python_name, ty.rawname))
     l.append('{')
+
+    u_check = union_check(f, 'NULL')
+
     if t == TYPE_BOOL:
         l.append('    PyObject *ret;')
+        l.extend(u_check)
         l.append('    ret = (self->obj.%s) ? Py_True : Py_False;'%f.name)
         l.append('    Py_INCREF(ret);')
         l.append('    return ret;')
     elif t == TYPE_INT:
+        l.extend(u_check)
         l.append('    return genwrap__ll_get(self->obj.%s);'%f.name)
     elif t == TYPE_UINT:
+        l.extend(u_check)
         l.append('    return genwrap__ull_get(self->obj.%s);'%f.name)
     elif t == TYPE_STRING:
-        l.append('    return genwrap__string_get(&self->obj.%s);'%f.name)
+        l.extend(u_check)
+        l.append('    return genwrap__string_get((char **)&self->obj.%s);'%f.name)
+    elif f.shadow_type is True:
+        l.append('    PyObject *ret;')
+        l.extend(u_check)
+        l.append('    ret = (self->%s_ref == NULL) ? Py_None : (PyObject *)self->%s_ref;'%(f.python_name, f.python_name))
+        l.append('    Py_INCREF(ret);')
+        l.append('    return ret;')
     else:
         tn = f.type.typename
+        l.extend(u_check)
         l.append('    return attrib__%s_get((%s *)&self->obj.%s);'%(fsanitize(tn), tn, f.name))
     l.append('}')
     return '\n'.join(l) + "\n\n"
@@ -70,14 +101,17 @@ def py_attrib_get(ty, f):
 def py_attrib_set(ty, f):
     t = py_type(f.type)
     l = []
-    l.append('static int py_%s_%s_set(Py_%s *self, PyObject *v, void *priv)'%(ty.rawname, f.name, ty.rawname))
+    l.append('static int py_%s_%s_set(Py_%s *self, PyObject *v, void *priv)'%(ty.rawname, f.python_name, ty.rawname))
     l.append('{')
+    u_check = union_check(f, '-1')
     if t == TYPE_BOOL:
+        l.extend(u_check)
         l.append('    self->obj.%s = (NULL == v || Py_None == v || Py_False == v) ? 0 : 1;'%f.name)
         l.append('    return 0;')
     elif t == TYPE_UINT or t == TYPE_INT:
         l.append('    %slong long tmp;'%(t == TYPE_UINT and 'unsigned ' or ''))
         l.append('    int ret;')
+        l.extend(u_check)
         if t == TYPE_UINT:
             l.append('    ret = genwrap__ull_set(v, &tmp, (%s)~0);'%f.type.typename)
         else:
@@ -86,47 +120,87 @@ def py_attrib_set(ty, f):
         l.append('        self->obj.%s = tmp;'%f.name)
         l.append('    return ret;')
     elif t == TYPE_STRING:
-        l.append('    return genwrap__string_set(v, &self->obj.%s);'%f.name)
+        l.extend(u_check)
+        l.append('    return genwrap__string_set(v, (char **)&self->obj.%s);'%f.name)
+    elif f.shadow_type is True:
+        l.extend(u_check)
+        l.append('    if ( !Py%s_Check(v) ) {'%f.type.rawname)
+        l.append('        PyErr_SetString(PyExc_TypeError, "Expected xl.%s");'%f.type.rawname)
+        l.append('        return -1;')
+        l.append('    }')
+        l.append('    if ( self->%s_ref ) {'%f.python_name)
+        l.append('        Py_DECREF(self->%s_ref);'%f.python_name)
+        l.append('    }')
+        l.append('    self->%s_ref = (Py_%s *)v;'%(f.python_name, f.type.rawname))
+        l.append('    Py_INCREF(self->%s_ref);'%f.python_name)
+        l.append('    return 0;')
     else:
         tn = f.type.typename
+        l.extend(u_check)
         l.append('    return attrib__%s_set(v, (%s *)&self->obj.%s);'%(fsanitize(tn), tn, f.name))
     l.append('}')
     return '\n'.join(l) + "\n\n"
 
+def sync_func(ty):
+    pf = shadow_fields(ty)
+    if len(pf) == 0:
+        return ''
+    l = []
+
+    l.append('void Py%s_Unshadow(Py_%s*self)\n'%(ty.rawname, ty.rawname))
+    l.append('{')
+    for f in pf:
+        l.append('    if ( self->%s_ref ) {'%f.python_name)
+        l.append('        Py_%s *x = (Py_%s *)self->%s_ref;'%(f.type.rawname, f.type.rawname, f.python_name))
+        if len(shadow_fields(f.type)):
+            l.append('        Py%s_Unshadow(x);'%f.type.rawname)
+        l.append('        memcpy(&self->obj.%s, &x->obj, sizeof(self->obj.%s));'%(f.name, f.name))
+        l.append('    }else{')
+        l.append('        memset(&self->obj.%s, 0, sizeof(self->obj.%s));'%(f.name, f.name))
+        l.append('    }')
+    l.append('}')
+    l.append('')
+    return '\n'.join(l)
+
 def py_object_def(ty):
     l = []
-    if ty.destructor_fn is not None:
-        dtor = '    %s(&self->obj);\n'%ty.destructor_fn
-    else:
-        dtor = ''
-
-    funcs="""static void Py%(rawname)s_dealloc(Py_%(rawname)s *self)
-{
-%(dtor)s    self->ob_type->tp_free((PyObject *)self);
-}
-
-static int Py%(rawname)s_init(Py_%(rawname)s *self, PyObject *args, PyObject *kwds)
+    funcs="""static int Py%s_init(Py_%s *self, PyObject *args, PyObject *kwds)
 {
     memset(&self->obj, 0, sizeof(self->obj));
     return genwrap__obj_init((PyObject *)self, args, kwds);
 }
 
-static PyObject *Py%(rawname)s_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
+static PyObject *Py%s_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
 {
-    Py_%(rawname)s *self = (Py_%(rawname)s *)type->tp_alloc(type, 0);
+    Py_%s *self = (Py_%s *)type->tp_alloc(type, 0);
     if (self == NULL)
         return NULL;
     memset(&self->obj, 0, sizeof(self->obj));
     return (PyObject *)self;
 }
 
-"""%{'rawname': ty.rawname, 'dtor': dtor}
+"""%tuple(ty.rawname for x in range(5))
+
+    funcs += sync_func(ty)
+
+
+    l.append('static void Py%s_dealloc(Py_%s *self)'%(ty.rawname, ty.rawname))
+    l.append('{')
+    if ty.destructor_fn is not None:
+        l.append('    %s(&self->obj);'%ty.destructor_fn)
+    for f in shadow_fields(ty):
+        l.append('    if ( self->%s_ref ) {'%f.python_name)
+        l.append('        Py_DECREF(self->%s_ref);'%f.python_name)
+        l.append('    }')
+    l.append('self->ob_type->tp_free((PyObject *)self);')
+    l.append('}')
+    l.append('')
 
     l.append('static PyGetSetDef Py%s_getset[] = {'%ty.rawname)
     for f in ty.fields:
-        l.append('    { .name = "%s", '%f.name)
-        l.append('      .get = (getter)py_%s_%s_get, '%(ty.rawname, f.name))
-        l.append('      .set = (setter)py_%s_%s_set },'%(ty.rawname, f.name))
+        l.append('    { .name = "%s", '%f.python_name)
+        l.append('      .get = (getter)py_%s_%s_get, '%(ty.rawname, f.python_name))
+        l.append('      .set = (setter)py_%s_%s_set },'%(ty.rawname, f.python_name))
     l.append('    { .name = NULL }')
     l.append('};')
     struct="""
@@ -196,12 +270,62 @@ def py_initfuncs(types):
     l.append('}')
     return '\n'.join(l) + "\n\n"
 
-def tree_frob(types):
-    ret = types[:]
-    for ty in ret:
-        ty.fields = filter(lambda f:f.name is not None and f.type.typename is not None, ty.fields)
+def dbg_tree(str, indent=0):
+    do_dbg = True
+    if not do_dbg:
+        return
+    print '%s%s'%(''.join('  ' for i in xrange(indent)), str)
+
+# We don't have a good translation for anonymous structures so we just
+# flatten them out recursively and replace '.' with '_'. For example
+# domain_build_info.u.hvm.pae becomes domain_build_info.u_hvm_pae
+def flatten_type(ty, path = None, depth = 0, condvar = []):
+    if not isinstance(ty, libxltypes.Aggregate):
+        return ty.fields
+
+    if path is None:
+        path = ''
+    else:
+        path = path + '.'
+
+    ret = []
+    for f in ty.fields:
+        f.name = path + f.name
+        f.python_name = f.name.replace('.', '_')
+        if isinstance(f.type, libxltypes.Aggregate) and \
+                f.type.typename is not None:
+            f.shadow_type = True
+        else:
+            f.shadow_type = False
+
+        if isinstance(f.type, libxltypes.KeyedUnion):
+            f.type.keyvar_name = path + f.type.keyvar_name
+
+        f.condlist = condvar[:]
+
+        if isinstance(f.type, libxltypes.Aggregate) and \
+                f.type.typename is None:
+            dbg_tree('(%s)'%(f.name), depth + 1)
+            if isinstance(ty, libxltypes.KeyedUnion):
+                condvar.append((f.keyvar_expr, ty.keyvar_name))
+            ret.extend(flatten_type(f.type, f.name, depth + 1, condvar))
+            if isinstance(ty, libxltypes.KeyedUnion):
+                condvar.pop()
+        else:
+            dbg_tree('%s - %s'%(f.name, f.python_name), depth + 1)
+            ret.append(f)
+
+
     return ret
 
+def frob_type(type):
+    dbg_tree('[%s]'%type.typename)
+    type.fields = flatten_type(type)
+    return type
+
+def frob_types(types):
+    return map(frob_type, types)
+
 if __name__ == '__main__':
     if len(sys.argv) < 4:
         print >>sys.stderr, "Usage: genwrap.py <idl> <decls> <defns>"
@@ -210,7 +334,7 @@ if __name__ == '__main__':
     idl = sys.argv[1]
     (_,types) = libxltypes.parse(idl)
 
-    types = tree_frob(types)
+    types = frob_types(types)
 
     decls = sys.argv[2]
     f = open(decls, 'w')
@@ -250,7 +374,7 @@ _hidden int genwrap__ll_set(PyObject *v,
 
 """ % " ".join(sys.argv))
     for ty in types:
-        f.write('/* Internal APU for %s wrapper */\n'%ty.typename)
+        f.write('/* Internal API for %s wrapper */\n'%ty.typename)
         f.write(py_wrapstruct(ty))
         f.write(py_decls(ty))
         f.write('\n')

Re: [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Ian Jackson]

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate"):
> We discussed this off-list at the time I was writing it. I think with
> one of the other guys. We decided that const char * in the interface was
> a bad idea since it imposed an allocation policy where no reason for it
> actually existed in the code. I could have worked around this by casting
> everything in the python bindings but we felt that was an ugly solution
> and not a good precedent to set.

Right.

However: There was no explanation of this change in your patch
comment.  It should probably have been a separate patch anyway.
Furthermore, if you had agreement from anyone else that this change
should be made despite the freeze I'm afraid you didn't mention it.

> You are right that it changes the code and may incorrectly free the ""
> allocated in libxl_dm.c - I could strdup() in that instance that but the
> interface is changed so we have a problem. Alternatively I can change 2
> lines in genwrap.py to emit correct code for this.

All of these are possibilities but I think this series should probably
wait for 4.2 now.

> > So I'm pushing 1-6 and the rest will have to wait for after the 4.1
> > release, I'm afraid.
> 
> Your chainsaw.... my baby... :(

There is no point saying we're having a freeze if we always let new
stuff in anyway.  I think this is beyond the scope of the exception we
should make at this stage.  Feel free to try to convince me otherwise.

Sorry.

As we have just discussed if you want to resubmit the rest of the
series in a way that doesn't change the autogenerated code, then I'm
happy to apply it on the basis that it's (a) a bugfix to the python
bindings (which are broken right now) and (b) low risk if it really
doesn't change the autogenerated code.

If you need to do something which changes the autogenerated code then
I'll be wanting a second opinion at least.

Ian.

Re: [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Ian Jackson]

I wrote:
> If you need to do something which changes the autogenerated code then
> I'll be wanting a second opinion at least.

Specifically, I'd like some a discussion of each change to the
autogenerated output, and why it is necessary/justifiable/safe.

Ian.

Re: [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Stefano Stabellini]

On Wed, 12 Jan 2011, Gianni Tedesco wrote:
> On Tue, 2011-01-11 at 19:26 +0000, Ian Jackson wrote:
> > Gianni Tedesco writes ("[Xen-devel] [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate"):
> > > pyxl: Recursively scan type-tree to produce complete binding boilerplate
> >
> > I applied patches 1-6.  They touch only pyxl and didn't break the
> > build.
> >
> > However patch 7 contains this:
> >
> > > --- a/tools/libxl/libxl.idl Tue Jan 11 16:01:07 2011 +0000
> > > +++ b/tools/libxl/libxl.idl Tue Jan 11 16:01:08 2011 +0000
> > ...
> > > -                                        ("features", string, True),
> > > +                                        ("features", string),
> >
> > This makes a substantive change to both the interface and the
> > generated code, and wasn't mentioned in the comment.  I'm not
> > convinced it's a change that's appropriate during feature freeze.
> 
> We discussed this off-list at the time I was writing it. I think with
> one of the other guys. We decided that const char * in the interface was
> a bad idea since it imposed an allocation policy where no reason for it
> actually existed in the code. I could have worked around this by casting
> everything in the python bindings but we felt that was an ugly solution
> and not a good precedent to set.
> 

I might have verbally misled Gianni to think that a change like this
would be appropriate.
Bu thinking twice about it, even though it appears correct and it is
just one line, it doesn't meet the criteria.

Re: [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Gianni Tedesco]

On Wed, 2011-01-12 at 13:00 +0000, Ian Jackson wrote:
> I wrote:
> > If you need to do something which changes the autogenerated code then
> > I'll be wanting a second opinion at least.
> 
> Specifically, I'd like some a discussion of each change to the
> autogenerated output, and why it is necessary/justifiable/safe.

OK, well the code I resubmitted just then no longer makes that change to
auto-generated code. Btw. we are talking about whether that field gets
free()'d in the destructor function.

The problem is that this field is used once in libxl and it's assigned
as "", a static const char *. In the original version of the patch I
submitted the change leads to free() being called on that which is
erroneous. Changing it to not be const is a problem because callers will
need to know to strdup() now but the compiler wont be able to tell them
that.

So, on the flip-side, my solution (patch v2) has just been to cast this
(const char *) to (char *) when we modify it from python bindings. This
is mostly fine correct the bindings handle all allocations. The problem
is that we just call the destructor function when the object in
question's (domain_build_info) goes away.

A workaround is to assign None to the field after it has been set to a
string but before the variable goes out of scope and gets collected.
It's a leak, but it's a small one. It would be a matter of a few more
lines of code in gewrap.py to make that go away. Perhaps I should make
these changes and re-submit end portion of series?

Gianni

Re: [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate

[Gianni Tedesco]

On Wed, 2011-01-12 at 13:17 +0000, Stefano Stabellini wrote:
> On Wed, 12 Jan 2011, Gianni Tedesco wrote:
> > On Tue, 2011-01-11 at 19:26 +0000, Ian Jackson wrote:
> > > Gianni Tedesco writes ("[Xen-devel] [PATCH 07 of 10] pyxl: Recursively scan type-tree to produce complete binding boilerplate"):
> > > > pyxl: Recursively scan type-tree to produce complete binding boilerplate
> > >
> > > I applied patches 1-6.  They touch only pyxl and didn't break the
> > > build.
> > >
> > > However patch 7 contains this:
> > >
> > > > --- a/tools/libxl/libxl.idl Tue Jan 11 16:01:07 2011 +0000
> > > > +++ b/tools/libxl/libxl.idl Tue Jan 11 16:01:08 2011 +0000
> > > ...
> > > > -                                        ("features", string, True),
> > > > +                                        ("features", string),
> > >
> > > This makes a substantive change to both the interface and the
> > > generated code, and wasn't mentioned in the comment.  I'm not
> > > convinced it's a change that's appropriate during feature freeze.
> > 
> > We discussed this off-list at the time I was writing it. I think with
> > one of the other guys. We decided that const char * in the interface was
> > a bad idea since it imposed an allocation policy where no reason for it
> > actually existed in the code. I could have worked around this by casting
> > everything in the python bindings but we felt that was an ugly solution
> > and not a good precedent to set.
> > 
> 
> I might have verbally misled Gianni to think that a change like this
> would be appropriate.
> Bu thinking twice about it, even though it appears correct and it is
> just one line, it doesn't meet the criteria.

Well, this would have been back in November/December time, maybe
earlier. I had even forgot all about it. It may have been appropriate
then but not at this late stage.

Gianni