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

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Fri, 2010-08-27 at 15:51 +0100, Ian Campbell wrote:
> 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.

Hmm, string list and kv list it appears. Well in that case they ought be
renamed _free() - it's very confusing otherwise.

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

That's fine with me as long as a decent naming convention is kept.


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