[Xen-devel] Re: [PATCH,v2]: add libxl python binding

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Fri, 2010-09-10 at 18:03 +0100, Gianni Tedesco wrote: 
> On Fri, 2010-09-10 at 17:45 +0100, Ian Campbell wrote:
> > On Fri, 2010-09-10 at 16:49 +0100, Gianni Tedesco wrote:
> > > Changes since v1:
> > >  - Interpret keyword arguments in initializers: eg. xl.device_pci(bus=1, 
> > > dev=2, fn=3)
> > >  - Implement domain pause/unpause/rename/pci_add/pci_del and pci_parse
> > >  - Proper type-checking in all (I hope) code-paths
> > >  - Improve API for accessing auto-generated code and types
> > > Changes since RFC:
> > >  - split auto-generated code in to c and h files
> > >  - un-break the build system
> > >  - fix ocaml binding due to libxl API change
> > >  - lot's of tidy-ups too numerous to mention
> > >
> > > -----8<---------------------------------------------------------------
> > > Introduce python binding for libxl. The binding is not yet complete but
> > > serveral methods are implemented and tested. Those which are implemented
> > > provide examples of the two or three basic patterns that most future
> > > methods should follow.
> > >
> > > Over 5,000 lines of boilerplate is automatically generated to wrap and
> > > export all relevant libxl structure definitions. There are a few places
> > > where such code cannot be fully auto-generated and special hooks are
> > > declared and stubbed where, for example, conversion between
> > > libxl_file_reference and a python file object is required.
> >
> > I'm not qualified to comment on the actual generated bindings themselves
> > but the tiny bit which remains looks good to me.
> >
> > One perhaps interesting tip:
> > > +    funcs="""static void Py%s_dealloc(Py_%s *self)
> > > +{
> > > +%s    self->ob_type->tp_free((PyObject *)self);
> > [...etc...]
> > > +}
> > > +"""%((ty.rawname, ty.rawname, dtor) + tuple(ty.rawname for x in 
> > > range(5)))
> >
> > String formatting in python can take a dictionary and then %(foo)s will
> > expand by looking for the key 'foo' in the dict which would likely
> > simplify (and help self-document) stuff with this pattern and get rid of
> > the "x in range 5" stuff. I bet you had a hell of a job lining up all
> > the %s's with their arguments correctly ;-)
> 
> Yeah, well, apart from the one case where it was the same thing 15
> times. But yes, I was aware of that usage but never used it before and
> tbh I hacked this up real quick so there's a few warts like that.
> 
> > e.g.
> > """static void Py%(rawname)s_dealloc(Py_%(rawname)s... etc
> > """ % {'rawname': ty.rawname, 'dtor': dtor... etc}
> >
> > etc
> >
> > I also think you could get rid of the long """ string containing hand
> > written helpers by putting all that stuff in a pyxl.h and/or xl.c. I'd
> > much prefer to edit the hand-written C code in a .h file where syntax
> > highlighting will work properly etc. If you generate the relevant
> > prototypes correctly then declaration shouldn't be too much of an issue.
> 
> Yeah, the definitions of the genwrap__*_(get|set) funcs can move to xl.c
> which makes it a lot tidier.
> 
> With that bit of code motion done (see patch below) it could make sense
> to add a few python_* attributes in libxltypes.c so that we could, for
> example, say
> 
> x = Type(..., python_methods=PyFoo_Methods, python_init=Foobar_Init)
> 
> which would allow, eg: pci = xl.device_pci(); pci.parse("00:11.2") or
> allow a non keyword initialiser, eg: pci = xl.device_pci("00:11.2")

I don't know that adding language specific stuff to the IDL is any
better than just recognising specific type names which would like
special handling in the generator. 

> Anyway, I don't think this stops us getting the code in the tree cos
> such things can be done in a future patch.

Agreed.

Two things though, firstly the patch to Makefile didn't apply for me in
any version you've sent because tabs have become spaces. Might be an
issue on my end though.

Secondly: 
xen/lowlevel/xl/xl.c: In function 'fixed_bytearray_set':
xen/lowlevel/xl/xl.c:164: error: implicit declaration of function 
'PyByteArray_Check'
xen/lowlevel/xl/xl.c:165: error: implicit declaration of function 
'PyByteArray_Size'
xen/lowlevel/xl/xl.c:166: error: implicit declaration of function 
'PyByteArray_AsString'
xen/lowlevel/xl/xl.c:166: error: assignment makes pointer from integer without 
a cast
xen/lowlevel/xl/xl.c: In function 'fixed_bytearray_get':
xen/lowlevel/xl/xl.c:195: error: implicit declaration of function 
'PyByteArray_FromStringAndSize'
xen/lowlevel/xl/xl.c:195: error: return makes pointer from integer without a 
cast
error: command 'gcc' failed with exit status 1

This is with Python 2.5 (default on Debian Lenny so, unfortunately,
still worth supporting). I guess PyByteArray is new in >2.5? I don't see
a cunning way around it other than making MAC and UUID first class
objects instead of wrappers around byte array -- does that make the
syntax in the user icky?

Oh, and I think you want to add the new generated files to .hgignore.

Ian.



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