Re: [Xen-devel] [PATCH 1/2] xl: add cpuid parameter

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2010-08-27 at 15:07 +0100, Gianni Tedesco wrote:
> On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:

Thanks Andre.

> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 099d82e..da9c7fd 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -98,6 +98,20 @@ void
> > libxl_key_value_list_destroy(libxl_key_value_list kvl)
> >      free(kvl);
> >  }
> >  
> > +void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
> > +{
> > +    int i, j;
> > +
> > +    if (cpuid == NULL)
> > +        return;
> > +    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > +        for (j = 0; j < 4; j++)
> > +            if (cpuid[i].policy[j] != NULL)
> > +                free(cpuid[i].policy[j]);
> > +    }
> > +    free(cpuid);
> > +}
> 
> This can be auto-generated.

The type is defined as a Builtin in the IDL, in that case hand coding
the destructor is valid.

>  Also libxl_*_destroy() functions never call
> free on the passed pointer. Hence ending _destroy() rather than _free().

There are some exceptions, such as the FOO_list builtins but as it
stands I don't think this is one of them. The free() _should_ have come
from the use of the Reference type in the IDL. This would be the first
non-const use of Reference it though so it is possible that gentypes.py
is not 100% correct in its handling of Reference types.

However I think overall it would be better to define an explicit list
type e.g. "typedef libxl_cpuid_type *libxl_cpuid_type_list" and use that
as the IDL builtin (without the Reference since it already has one),
similar to the existing string list builtins. In that case it would be
valid for libxl_cpuid_type_list_destroy to free the actual list as well
as the contents.

I don't particularly like the name "libxl_cpuid_type" though, is there a
more descriptive name? libxl_cpuid_policy perhaps? The opaque arrays
could do with a comment or two as well.

Ian.


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