[libvirt] [PATCH] Refactor storage XML parsing to be consistent with domain/network conf.

The storage driver arranges its parsing routines in a way that make them difficult to use in the test driver for non-default file parsing. This refactoring moves things to be consistent with the way domain_conf and network_conf do things. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 8 ++- src/storage_conf.c | 163 ++++++++++++++++++++++++++++++++-------------- src/storage_conf.h | 26 ++++++-- src/storage_driver.c | 8 +- src/test.c | 25 ++----- 5 files changed, 150 insertions(+), 80 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f63fa05..309d7f9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -267,7 +267,9 @@ virSecurityDriverGetModel; # storage_conf.h virStoragePoolDefFormat; virStoragePoolDefFree; -virStoragePoolDefParse; +virStoragePoolDefParseString; +virStoragePoolDefParseFile; +virStoragePoolDefParseNode; virStoragePoolLoadAllConfigs; virStoragePoolObjAssignDef; virStoragePoolObjClearVols; @@ -284,7 +286,9 @@ virStorageVolDefFindByName; virStorageVolDefFindByPath; virStorageVolDefFormat; virStorageVolDefFree; -virStorageVolDefParse; +virStorageVolDefParseFile; +virStorageVolDefParseString; +virStorageVolDefParseNode; virStoragePoolFormatDiskTypeToString; virStoragePoolFormatFileSystemTypeToString; virStoragePoolFormatFileSystemNetTypeToString; diff --git a/src/storage_conf.c b/src/storage_conf.c index 5f724dc..7d495ec 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -452,14 +452,12 @@ error: return ret; } - static virStoragePoolDefPtr -virStoragePoolDefParseDoc(virConnectPtr conn, - xmlXPathContextPtr ctxt, - xmlNodePtr root) { +virStoragePoolDefParseXML(virConnectPtr conn, + xmlXPathContextPtr ctxt) { virStoragePoolOptionsPtr options; virStoragePoolDefPtr ret; - xmlChar *type = NULL; + char *type = NULL; char *uuid = NULL; char *authType = NULL; @@ -468,13 +466,7 @@ virStoragePoolDefParseDoc(virConnectPtr conn, return NULL; } - if (STRNEQ((const char *)root->name, "pool")) { - virStorageReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("unknown root element for storage pool")); - goto cleanup; - } - - type = xmlGetProp(root, BAD_CAST "type"); + type = virXPathString(conn, "string(./@type)", ctxt); if ((ret->type = virStoragePoolTypeFromString((const char *)type)) < 0) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, _("unknown storage pool type %s"), (const char*)type); @@ -656,6 +648,32 @@ catchXMLError (void *ctx, const char *msg ATTRIBUTE_UNUSED, ...) } virStoragePoolDefPtr +virStoragePoolDefParseNode(virConnectPtr conn, + xmlDocPtr xml, + xmlNodePtr root) { + xmlXPathContextPtr ctxt = NULL; + virStoragePoolDefPtr def = NULL; + + if (STRNEQ((const char *)root->name, "pool")) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("unknown root element for storage pool")); + goto cleanup; + } + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(conn); + goto cleanup; + } + + ctxt->node = root; + def = virStoragePoolDefParseXML(conn, ctxt); +cleanup: + xmlXPathFreeContext(ctxt); + return def; +} + +static virStoragePoolDefPtr virStoragePoolDefParse(virConnectPtr conn, const char *xmlStr, const char *filename) { @@ -663,7 +681,6 @@ virStoragePoolDefParse(virConnectPtr conn, xmlParserCtxtPtr pctxt; xmlDocPtr xml = NULL; xmlNodePtr node = NULL; - xmlXPathContextPtr ctxt = NULL; /* Set up a parser context so we can catch the details of XML errors. */ pctxt = xmlNewParserCtxt (); @@ -673,10 +690,17 @@ virStoragePoolDefParse(virConnectPtr conn, pctxt->_private = conn; if (conn) virResetError (&conn->err); - xml = xmlCtxtReadDoc (pctxt, BAD_CAST xmlStr, - filename ? filename : "storage.xml", NULL, - XML_PARSE_NOENT | XML_PARSE_NONET | - XML_PARSE_NOWARNING); + if (filename) { + xml = xmlCtxtReadFile (pctxt, filename, NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + } else { + xml = xmlCtxtReadDoc (pctxt, BAD_CAST xmlStr, + "storage.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + } + if (!xml) { if (conn && conn->err.code == VIR_ERR_NONE) virStorageReportError(conn, VIR_ERR_XML_ERROR, @@ -684,12 +708,6 @@ virStoragePoolDefParse(virConnectPtr conn, goto cleanup; } - ctxt = xmlXPathNewContext(xml); - if (ctxt == NULL) { - virReportOOMError(conn); - goto cleanup; - } - node = xmlDocGetRootElement(xml); if (node == NULL) { virStorageReportError(conn, VIR_ERR_XML_ERROR, @@ -697,21 +715,33 @@ virStoragePoolDefParse(virConnectPtr conn, goto cleanup; } - ret = virStoragePoolDefParseDoc(conn, ctxt, node); + ret = virStoragePoolDefParseNode(conn, xml, node); xmlFreeParserCtxt (pctxt); - xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); return ret; cleanup: xmlFreeParserCtxt (pctxt); - xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); return NULL; } +virStoragePoolDefPtr +virStoragePoolDefParseString(virConnectPtr conn, + const char *xmlStr) +{ + return virStoragePoolDefParse(conn, xmlStr, NULL); +} + +virStoragePoolDefPtr +virStoragePoolDefParseFile(virConnectPtr conn, + const char *filename) +{ + return virStoragePoolDefParse(conn, NULL, filename); +} + static int virStoragePoolSourceFormat(virConnectPtr conn, virBufferPtr buf, @@ -916,10 +946,9 @@ virStorageSize(virConnectPtr conn, } static virStorageVolDefPtr -virStorageVolDefParseDoc(virConnectPtr conn, +virStorageVolDefParseXML(virConnectPtr conn, virStoragePoolDefPtr pool, - xmlXPathContextPtr ctxt, - xmlNodePtr root) { + xmlXPathContextPtr ctxt) { virStorageVolDefPtr ret; virStorageVolOptionsPtr options; char *allocation = NULL; @@ -935,12 +964,6 @@ virStorageVolDefParseDoc(virConnectPtr conn, return NULL; } - if (STRNEQ((const char *)root->name, "volume")) { - virStorageReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("unknown root element")); - goto cleanup; - } - ret->name = virXPathString(conn, "string(/volume/name)", ctxt); if (ret->name == NULL) { virStorageReportError(conn, VIR_ERR_XML_ERROR, @@ -1028,8 +1051,34 @@ virStorageVolDefParseDoc(virConnectPtr conn, return NULL; } - virStorageVolDefPtr +virStorageVolDefParseNode(virConnectPtr conn, + virStoragePoolDefPtr pool, + xmlDocPtr xml, + xmlNodePtr root) { + xmlXPathContextPtr ctxt = NULL; + virStorageVolDefPtr def = NULL; + + if (STRNEQ((const char *)root->name, "volume")) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("unknown root element for storage vol")); + goto cleanup; + } + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(conn); + goto cleanup; + } + + ctxt->node = root; + def = virStorageVolDefParseXML(conn, pool, ctxt); +cleanup: + xmlXPathFreeContext(ctxt); + return def; +} + +static virStorageVolDefPtr virStorageVolDefParse(virConnectPtr conn, virStoragePoolDefPtr pool, const char *xmlStr, @@ -1038,7 +1087,6 @@ virStorageVolDefParse(virConnectPtr conn, xmlParserCtxtPtr pctxt; xmlDocPtr xml = NULL; xmlNodePtr node = NULL; - xmlXPathContextPtr ctxt = NULL; /* Set up a parser context so we can catch the details of XML errors. */ pctxt = xmlNewParserCtxt (); @@ -1048,10 +1096,18 @@ virStorageVolDefParse(virConnectPtr conn, pctxt->_private = conn; if (conn) virResetError (&conn->err); - xml = xmlCtxtReadDoc (pctxt, BAD_CAST xmlStr, - filename ? filename : "storage.xml", NULL, - XML_PARSE_NOENT | XML_PARSE_NONET | - XML_PARSE_NOWARNING); + + if (filename) { + xml = xmlCtxtReadFile (pctxt, filename, NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + } else { + xml = xmlCtxtReadDoc (pctxt, BAD_CAST xmlStr, + "storage.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + } + if (!xml) { if (conn && conn->err.code == VIR_ERR_NONE) virStorageReportError(conn, VIR_ERR_XML_ERROR, @@ -1059,12 +1115,6 @@ virStorageVolDefParse(virConnectPtr conn, goto cleanup; } - ctxt = xmlXPathNewContext(xml); - if (ctxt == NULL) { - virReportOOMError(conn); - goto cleanup; - } - node = xmlDocGetRootElement(xml); if (node == NULL) { virStorageReportError(conn, VIR_ERR_XML_ERROR, @@ -1072,21 +1122,34 @@ virStorageVolDefParse(virConnectPtr conn, goto cleanup; } - ret = virStorageVolDefParseDoc(conn, pool, ctxt, node); + ret = virStorageVolDefParseNode(conn, pool, xml, node); xmlFreeParserCtxt (pctxt); - xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); return ret; cleanup: xmlFreeParserCtxt (pctxt); - xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); return NULL; } +virStorageVolDefPtr +virStorageVolDefParseString(virConnectPtr conn, + virStoragePoolDefPtr pool, + const char *xmlStr) +{ + return virStorageVolDefParse(conn, pool, xmlStr, NULL); +} + +virStorageVolDefPtr +virStorageVolDefParseFile(virConnectPtr conn, + virStoragePoolDefPtr pool, + const char *filename) +{ + return virStorageVolDefParse(conn, pool, NULL, filename); +} static int virStorageVolTargetDefFormat(virConnectPtr conn, diff --git a/src/storage_conf.h b/src/storage_conf.h index 8a4fed2..a2a164e 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -28,6 +28,8 @@ #include "util.h" #include "threads.h" +#include <libxml/tree.h> + /* Shared structs */ @@ -306,16 +308,26 @@ virStorageVolDefPtr virStorageVolDefFindByName(virStoragePoolObjPtr pool, void virStoragePoolObjClearVols(virStoragePoolObjPtr pool); -virStoragePoolDefPtr virStoragePoolDefParse(virConnectPtr conn, - const char *xml, - const char *filename); +virStoragePoolDefPtr virStoragePoolDefParseString(virConnectPtr conn, + const char *xml); +virStoragePoolDefPtr virStoragePoolDefParseFile(virConnectPtr conn, + const char *filename); +virStoragePoolDefPtr virStoragePoolDefParseNode(virConnectPtr conn, + xmlDocPtr xml, + xmlNodePtr root); char *virStoragePoolDefFormat(virConnectPtr conn, virStoragePoolDefPtr def); -virStorageVolDefPtr virStorageVolDefParse(virConnectPtr conn, - virStoragePoolDefPtr pool, - const char *xml, - const char *filename); +virStorageVolDefPtr virStorageVolDefParseString(virConnectPtr conn, + virStoragePoolDefPtr pool, + const char *xml); +virStorageVolDefPtr virStorageVolDefParseFile(virConnectPtr conn, + virStoragePoolDefPtr pool, + const char *filename); +virStorageVolDefPtr virStorageVolDefParseNode(virConnectPtr conn, + virStoragePoolDefPtr pool, + xmlDocPtr xml, + xmlNodePtr root); char *virStorageVolDefFormat(virConnectPtr conn, virStoragePoolDefPtr pool, virStorageVolDefPtr def); diff --git a/src/storage_driver.c b/src/storage_driver.c index 5a248a3..5fe22c6 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -466,7 +466,7 @@ storagePoolCreate(virConnectPtr conn, virStorageBackendPtr backend; storageDriverLock(driver); - if (!(def = virStoragePoolDefParse(conn, xml, NULL))) + if (!(def = virStoragePoolDefParseString(conn, xml))) goto cleanup; pool = virStoragePoolObjFindByUUID(&driver->pools, def->uuid); @@ -518,7 +518,7 @@ storagePoolDefine(virConnectPtr conn, virStorageBackendPtr backend; storageDriverLock(driver); - if (!(def = virStoragePoolDefParse(conn, xml, NULL))) + if (!(def = virStoragePoolDefParseString(conn, xml))) goto cleanup; if ((backend = virStorageBackendForType(def->type)) == NULL) @@ -1220,7 +1220,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; - voldef = virStorageVolDefParse(obj->conn, pool->def, xmldesc, NULL); + voldef = virStorageVolDefParseString(obj->conn, pool->def, xmldesc); if (voldef == NULL) goto cleanup; @@ -1362,7 +1362,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } - newvol = virStorageVolDefParse(obj->conn, pool->def, xmldesc, NULL); + newvol = virStorageVolDefParseString(obj->conn, pool->def, xmldesc); if (newvol == NULL) goto cleanup; diff --git a/src/test.c b/src/test.c index 2a672a3..6874d63 100644 --- a/src/test.c +++ b/src/test.c @@ -286,7 +286,7 @@ static int testOpenDefault(virConnectPtr conn) { netobj->persistent = 1; virNetworkObjUnlock(netobj); - if (!(pooldef = virStoragePoolDefParse(conn, defaultPoolXML, NULL))) + if (!(pooldef = virStoragePoolDefParseString(conn, defaultPoolXML))) goto error; if (!(poolobj = virStoragePoolObjAssignDef(conn, &privconn->pools, @@ -564,22 +564,13 @@ static int testOpenFromFile(virConnectPtr conn, goto error; } - def = virStoragePoolDefParse(conn, NULL, absFile); + def = virStoragePoolDefParseFile(conn, absFile); VIR_FREE(absFile); if (!def) goto error; } else { - xmlBufferPtr buf; - xmlSaveCtxtPtr sctxt; - - buf = xmlBufferCreate(); - sctxt = xmlSaveToBuffer(buf, NULL, 0); - xmlSaveTree(sctxt, pools[i]); - xmlSaveClose(sctxt); - if ((def = virStoragePoolDefParse(conn, - (const char *) buf->content, - NULL)) == NULL) { - xmlBufferFree(buf); + if ((def = virStoragePoolDefParseNode(conn, xml, + pools[i])) == NULL) { goto error; } } @@ -2514,7 +2505,7 @@ testStoragePoolCreate(virConnectPtr conn, virStoragePoolPtr ret = NULL; testDriverLock(privconn); - if (!(def = virStoragePoolDefParse(conn, xml, NULL))) + if (!(def = virStoragePoolDefParseString(conn, xml))) goto cleanup; pool = virStoragePoolObjFindByUUID(&privconn->pools, def->uuid); @@ -2557,7 +2548,7 @@ testStoragePoolDefine(virConnectPtr conn, virStoragePoolPtr ret = NULL; testDriverLock(privconn); - if (!(def = virStoragePoolDefParse(conn, xml, NULL))) + if (!(def = virStoragePoolDefParseString(conn, xml))) goto cleanup; def->capacity = defaultPoolCap; @@ -3082,7 +3073,7 @@ testStorageVolumeCreateXML(virStoragePoolPtr pool, goto cleanup; } - privvol = virStorageVolDefParse(pool->conn, privpool->def, xmldesc, NULL); + privvol = virStorageVolDefParseString(pool->conn, privpool->def, xmldesc); if (privvol == NULL) goto cleanup; @@ -3163,7 +3154,7 @@ testStorageVolumeCreateXMLFrom(virStoragePoolPtr pool, goto cleanup; } - privvol = virStorageVolDefParse(pool->conn, privpool->def, xmldesc, NULL); + privvol = virStorageVolDefParseString(pool->conn, privpool->def, xmldesc); if (privvol == NULL) goto cleanup; -- 1.6.3.2

