WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 1/2] xl: add cpuid parameter
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Fri, 27 Aug 2010 15:51:37 +0100
Cc: Andre Przywara <andre.przywara@xxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Keir Fraser <Keir.Fraser@xxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Fri, 27 Aug 2010 07:52:20 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1282918055.3731.111.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <4C77B616.30900@xxxxxxx> <1282918055.3731.111.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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