Re: [libvirt] [PATCH] OpenVZ xml refactoring

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,