Hi, this one adds a cpuid parameter into libxl_domain_build_info and uses it''s content while setting up the domain. This is a placeholder for now, since the parsing is only implemented in the next patch. Please review, I am especially interested if I got the IDL stuff right. Regards, Andre. Signed-off-by: Andre Przywara <andre.przywara@amd.com> -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:> 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. Also libxl_*_destroy() functions never call free on the passed pointer. Hence ending _destroy() rather than _free(). _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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@lists.xensource.com http://lists.xensource.com/xen-devel
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@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2010-08-27 at 15:51 +0100, Ian Campbell wrote:> 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.I think there is a bug in this handling and I think this patch fixes it. Subject: libxl: correctly free Reference types in autogenerated destroy functions References types should be recursively destroyed and then the actual reference itself should be destroyed. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> diff -r a1612bdf9cb3 tools/libxl/gentypes.py --- a/tools/libxl/gentypes.py Fri Aug 27 15:40:05 2010 +0100 +++ b/tools/libxl/gentypes.py Fri Aug 27 15:57:25 2010 +0100 @@ -64,6 +64,11 @@ def libxl_C_type_destroy(ty, v, referenc deref = v + "->" else: deref = v + "." + + if ty.passby == libxltypes.PASS_BY_REFERENCE and not reference: + makeref = "&" + else: + makeref = "" s = "" if isinstance(ty, libxltypes.KeyedUnion): @@ -76,6 +81,8 @@ def libxl_C_type_destroy(ty, v, referenc s += "}\n" elif isinstance(ty, libxltypes.Reference): s += libxl_C_type_destroy(ty.ref_type, v, True, indent, v) + if ty.destructor_fn is not None: + s += "%s(%s);\n" % (ty.destructor_fn, makeref + v) elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.destructor_fn is None): for f in [f for f in ty.fields if not f.const]: @@ -84,11 +91,6 @@ def libxl_C_type_destroy(ty, v, referenc else: s += libxl_C_type_destroy(f.type, deref + f.name, False, "", deref) else: - if ty.passby == libxltypes.PASS_BY_REFERENCE and not reference: - makeref = "&" - else: - makeref = "" - if ty.destructor_fn is not None: s += "%s(%s);\n" % (ty.destructor_fn, makeref + v) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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@lists.xensource.com http://lists.xensource.com/xen-devel
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@lists.xensource.com http://lists.xensource.com/xen-devel
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 I thought about this, for a bit too long because it is subtle, but in the end I agree. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel