Re: [Xen-devel] [PATCH 2 of 3] support of cpupools in xl: commands and library changes

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Fri, 2010-10-08 at 09:52 +0100, Ian Campbell wrote:
> On Fri, 2010-10-08 at 09:41 +0100, Juergen Gross wrote:
> > On 10/06/10 15:47, Gianni Tedesco wrote:
> > > On Tue, 2010-10-05 at 14:45 +0100, Juergen Gross wrote:
> > >> diff -r cfce8e755505 -r baee85a24411 tools/python/xen/lowlevel/xl/xl.c
> > >> --- a/tools/python/xen/lowlevel/xl/xl.c Tue Oct 05 14:19:13 2010 +0200
> > >> +++ b/tools/python/xen/lowlevel/xl/xl.c Tue Oct 05 15:26:24 2010 +0200
> > >> @@ -208,6 +208,11 @@
> > >>       return -1;
> > >>   }
> > >>
> > >> +int attrib__uint64_t_ptr_set(PyObject *v, uint64_t * *pptr)
> > >> +{
> > >> +    return -1;
> > >> +}
> > >> +
> > >>   int attrib__libxl_cpumap_set(PyObject *v, libxl_cpumap *pptr)
> > >>   {
> > >>       return -1;
> > >> @@ -254,6 +259,11 @@
> > >>   }
> > >>
> > >>   PyObject *attrib__libxl_cpuid_policy_list_get(libxl_cpuid_policy_list
> > >> *pptr)
> > >> +{
> > >> +    return NULL;
> > >> +}
> > >> +
> > >> +PyObject *attrib__uint64_t_ptr_get(uint64_t * *pptr)
> > >>   {
> > >>       return NULL;
> > >>   }
> > >
> > > Because of using Reference(Uint64) directly in the idl - the python
> > > bindings autogenerate these type-marshalling functions. It's not quite
> > > clear how these are supposed to be implemented in a generic way! :)
> > >
> > > There ought to be a builtin type for this ala libxl_uuid and other types
> > > to generate correct bindings.
> > 
> > Gianni, I suppose I need a builtin for the complete libxl_cpumap type, not
> > only for the uint64_t array due to the size info which is needed to access
> > the array correctly.

Yes exactly, creating a full struct as you have done is "the right way
to do it"(tm). See detailed comments below.

> Yes, I think so.
> 
> There is no way in the IDL to have a pure bitmap type which refers to a
> different field in the containing structure to get the length, and doing
> this would likely be unnecessarily complicated.
>
> > I'm not sure how to translate this into the correct bindings. I think the
> > cpumap should be translated into a python list.
> 
> Yes, I think lists make sense.
> 
> > Which methods do I need to include in xc.c?
> 
> I think it will become clear when you try to build it, just follow the
> build errors ;-)
>
> > Or would it be okay to add just some minimal dummy
> > functions and put in the functionality if needed?

Yes exactly as Ian says. You will notice plenty of similar stubbed out
and unimplemented functions in the python bindings that we can do such
nice things with later. If you would like to flesh out the cpupool stuff
at the same time as adding it then that would be much appreciated but
given how incomplete the binding is we can't say it's 'mandatory' and
keep a straight face.

> Personally, given that you are planning to change the type from uint64_t
> to bytes I'd be happy if you stubbed them out for uint64_t for now and
> only implemented properly during the conversion.

Yeha on closer inspection, this is all kind of moot from the binding
perspective. What needs to happen is for libxl_cpumap to be the builtin
type defined in libxl.h (and possibly c&p any destructor functions that
were generated in to libxl.c). We don't need to generate bindings for an
object who's attributes are: (array, length)... Similar to
libxl_string_list.

In that case the existing attrib__libxl_cpumap_(set|get) for cpupoolinfo
can do all of the necessary work of converting to and from python lists
and no patch is required to tools/python/.../xl.c.

But yes, no point doing an non-dummy implementation of that if it's only
going to change before the rest of the binding is actually written! :)

Gianni


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