[libvirt] [PATCH 2/2] XML parsing/formating code for CPU flags

[Daniel Veillard]

On Tue, Nov 03, 2009 at 04:40:01PM +0100, Jiri Denemark wrote:
> 
> Signed-off-by: Jiri Denemark <jdenemar at 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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/