On Fri, Jun 19, 2009 at 12:37:11PM -0400, Cole Robinson wrote:
The storage driver arranges its parsing routines in a way that make them difficult to use in the test driver for non-default file parsing. This refactoring moves things to be consistent with the way domain_conf and network_conf do things.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 8 ++- src/storage_conf.c | 163 ++++++++++++++++++++++++++++++++-------------- src/storage_conf.h | 26 ++++++-- src/storage_driver.c | 8 +- src/test.c | 25 ++----- 5 files changed, 150 insertions(+), 80 deletions(-)
ACK, this was on my todo list too. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On 06/19/2009 01:09 PM, Daniel P. Berrange wrote:
On Fri, Jun 19, 2009 at 12:37:11PM -0400, Cole Robinson wrote:
The storage driver arranges its parsing routines in a way that make them difficult to use in the test driver for non-default file parsing. This refactoring moves things to be consistent with the way domain_conf and network_conf do things.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 8 ++- src/storage_conf.c | 163 ++++++++++++++++++++++++++++++++-------------- src/storage_conf.h | 26 ++++++-- src/storage_driver.c | 8 +- src/test.c | 25 ++----- 5 files changed, 150 insertions(+), 80 deletions(-)
ACK, this was on my todo list too.
Daniel
Hmm, this isn't quite complete. The above doesn't work for inlined pool/volume definitions for a custom test driver (reading a separate file, like the example in the docs section, works fine). Reason being that the xpaths in the parsing routines are all absolute, and not relative to the root node. The following additive patch solves that issue (this is what domain and network parsers do as well). Thanks, Cole

Cole Robinson wrote:
On 06/19/2009 01:09 PM, Daniel P. Berrange wrote:
On Fri, Jun 19, 2009 at 12:37:11PM -0400, Cole Robinson wrote:
The storage driver arranges its parsing routines in a way that make them difficult to use in the test driver for non-default file parsing. This refactoring moves things to be consistent with the way domain_conf and network_conf do things.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 8 ++- src/storage_conf.c | 163 ++++++++++++++++++++++++++++++++-------------- src/storage_conf.h | 26 ++++++-- src/storage_driver.c | 8 +- src/test.c | 25 ++----- 5 files changed, 150 insertions(+), 80 deletions(-) ACK, this was on my todo list too.
Daniel
Hmm, this isn't quite complete. The above doesn't work for inlined pool/volume definitions for a custom test driver (reading a separate file, like the example in the docs section, works fine). Reason being that the xpaths in the parsing routines are all absolute, and not relative to the root node. The following additive patch solves that issue (this is what domain and network parsers do as well).
Thanks, Cole
I've pushed this with the extra change, along with the rest of my pending patches. Thanks, Cole
participants (2)
-
Cole Robinson
-
Daniel P. Berrange