[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

> -----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.

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.

> 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?

Ian.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.