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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Fri, 2010-08-27 at 15:56 +0100, Gianni Tedesco (3P) wrote:
> 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.

In these cases the type itself is a list rather than having a list of a
given type, this is why it is appropriate for the destructor to destroy
the list itself as well as the contents.

It's a bit subtle but I think it is correct and consistent with the
behaviour of Reference types for example (modulo the bug I just sent a
patch for).

Ian.


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