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

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Thu, 2010-09-09 at 12:48 +0100, Ian Campbell wrote:
> On Thu, 2010-09-09 at 12:18 +0100, Gianni Tedesco wrote:
> > Changes since last time:
> >  - 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
> > 
> > Please consider and apply :)
> 
> Like it.
> 
> I think it'd be nice to separate out stuff like the PCI API change and
> IDL changes into preparatory patches. Patches which do one logical thing
> are generally easier to review (and useful for bisecting and such too).

Yeah, that's not a problem, in this case I can just split it out by
filename.

> > -----8<---------------------------------------------------------------
> > Introduce python binding for libxl. The binding is not yet complete but
> > list_domains, domid_to_name, domain_shutdown and domain_destroy methods
> > are implemented and tested. These functions provide examples of the two
> > basic patterns that all future methods should follow.
> > 
> > About 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 think this is acceptable for Builtin() types and is really part of the
> meaning of builtin. (except as I read on I realise that
> libxl_file_reference isn't actually a Builtin like I thought, more on
> that later!)
>
> > There are minor changes to libxltypes.py and the libxl.idl file and the
> > new rules are documented. I have also removed the use of a nasty union
> > in libxl_device_pci structure which was making the binding more
> > complicated than necessary and for no good reason (exporting random bits
> > of low level ABI that libxl would rather hide anyway). Finally there is
> > a corresponding (untested) change to the ocaml binding to fix a compile
> > error and maintain previous API in ocaml.
> > 
> > Signed-off-by: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
> 
> > +Type.internal (default: False):
> > + Indicates that type is not suitable for export, for example, via accessors
> > + in auto-generated language bindings.
> 
> Maybe the issue here is that libxl_file_reference should really be a
> Builtin? It's suspicious that it is the only non-Builtin to which both
> this flag and autogenerate_destructor=False apply.

I think your analysis is right and this in fact works for me. I had
misunderstood full meaning of Builtins even though you had mentioned the
(builtins,types) tuple thing. Fixed this and it works fine.

> If that doesn't make sense then perhaps a new sub-class of Builtin might
> be more appropriate than an attribute (not sure what it would be called
> though, SuperBuiltin ;-)). I'm not sure that a non-Builtin should ever
> be internal and this would help prevent that.

Meh

> > diff -r b19856f6dd76 tools/python/Makefile
> > --- a/tools/python/Makefile     Thu Sep 09 09:24:24 2010 +0100
> > +++ b/tools/python/Makefile     Thu Sep 09 12:06:50 2010 +0100
> > @@ -20,6 +20,10 @@ genpath-target = $(call buildmakevars2fi
> > 
> >  .PHONY: build buildpy
> >  buildpy: genpath
> > +       PYTHONPATH=$(XEN_ROOT)/tools/libxl $(PYTHON) genwrap.py \
> > +               $(XEN_ROOT)/tools/libxl/libxl.idl \
> > +               xen/lowlevel/xl/_pyxl_types.h \
> > +               xen/lowlevel/xl/_pyxl_types.c
> >         CC="$(CC)" CFLAGS="$(CFLAGS)" $(PYTHON) setup.py build
> 
> Needs some additional dependencies on genwrap.py and
> $(XE_ROOT)/tools/libxl/libxl.idl?

Yes, and libxltypes.py, fixed.


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