[libvirt] [PATCH 00/12 v2] storage_conf: left cleanup/fixes/improvement patches
Part of v1 are pushed, the left patches are splitted Osier Yang (12): storage_conf: Fix the wrong error message storage_conf: Don't leak "uuid" in virStoragePoolDefParseAuthCephx storage_conf: Remove the useless casting storage_conf: Use xmlStrEqual instead of STREQ storage_conf: Put "%s" at the same line with error type storage_conf: Fix the error type storage_conf: Improve the memory deallocation of pool def parsing storage_conf: Improve the memory deallocation of virStorageVolDefParseXML storage_conf: Use NULLSTR instead storage_conf: Use VIR_STRDUP instead of strdup storage_conf: Improve error messages storage_conf: Use uid_t/gid_t instead of int to cast the value src/conf/storage_conf.c | 242 ++++++++++++++++++++++++------------------------ 1 file changed, 122 insertions(+), 120 deletions(-) -- 1.8.1.4
It's for parsing "login" attribute of "auth". --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6cb98bf..bd8eef0 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -447,7 +447,7 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, auth->login = virXPathString("string(./auth/@login)", ctxt); if (auth->login == NULL) { virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth host attribute")); + "%s", _("missing auth login attribute")); return -1; } -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
It's for parsing "login" attribute of "auth". --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6cb98bf..bd8eef0 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -447,7 +447,7 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, auth->login = virXPathString("string(./auth/@login)", ctxt); if (auth->login == NULL) { virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth host attribute")); + "%s", _("missing auth login attribute")); return -1; }
ACK Introduced by 2087872, but it doesn't seem to be used anywhere. Jan
Any string returned from virXPathString should be freed. --- src/conf/storage_conf.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index bd8eef0..073099b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -466,6 +466,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, virStoragePoolAuthCephxPtr auth) { char *uuid = NULL; + int ret = -1; + auth->username = virXPathString("string(./auth/@username)", ctxt); if (auth->username == NULL) { virReportError(VIR_ERR_XML_ERROR, @@ -485,19 +487,22 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, if (auth->secret.usage != NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("either auth secret uuid or usage expected")); - return -1; + goto cleanup; } if (virUUIDParse(uuid, auth->secret.uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid auth secret uuid")); - return -1; + goto cleanup; } auth->secret.uuidUsable = true; } else { auth->secret.uuidUsable = false; } - return 0; + ret = 0; +cleanup: + VIR_FREE(uuid); + return ret; } static int -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
Any string returned from virXPathString should be freed. --- src/conf/storage_conf.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
ACK Jan
--- src/conf/storage_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 073099b..6f89f1c 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -672,7 +672,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, } else { virReportError(VIR_ERR_XML_ERROR, _("unknown auth type '%s'"), - (const char *)authType); + authType); goto cleanup; } } @@ -828,9 +828,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } type = virXPathString("string(./@type)", ctxt); - if ((ret->type = virStoragePoolTypeFromString((const char *)type)) < 0) { + if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage pool type %s"), (const char*)type); + _("unknown storage pool type %s"), type); goto cleanup; } -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
--- src/conf/storage_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 073099b..6f89f1c 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c
ACK Jan
And improve the error message --- src/conf/storage_conf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6f89f1c..5c8577e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -975,9 +975,11 @@ virStoragePoolDefParseNode(xmlDocPtr xml, xmlXPathContextPtr ctxt = NULL; virStoragePoolDefPtr def = NULL; - if (STRNEQ((const char *)root->name, "pool")) { + if (!xmlStrEqual(root->name, BAD_CAST "pool")) { virReportError(VIR_ERR_XML_ERROR, - "%s", _("unknown root element for storage pool")); + _("unexpected root element <%s>, " + "expecting <pool>"), + root->name); goto cleanup; } @@ -1353,9 +1355,11 @@ virStorageVolDefParseNode(virStoragePoolDefPtr pool, xmlXPathContextPtr ctxt = NULL; virStorageVolDefPtr def = NULL; - if (STRNEQ((const char *)root->name, "volume")) { + if (!xmlStrEqual(root->name, BAD_CAST "volume")) { virReportError(VIR_ERR_XML_ERROR, - "%s", _("unknown root element for storage vol")); + _("unexpected root element <%s>, " + "expecting <volume>"), + root->name); goto cleanup; } -- 1.8.1.4
Trivial, but it allows the "error message" to have more spaces. --- src/conf/storage_conf.c | 95 ++++++++++++++++++++++++------------------------- 1 file changed, 47 insertions(+), 48 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5c8577e..76dae52 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -446,15 +446,15 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, { auth->login = virXPathString("string(./auth/@login)", ctxt); if (auth->login == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth login attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth login attribute")); return -1; } auth->passwd = virXPathString("string(./auth/@passwd)", ctxt); if (auth->passwd == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth passwd attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth passwd attribute")); return -1; } @@ -470,8 +470,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, auth->username = virXPathString("string(./auth/@username)", ctxt); if (auth->username == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth username attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth username attribute")); return -1; } @@ -490,8 +490,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, goto cleanup; } if (virUUIDParse(uuid, auth->secret.uuid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("invalid auth secret uuid")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid auth secret uuid")); goto cleanup; } auth->secret.uuidUsable = true; @@ -564,8 +564,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, for (i = 0; i < source->nhost; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool host name")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool host name")); goto cleanup; } source->hosts[i].name = name; @@ -600,8 +600,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, char *path = virXMLPropString(nodeset[i], "path"); if (path == NULL) { VIR_FREE(nodeset); - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source device path")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source device path")); goto cleanup; } source->devices[i].path = path; @@ -773,8 +773,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { VIR_FREE(mode); - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed octal mode")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed octal mode")); goto error; } perms->mode = tmp; @@ -785,8 +785,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, perms->uid = (uid_t) -1; } else { if (virXPathLong("number(./owner)", ctxt, &v) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed owner element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed owner element")); goto error; } perms->uid = (int)v; @@ -796,8 +796,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, perms->gid = (gid_t) -1; } else { if (virXPathLong("number(./group)", ctxt, &v) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed group element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed group element")); goto error; } perms->gid = (int)v; @@ -853,22 +853,22 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) options->flags & VIR_STORAGE_POOL_SOURCE_NAME) ret->name = ret->source.name; if (ret->name == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing pool source name element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing pool source name element")); goto cleanup; } uuid = virXPathString("string(./uuid)", ctxt); if (uuid == NULL) { if (virUUIDGenerate(ret->uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unable to generate uuid")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to generate uuid")); goto cleanup; } } else { if (virUUIDParse(uuid, ret->uuid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed uuid element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed uuid element")); goto cleanup; } VIR_FREE(uuid); @@ -876,8 +876,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { if (!ret->source.nhost) { - virReportError(VIR_ERR_XML_ERROR, - "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source host name")); goto cleanup; } @@ -885,8 +884,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) { if (!ret->source.dir) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source path")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source path")); goto cleanup; } } @@ -924,8 +923,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } else if (ret->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { if (!ret->source.adapter.data.name) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source adapter name")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source adapter name")); goto cleanup; } } @@ -934,8 +933,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) /* If DEVICE is the only source type, then its required */ if (options->flags == VIR_STORAGE_POOL_SOURCE_DEVICE) { if (!ret->source.ndevice) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source device name")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source device name")); goto cleanup; } } @@ -944,8 +943,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) * path and permissions */ if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) { if ((tmppath = virXPathString("string(./target/path)", ctxt)) == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool target path")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool target path")); goto cleanup; } ret->target.path = virFileSanitizePath(tmppath); @@ -1155,8 +1154,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) type = virStoragePoolTypeToString(def->type); if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unexpected pool type")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected pool type")); goto cleanup; } virBufferAsprintf(&buf, "<pool type='%s'>\n", type); @@ -1220,8 +1219,8 @@ virStorageSize(const char *unit, unsigned long long *ret) { if (virStrToLong_ull(val, NULL, 10, ret) < 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("malformed capacity element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed capacity element")); return -1; } /* off_t is signed, so you cannot create a file larger than 2**63 @@ -1254,8 +1253,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, ret->name = virXPathString("string(./name)", ctxt); if (ret->name == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing volume name element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing volume name element")); goto cleanup; } @@ -1265,8 +1264,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); if (capacity == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing capacity element")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing capacity element")); goto cleanup; } if (virStorageSize(unit, capacity, &ret->capacity) < 0) @@ -1666,8 +1665,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, } if (virMutexInit(&pool->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize mutex")); VIR_FREE(pool); return NULL; } @@ -1815,8 +1814,8 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, } if (!(xml = virStoragePoolDefFormat(def))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to generate XML")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate XML")); return -1; } @@ -1878,8 +1877,8 @@ virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def) type = virStoragePoolTypeToString(def->type); if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unexpected pool type")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected pool type")); goto cleanup; } -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
Trivial, but it allows the "error message" to have more spaces. --- src/conf/storage_conf.c | 95 ++++++++++++++++++++++++------------------------- 1 file changed, 47 insertions(+), 48 deletions(-)
ACK Jan
s/VIR_ERR_INTERNAL_ERROR/VIR_ERR_XML_ERROR/. --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 76dae52..efe02e8 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -829,7 +829,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) type = virXPathString("string(./@type)", ctxt); if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_XML_ERROR, _("unknown storage pool type %s"), type); goto cleanup; } -- 1.8.1.4
On 24/05/13 20:05, Ján Tomko wrote:
On 05/22/2013 02:05 PM, Osier Yang wrote:
s/VIR_ERR_INTERNAL_ERROR/VIR_ERR_XML_ERROR/. --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
Thanks, 1/12 ~ 6/12 are pushed.
Changes: * Free all the strings at "cleanup", instead of freeing them in the middle * Remove xmlFree * s/tmppath/target_path/, to make it more sensible * Add new goto label "error" --- src/conf/storage_conf.c | 54 ++++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index efe02e8..6f0ed74 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -820,7 +820,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) xmlNodePtr source_node; char *type = NULL; char *uuid = NULL; - char *tmppath; + char *target_path = NULL; if (VIR_ALLOC(ret) < 0) { virReportOOMError(); @@ -831,21 +831,18 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown storage pool type %s"), type); - goto cleanup; + goto error; } - xmlFree(type); - type = NULL; - if ((options = virStoragePoolOptionsForPoolType(ret->type)) == NULL) { - goto cleanup; + goto error; } source_node = virXPathNode("./source", ctxt); if (source_node) { if (virStoragePoolDefParseSource(ctxt, &ret->source, ret->type, source_node) < 0) - goto cleanup; + goto error; } ret->name = virXPathString("string(./name)", ctxt); @@ -855,7 +852,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (ret->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing pool source name element")); - goto cleanup; + goto error; } uuid = virXPathString("string(./uuid)", ctxt); @@ -863,22 +860,21 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (virUUIDGenerate(ret->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to generate uuid")); - goto cleanup; + goto error; } } else { if (virUUIDParse(uuid, ret->uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed uuid element")); - goto cleanup; + goto error; } - VIR_FREE(uuid); } if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { if (!ret->source.nhost) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source host name")); - goto cleanup; + goto error; } } @@ -886,7 +882,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (!ret->source.dir) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source path")); - goto cleanup; + goto error; } } if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) { @@ -895,7 +891,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) ret->source.name = strdup(ret->name); if (ret->source.name == NULL) { virReportOOMError(); - goto cleanup; + goto error; } } } @@ -904,7 +900,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (!ret->source.adapter.type) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source adapter")); - goto cleanup; + goto error; } if (ret->source.adapter.type == @@ -914,7 +910,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) virReportError(VIR_ERR_XML_ERROR, "%s", _("'wwnn' and 'wwpn' must be specified for adapter " "type 'fchost'")); - goto cleanup; + goto error; } if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) || @@ -925,7 +921,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (!ret->source.adapter.data.name) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source adapter name")); - goto cleanup; + goto error; } } } @@ -935,36 +931,38 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (!ret->source.ndevice) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source device name")); - goto cleanup; + goto error; } } /* When we are working with a virtual disk we can skip the target * path and permissions */ if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) { - if ((tmppath = virXPathString("string(./target/path)", ctxt)) == NULL) { + if (!(target_path = virXPathString("string(./target/path)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool target path")); - goto cleanup; + goto error; } - ret->target.path = virFileSanitizePath(tmppath); - VIR_FREE(tmppath); + ret->target.path = virFileSanitizePath(target_path); if (!ret->target.path) - goto cleanup; + goto error; if (virStorageDefParsePerms(ctxt, &ret->target.perms, "./target/permissions", DEFAULT_POOL_PERM_MODE) < 0) - goto cleanup; + goto error; } - return ret; - cleanup: VIR_FREE(uuid); - xmlFree(type); + VIR_FREE(type); + VIR_FREE(target_path); + return ret; + +error: virStoragePoolDefFree(ret); - return NULL; + ret = NULL; + goto cleanup; } virStoragePoolDefPtr -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
Changes: * Free all the strings at "cleanup", instead of freeing them in the middle * Remove xmlFree * s/tmppath/target_path/, to make it more sensible * Add new goto label "error" --- src/conf/storage_conf.c | 54 ++++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index efe02e8..6f0ed74 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c
@@ -914,7 +910,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) virReportError(VIR_ERR_XML_ERROR, "%s", _("'wwnn' and 'wwpn' must be specified for adapter " "type 'fchost'")); - goto cleanup; + goto error; }
if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) ||
If the WWN validation fails here, you still jump to cleanup instead of error. ACK if you fix that. Jan
Changes: * Add a new goto label "error" * Free the strings at "cleanup" * Remove the unnecessary frees --- src/conf/storage_conf.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6f0ed74..4c08cea 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1253,7 +1253,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (ret->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing volume name element")); - goto cleanup; + goto error; } /* Auto-generated so deliberately ignore */ @@ -1264,20 +1264,17 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (capacity == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); - goto cleanup; + goto error; } if (virStorageSize(unit, capacity, &ret->capacity) < 0) - goto cleanup; - VIR_FREE(capacity); + goto error; VIR_FREE(unit); allocation = virXPathString("string(./allocation)", ctxt); if (allocation) { unit = virXPathString("string(./allocation/@unit)", ctxt); if (virStorageSize(unit, allocation, &ret->allocation) < 0) - goto cleanup; - VIR_FREE(allocation); - VIR_FREE(unit); + goto error; } else { ret->allocation = ret->capacity; } @@ -1294,7 +1291,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, virReportError(VIR_ERR_XML_ERROR, _("unknown volume format type %s"), format); VIR_FREE(format); - goto cleanup; + goto error; } VIR_FREE(format); } @@ -1302,14 +1299,14 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (virStorageDefParsePerms(ctxt, &ret->target.perms, "./target/permissions", DEFAULT_VOL_PERM_MODE) < 0) - goto cleanup; + goto error; node = virXPathNode("./target/encryption", ctxt); if (node != NULL) { ret->target.encryption = virStorageEncryptionParseNode(ctxt->doc, node); if (ret->target.encryption == NULL) - goto cleanup; + goto error; } ret->backingStore.path = virXPathString("string(./backingStore/path)", ctxt); @@ -1324,7 +1321,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, virReportError(VIR_ERR_XML_ERROR, _("unknown volume format type %s"), format); VIR_FREE(format); - goto cleanup; + goto error; } VIR_FREE(format); } @@ -1332,16 +1329,18 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (virStorageDefParsePerms(ctxt, &ret->backingStore.perms, "./backingStore/permissions", DEFAULT_VOL_PERM_MODE) < 0) - goto cleanup; - - return ret; + goto error; cleanup: VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); + return ret; + +error: virStorageVolDefFree(ret); - return NULL; + ret = NULL; + goto cleanup; } virStorageVolDefPtr -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
Changes: * Add a new goto label "error" * Free the strings at "cleanup" * Remove the unnecessary frees --- src/conf/storage_conf.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
ACK. Jan
--- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4c08cea..1f376ef 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1485,7 +1485,7 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virBufferAddLit(&buf, "<volume>\n"); virBufferAsprintf(&buf," <name>%s</name>\n", def->name); - virBufferAsprintf(&buf," <key>%s</key>\n", def->key ? def->key : "(null)"); + virBufferAsprintf(&buf," <key>%s</key>\n", NULLSTR(def->key)); virBufferAddLit(&buf, " <source>\n"); if (def->source.nextent) { -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
--- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4c08cea..1f376ef 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1485,7 +1485,7 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool,
virBufferAddLit(&buf, "<volume>\n"); virBufferAsprintf(&buf," <name>%s</name>\n", def->name); - virBufferAsprintf(&buf," <key>%s</key>\n", def->key ? def->key : "(null)"); + virBufferAsprintf(&buf," <key>%s</key>\n", NULLSTR(def->key)); virBufferAddLit(&buf, " <source>\n");
if (def->source.nextent) {
ACK Jan
--- src/conf/storage_conf.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 1f376ef..44ecb2a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -888,11 +888,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) { if (ret->source.name == NULL) { /* source name defaults to pool name */ - ret->source.name = strdup(ret->name); - if (ret->source.name == NULL) { - virReportOOMError(); + if (VIR_STRDUP(ret->source.name, ret->name) < 0) goto error; - } } } @@ -1710,16 +1707,12 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, } VIR_FREE(pool->configFile); /* for driver reload */ - pool->configFile = strdup(path); - if (pool->configFile == NULL) { - virReportOOMError(); + if (VIR_STRDUP(pool->configFile, path) < 0) { virStoragePoolDefFree(def); return NULL; } VIR_FREE(pool->autostartLink); /* for driver reload */ - pool->autostartLink = strdup(autostartLink); - if (pool->autostartLink == NULL) { - virReportOOMError(); + if (VIR_STRDUP(pool->autostartLink, autostartLink) < 0) { virStoragePoolDefFree(def); return NULL; } -- 1.8.1.4
On 05/22/2013 06:05 AM, Osier Yang wrote:
--- src/conf/storage_conf.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
I think this was independently fixed by the STRDUP patch series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
virStoragePoolDefParseSource: * Better error message virStoragePoolObjLoad: * Break the line line --- src/conf/storage_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 44ecb2a..a62629e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -531,7 +531,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->name = virXPathString("string(./name)", ctxt); if (pool_type == VIR_STORAGE_POOL_RBD && source->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing mandatory 'name' field for RBD pool name")); + _("element 'name' is mandatory for RBD pool name")); goto cleanup; } @@ -1695,7 +1695,8 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, if (!virFileMatchesNameSuffix(file, def->name, ".xml")) { virReportError(VIR_ERR_XML_ERROR, - _("Storage pool config filename '%s' does not match pool name '%s'"), + _("Storage pool config filename '%s' does " + "not match pool name '%s'"), path, def->name); virStoragePoolDefFree(def); return NULL; -- 1.8.1.4
On 05/22/2013 02:05 PM, Osier Yang wrote:
virStoragePoolDefParseSource: * Better error message
virStoragePoolObjLoad: * Break the line line --- src/conf/storage_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 44ecb2a..a62629e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -531,7 +531,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->name = virXPathString("string(./name)", ctxt); if (pool_type == VIR_STORAGE_POOL_RBD && source->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing mandatory 'name' field for RBD pool name")); + _("element 'name' is mandatory for RBD pool name"));
s/pool name/pools/
goto cleanup; }
@@ -1695,7 +1695,8 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
if (!virFileMatchesNameSuffix(file, def->name, ".xml")) { virReportError(VIR_ERR_XML_ERROR, - _("Storage pool config filename '%s' does not match pool name '%s'"), + _("Storage pool config filename '%s' does " + "not match pool name '%s'"), path, def->name); virStoragePoolDefFree(def); return NULL;
ACK Jan
And error out if the casted value is not same with the original one, which prevents the bug on platform(s) where uid_t/gid_t has different size with long. --- src/conf/storage_conf.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a62629e..a648c6d 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -747,7 +747,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, int defaultmode) { char *mode; - long v; + long val; int ret = -1; xmlNodePtr relnode; xmlNodePtr node; @@ -784,23 +784,26 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (virXPathNode("./owner", ctxt) == NULL) { perms->uid = (uid_t) -1; } else { - if (virXPathLong("number(./owner)", ctxt, &v) < 0) { + if (virXPathLong("number(./owner)", ctxt, &val) < 0 || + (uid_t)val != val) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed owner element")); goto error; } - perms->uid = (int)v; + + perms->uid = (uid_t)val; } if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { - if (virXPathLong("number(./group)", ctxt, &v) < 0) { + if (virXPathLong("number(./group)", ctxt, &val) < 0 || + (gid_t)val != val) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed group element")); goto error; } - perms->gid = (int)v; + perms->gid = (gid_t)val; } /* NB, we're ignoring missing labels here - they'll simply inherit */ -- 1.8.1.4
On 22/05/13 20:05, Osier Yang wrote:
And error out if the casted value is not same with the original one, which prevents the bug on platform(s) where uid_t/gid_t has different size with long. --- src/conf/storage_conf.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a62629e..a648c6d 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -747,7 +747,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, int defaultmode) { char *mode; - long v; + long val; int ret = -1; xmlNodePtr relnode; xmlNodePtr node; @@ -784,23 +784,26 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (virXPathNode("./owner", ctxt) == NULL) { perms->uid = (uid_t) -1; } else { - if (virXPathLong("number(./owner)", ctxt, &v) < 0) { + if (virXPathLong("number(./owner)", ctxt, &val) < 0 || + (uid_t)val != val) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed owner element")); goto error; } - perms->uid = (int)v; + + perms->uid = (uid_t)val; }
if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { - if (virXPathLong("number(./group)", ctxt, &v) < 0) { + if (virXPathLong("number(./group)", ctxt, &val) < 0 || + (gid_t)val != val) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed group element")); goto error; } - perms->gid = (int)v; + perms->gid = (gid_t)val; }
/* NB, we're ignoring missing labels here - they'll simply inherit */
Rng schema for "owner", and "group": <element name='owner'> <choice> <ref name='unsignedInt'/> <value>-1</value> </choice> </element> <element name='group'> <choice> <ref name='unsignedInt'/> <value>-1</value> </choice> </element> Which means the patch has to filter out the "-1" when do "(uid_t)val != val" or "(gid_t)val != val". So with the attached diff squashed in (will update the commit log when pushing)
On 05/22/2013 06:05 AM, Osier Yang wrote:
And error out if the casted value is not same with the original one, which prevents the bug on platform(s) where uid_t/gid_t has different size with long. --- src/conf/storage_conf.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
} else { - if (virXPathLong("number(./owner)", ctxt, &v) < 0) { + if (virXPathLong("number(./owner)", ctxt, &val) < 0 || + (uid_t)val != val) {
Once you have made this check...
virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed owner element")); goto error; } - perms->uid = (int)v; + + perms->uid = (uid_t)val;
...the cast here is redundant. You could write 'perms->uid = val'.
}
if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { - if (virXPathLong("number(./group)", ctxt, &v) < 0) { + if (virXPathLong("number(./group)", ctxt, &val) < 0 || + (gid_t)val != val) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed group element")); goto error; } - perms->gid = (int)v; + perms->gid = (gid_t)val;
Likewise. ACK with that simplification, and with your followup that explicitly allows -1. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 24/05/13 22:11, Eric Blake wrote:
On 05/22/2013 06:05 AM, Osier Yang wrote:
And error out if the casted value is not same with the original one, which prevents the bug on platform(s) where uid_t/gid_t has different size with long. --- src/conf/storage_conf.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
} else { - if (virXPathLong("number(./owner)", ctxt, &v) < 0) { + if (virXPathLong("number(./owner)", ctxt, &val) < 0 || + (uid_t)val != val) { Once you have made this check...
virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed owner element")); goto error; } - perms->uid = (int)v; + + perms->uid = (uid_t)val;
...the cast here is redundant. You could write 'perms->uid = val'.
}
if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { - if (virXPathLong("number(./group)", ctxt, &v) < 0) { + if (virXPathLong("number(./group)", ctxt, &val) < 0 || + (gid_t)val != val) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed group element")); goto error; } - perms->gid = (int)v; + perms->gid = (gid_t)val;
Likewise.
ACK with that simplification, and with your followup that explicitly allows -1.
Thanks for the reviewing, I pushed the left patches (except 10/12) with the small nits pointed by you guys fixed. Osier
On 22/05/13 20:05, Osier Yang wrote: Ping. 10/12 is outdated after Michal's patches are pushed.
Part of v1 are pushed, the left patches are splitted
Osier Yang (12): storage_conf: Fix the wrong error message storage_conf: Don't leak "uuid" in virStoragePoolDefParseAuthCephx storage_conf: Remove the useless casting storage_conf: Use xmlStrEqual instead of STREQ storage_conf: Put "%s" at the same line with error type storage_conf: Fix the error type storage_conf: Improve the memory deallocation of pool def parsing storage_conf: Improve the memory deallocation of virStorageVolDefParseXML storage_conf: Use NULLSTR instead storage_conf: Use VIR_STRDUP instead of strdup storage_conf: Improve error messages storage_conf: Use uid_t/gid_t instead of int to cast the value
src/conf/storage_conf.c | 242 ++++++++++++++++++++++++------------------------ 1 file changed, 122 insertions(+), 120 deletions(-)
participants (3)
-
Eric Blake -
Ján Tomko -
Osier Yang