[PATCH 0 of 4] Python libxl binding updates

[PATCH 0 of 4] Python libxl binding updates

[Gianni Tedesco]

Various, mostly minor, updates to the libxl python binding which address both
correctness and usability issues.

Gianni

[PATCH 1 of 4] pyxl: Fix refcounting of Py_None

[Gianni Tedesco]

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


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1291900443 0
# Node ID e4610644ba98610d017d149b0e5aaed2aaecd346
# Parent  5ac189556629e2db96196b2556d870f7cefc959f
pyxl: Fix refcounting of Py_None

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

diff -r 5ac189556629 -r e4610644ba98 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 10:09:59 2010 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 13:14:03 2010 +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);
 }
 
@@ -354,6 +356,7 @@ static PyObject *pyxl_list_domains(XlObj
             goto err_mem;
         memcpy(&di->obj, cur, sizeof(di->obj));
         PyList_SetItem(list, i, (PyObject *)di);
+        Py_DECREF(di);
     }
 
     free(info);
@@ -389,6 +392,7 @@ static PyObject *pyxl_domain_shutdown(Xl
         PyErr_SetString(xl_error_obj, "cannot shutdown domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -401,6 +405,7 @@ static PyObject *pyxl_domain_destroy(XlO
         PyErr_SetString(xl_error_obj, "cannot destroy domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -413,6 +418,7 @@ static PyObject *pyxl_domain_pause(XlObj
         PyErr_SetString(xl_error_obj, "cannot pause domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -425,6 +431,7 @@ static PyObject *pyxl_domain_unpause(XlO
         PyErr_SetString(xl_error_obj, "cannot unpause domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -438,6 +445,7 @@ static PyObject *pyxl_domain_rename(XlOb
         PyErr_SetString(xl_error_obj, "cannot rename domain");
         return NULL;
     }
+    Py_INCREF(Py_None);
     return Py_None;
 }
 
@@ -457,6 +465,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;
 }
 
@@ -477,6 +486,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;
 }

Re: [PATCH 1 of 4] pyxl: Fix refcounting of Py_None

[Gianni Tedesco]

On Thu, 2010-12-09 at 13:54 +0000, Gianni Tedesco wrote:
> @@ -354,6 +356,7 @@ static PyObject *pyxl_list_domains(XlObj
>              goto err_mem;
>          memcpy(&di->obj, cur, sizeof(di->obj));
>          PyList_SetItem(list, i, (PyObject *)di);
> +        Py_DECREF(di);
>      }

This part is erroneous because PyList_SetItem, unlike PyList_(Insert|
Append), steals a reference.

Let's NAK this entire series and I'll respin a fixed a version, possibly
with some other updates.

Gianni

Re: [PATCH 1 of 4] pyxl: Fix refcounting of Py_None

[Ian Jackson]

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 1 of 4] pyxl: Fix refcounting of Py_None"):
> Let's NAK this entire series and I'll respin a fixed a version, possibly
> with some other updates.

OK.  I was going to let them sit and stew for a bit anyway to see if
anyone objected.  I'm no expert on the Python FFI so I'll just have to
trust you and the list.

Ian.

Re: [PATCH 1 of 4] pyxl: Fix refcounting of Py_None

[Gianni Tedesco]

On Fri, 2010-12-10 at 17:54 +0000, Ian Jackson wrote:
> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 1 of 4] pyxl: Fix refcounting of Py_None"):
> > Let's NAK this entire series and I'll respin a fixed a version, possibly
> > with some other updates.
> 
> OK.  I was going to let them sit and stew for a bit anyway to see if
> anyone objected.  I'm no expert on the Python FFI so I'll just have to
> trust you and the list.

Fair enough, my main rationale was getting them out there so that nobody
duplicates the effort.

There's another trivial fix and more substantial updates on the way too.

Gianni

[PATCH 2 of 4] pyxl: Export integer constants required for use as libxl paramaters

[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 1291900495 0
# Node ID 0e64db4b86138afcf49d7f2273fad4af5e7da997
# Parent  e4610644ba98610d017d149b0e5aaed2aaecd346
pyxl: Export integer constants required for use as libxl paramaters

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

diff -r e4610644ba98 -r 0e64db4b8613 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 13:14:03 2010 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 13:14:55 2010 +0000
@@ -627,6 +627,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;
@@ -647,6 +649,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 3 of 4] pyxl: Allow all xl types to be used as base classes

[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 1291900579 0
# Node ID 521af80c82bb6fc8e4403bbe2313b3a8b3c78841
# Parent  0e64db4b86138afcf49d7f2273fad4af5e7da997
pyxl: Allow all xl types to be used as base classes

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

diff -r 0e64db4b8613 -r 521af80c82bb tools/python/genwrap.py
--- a/tools/python/genwrap.py	Thu Dec 09 13:14:55 2010 +0000
+++ b/tools/python/genwrap.py	Thu Dec 09 13:16:19 2010 +0000
@@ -148,7 +148,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 0e64db4b8613 -r 521af80c82bb tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 13:14:55 2010 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 13:16:19 2010 +0000
@@ -604,7 +604,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 4 of 4] pyxl: Add _get_attribs() method to all classes

