Re: [libvirt] [PATCH 2/2] XML parsing/formating code for CPU flags
On Tue, Nov 03, 2009 at 04:40:01PM +0100, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
include/libvirt/virterror.h | 1 +
src/Makefile.am | 9 +-
src/conf/capabilities.c | 31 ++++-
src/conf/capabilities.h | 6 +
src/conf/cpu_conf.c | 345 +++++++++++++++++++++++++++++++++++++++++++
src/conf/cpu_conf.h | 110 ++++++++++++++
src/conf/domain_conf.c | 15 ++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 8 +
src/util/virterror.c | 3 +
10 files changed, 527 insertions(+), 3 deletions(-)
create mode 100644 src/conf/cpu_conf.c
create mode 100644 src/conf/cpu_conf.h
[...]
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
[...]
+ /*
+ * sockets, cores, and threads must all be either zero or nonzero;
+ * if all of them are zero, we don't care about topology
+ */
+ if ((!def->sockets || !def->cores || !def->threads) &&
!all_zero) {
+ virCPUReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Invalid CPU topology"));
+ goto error;
+ }
+ }
Hum, I would just ban all being 0 and drop the element in that case
which would match my suggestion at the schemas level.
+ n = virXPathNodeSet(conn, "./feature", ctxt,
&nodes);
+ if (n < 0)
+ goto error;
+
+ if (n > 0) {
+ if (VIR_ALLOC_N(def->features, n) < 0)
+ goto no_memory;
+ def->nfeatures = n;
+ }
+
+ for (i = 0 ; i < n ; i++) {
+ char *name;
[...]
+
+ if (!(name = (char *) xmlNodeGetContent(nodes[i]))
+ || *name == 0) {
+ VIR_FREE(name);
+ virCPUReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Invalid CPU feature name"));
+ goto error;
+ }
let's avoid xmlNodeGetContent() and just use the name in an attribute
it's better and simpler at this point, plus better if we want this
construct to evolve in the future.
+ len = strlen(name);
+ for (j = 0 ; j < len ; j++)
+ name[j] = c_tolower(name[j]);
hum ... are we really normalizing features names ? On one hand we
don't define the list, but on the other hand we lower-case them. Sounds
a bit weird, either we control them or not.
Patch looks fine overall, just those could of suggestions especially
about matching the proposed changes in the schemas and avoiding
tolower()
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/