On Tue, Nov 03, 2009 at 04:40:01PM +0100, Jiri Denemark wrote:
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list