[Gianni Tedesco]

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


# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@citrix.com>
# Date 1291900689 0
# Node ID 35dd6a11ce56488d01f9f1cb365e6035687390ca
# Parent  521af80c82bb6fc8e4403bbe2313b3a8b3c78841
pyxl: Add _get_attribs() method to all classes

This returns a dictionary of (attrib_name, value) tuples corresponding to all
fields of the libxl data-type that is being wrapped. This allows for creation
of super-classes of those types after they have been returned from libxl.

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

diff -r 521af80c82bb -r 35dd6a11ce56 tools/python/genwrap.py
--- a/tools/python/genwrap.py	Thu Dec 09 13:16:19 2010 +0000
+++ b/tools/python/genwrap.py	Thu Dec 09 13:18:09 2010 +0000
@@ -127,6 +127,17 @@ static PyObject *Py%(rawname)s_new(PyTyp
     l.append('    { .name = NULL }')
     l.append('};')
     struct="""
+static PyObject *Py%s__get_attribs(PyObject *self, PyObject *args)
+{
+    return genwrap__get_attribs(self, Py%s_getset);
+}
+
+static PyMethodDef Py%s_methods[] = {
+    {"_get_attribs", (PyCFunction)Py%s__get_attribs, METH_NOARGS,
+         "Get all attributes as dictionary"},
+    {NULL, NULL, 0, NULL}
+};
+
 static PyTypeObject Py%s_Type= {
     PyObject_HEAD_INIT(NULL)
     0,
@@ -156,7 +167,7 @@ static PyTypeObject Py%s_Type= {
     0,                            /* tp_weaklistoffset */
     NULL,                         /* tp_iter           */
     NULL,                         /* tp_iternext       */
-    NULL,                         /* tp_methods        */
+    Py%s_methods,                 /* tp_methods        */
     NULL,                         /* tp_members        */
     Py%s_getset,                  /* tp_getset         */
     NULL,                         /* tp_base           */
@@ -178,7 +189,7 @@ int Py%s_Check(PyObject *self)
 {
     return (self->ob_type == &Py%s_Type);
 }
-"""%tuple(ty.rawname for x in range(15))
+"""%tuple(ty.rawname for x in range(20))
     return funcs + '\n'.join(l) + "\n" + struct
 
 def py_initfuncs(types):
@@ -237,6 +248,9 @@ _hidden void genwrap__init(PyObject *m);
 /* Generic type initialiser */
 _hidden int genwrap__obj_init(PyObject *self, PyObject *args, PyObject *kwds);
 
+/* Generic type dictionary getter */
+_hidden PyObject *genwrap__get_attribs(PyObject *self, PyGetSetDef *getsets);
+
 /* 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);
diff -r 521af80c82bb -r 35dd6a11ce56 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 13:16:19 2010 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Thu Dec 09 13:18:09 2010 +0000
@@ -57,6 +57,31 @@
 
 static PyObject *xl_error_obj;
 
+PyObject *genwrap__get_attribs(PyObject *self, PyGetSetDef *getsets)
+{
+    PyObject *dict;
+    PyGetSetDef *cur;
+
+    dict = PyDict_New();
+    if ( NULL == dict ) {
+        PyErr_SetString(PyExc_MemoryError, "Allocating dictionary");
+        return NULL;
+    }
+
+    for(cur = getsets; cur->name; cur++) {
+        PyObject *tmp;
+        tmp = (*cur->get)(self, cur->closure);
+        if ( NULL == tmp ) {
+            Py_DECREF(dict);
+            return NULL;
+        }
+        PyDict_SetItemString(dict, cur->name, tmp);
+        Py_DECREF(tmp);
+    }
+
+    return dict;
+}
+
 int genwrap__obj_init(PyObject *self, PyObject *args, PyObject *kwds)
 {
     PyObject *key, *value;

Re: [PATCH 4 of 4] pyxl: Add _get_attribs() method to all classes

[Gianni Tedesco]

On Thu, 2010-12-09 at 13:54 +0000, Gianni Tedesco wrote:
> tools/python/genwrap.py           |  18 ++++++++++++++++--
>  tools/python/xen/lowlevel/xl/xl.c |  25 +++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
> 
> 
> # HG changeset patch
> # User Gianni Tedesco <gianni.tedesco@citrix.com>
> # Date 1291900689 0
> # Node ID 35dd6a11ce56488d01f9f1cb365e6035687390ca
> # Parent  521af80c82bb6fc8e4403bbe2313b3a8b3c78841
> pyxl: Add _get_attribs() method to all classes
> 
> This returns a dictionary of (attrib_name, value) tuples corresponding to all
> fields of the libxl data-type that is being wrapped. This allows for creation
> of super-classes of those types after they have been returned from libxl.

I don't think this is strictly necessary now as it can be implemented in
python code. Ian if you do apply these, you should hold off on this one.

Gianni

Re: [PATCH 0 of 4] Python libxl binding updates

[Gianni Tedesco]

On Thu, 2010-12-09 at 13:53 +0000, Gianni Tedesco wrote:
> Various, mostly minor, updates to the libxl python binding which address both
> correctness and usability issues.

I have a new version of this patch-set which currently contains 10
patches and brings the bindings up to a state where we can create
domains and all that is left is basic plumbing through of libxl
functions/methods.

Do you think it would be possible to get a freeze exception so that I
can re-submit this next week? It provides new features but only to the
xl python bindings which are unused and unusable in their current state
anyway.

There are bugfixes:
 1. pyxl-fix-refcounting
 2. pyxl-unmuddle-pause-unpause

Some IDL and code-generation work which touches things in the
tools/libxl tree but doesn't change any C code (not even the
auto-generated headers and destructors). I have decided not to abolish
keyed unions after all.
 1. pyxl-marshall-substructures-and-unions
 2. xl-idl-arrays
 3. xl-idl-enums
 4. pyxl-builtin-type-marshallers

Exporting more things:
 1. pyxl-add-int-constants
 2. pyxl-allow-subclassing
 3. pyxl-pci-passthrough
 4. pyxl-domain-create

The last of which depends on my previous patch to re-work domain
creation in libxl.

Thanks

Gianni

Re: [PATCH 0 of 4] Python libxl binding updates

[Gianni Tedesco]

On Fri, 2011-01-07 at 16:25 +0000, Gianni Tedesco wrote:
> On Thu, 2010-12-09 at 13:53 +0000, Gianni Tedesco wrote:
> > Various, mostly minor, updates to the libxl python binding which address both
> > correctness and usability issues.
> 
> I have a new version of this patch-set which currently contains 10
> patches and brings the bindings up to a state where we can create
> domains and all that is left is basic plumbing through of libxl
> functions/methods.
> 
> Do you think it would be possible to get a freeze exception so that I
> can re-submit this next week? It provides new features but only to the
> xl python bindings which are unused and unusable in their current state
> anyway.
> 
> There are bugfixes:
>  1. pyxl-fix-refcounting
>  2. pyxl-unmuddle-pause-unpause
> 
> Some IDL and code-generation work which touches things in the
> tools/libxl tree but doesn't change any C code (not even the
> auto-generated headers and destructors). I have decided not to abolish
> keyed unions after all.
>  1. pyxl-marshall-substructures-and-unions
>  2. xl-idl-arrays
>  3. xl-idl-enums
>  4. pyxl-builtin-type-marshallers

Actually, sorry, that's slightly incorrect. In the final patch
'pyxl-domain-create' the libxl_domain_config struct comes out of libxl.h
and in to the IDL but the net is an identical C program.

That's why arrays and enums are implemented in the IDL.

Gianni

Re: [PATCH 0 of 4] Python libxl binding updates

[Ian Jackson]

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 0 of 4] Python libxl binding updates"):
> Do you think it would be possible to get a freeze exception so that I
> can re-submit this next week? It provides new features but only to the
> xl python bindings which are unused and unusable in their current state
> anyway.

I think this is fine but we will look very closely at changes to
common code, specifically changes to the IDL and IDL compiler, for
which I'll manually verify that the generated code is unchanged.

Can you please structure your changes to group together all the
changes to the IDL compiler ?

Thanks,
Ian.

Re: [PATCH 0 of 4] Python libxl binding updates

[Gianni Tedesco]

On Tue, 2011-01-11 at 16:20 +0000, Ian Jackson wrote:
> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 0 of 4] Python libxl binding updates"):
> > Do you think it would be possible to get a freeze exception so that I
> > can re-submit this next week? It provides new features but only to the
> > xl python bindings which are unused and unusable in their current state
> > anyway.
> 
> I think this is fine but we will look very closely at changes to
> common code, specifically changes to the IDL and IDL compiler, for
> which I'll manually verify that the generated code is unchanged.
> 
> Can you please structure your changes to group together all the
> changes to the IDL compiler ?

Each incremental change to the generic IDL system is in lock-step with
the changes to python binding because the python wrapper generator
imports libxltypes.py from tools/libxl/ so a mis-match between them
would cause build failures.

Perhaps it is easier if I point out the patches which make these
changes:

[PATCH 08 of 10] xl: support array types in IDL
- the main functional change

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

[PATCH 10 of 10] pyxl: Export libxl_domain_create_new() to python binding
- actually uses the code implemented above by adding a new type to libxl.idl without
  this patch, the output doesn't change at all.

Gianni