[libvirt] [PATCH] OpenVZ xml refactoring

[Daniel Veillard]

On Fri, Jul 25, 2008 at 04:44:09PM +0400, Evgeniy Sokolov wrote:
> Patch switch OpenVZ XML format to generic.
> main changes:
> - I used generic virDomainNetDef to define network in container.
>   And wrote function to apply virDomainNetDef for container.
>   Method virDomainNetDefParseXML is public now.
> - tag "container" is changed to "devices"
> - changed format of tag "filesystem"
> - added processing "vcpu" tag.

  Generally looks fine to me, just a few remarks

> +/* Parse filesystem section
> +Sample:
> +<filesystem type="template">
> +      <source name="fedora-core-5-i386"/>
> +      <quota type="size" max="10000"/>
> +      <quota type="inodes" max="100"/>
> +</filesystem>
> +*/
> +static int openvzParseDomainFS(virConnectPtr conn,
> +                               struct openvz_fs_def *fs,
> +                               xmlXPathContextPtr ctxt)
> +{
> +    xmlNodePtr cur, obj;
> +    char *type;
> +
> +    obj = virXPathNode("/domain/devices/filesystem[1]", ctxt);
> +    if (obj == NULL) {
> +        openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                                   _("missing filesystem tag"));
> +        return -1;
> +    }

  hum, maybe use virXPathNodeSet and checking you won't get more than one ?

>      /* Extract domain uuid */
> -    obj = xmlXPathEval(BAD_CAST "string(/domain/uuid[1])", ctxt);
> -    if ((obj == NULL) || (obj->type != XPATH_STRING) ||
> -        (obj->stringval == NULL) || (obj->stringval[0] == 0)) {
> +    prop = virXPathString("string(./uuid[1])", ctxt);
> +    if (!prop) {
>          int err;

  Hum, if you start using relative XPath queries like that it's a good idea
to make sure ctxt->node is set to the node you're starting the lookup from
I don't see that in the patch, maybe I missed it...

> +    } else {
> +        if (virUUIDParse(prop, def->uuid) < 0) {
> +            openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                                _("malformed uuid element"));
> +            goto bail_out;
> +        }
> +        VIR_FREE(prop);

[...]
> +    //TODO: processing NAT and phisical device

  typo physical :-)

  In general that looks way cleaner to me,
I will give it a few nmore days and apply, unless you suggest another version,

   thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/