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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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

Actually, what was wrong/confusing was that the destroy function was
written such that it used one of the references within the type in order
to pass-by-reference instead defining the type as pass-by-reference in
the IDL. The existing code works and is "correct" but is semantically
wrong.

I've just sent out a short series of patches to the autogenerator which
includes the fix for this issue.

Ian.


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