7:05 a.m.
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
7:05 a.m.
New subject: [libvirt] [PATCH 01/12] storage_conf: Fix the wrong error message
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
6:45 a.m.
New subject: [libvirt] [PATCH 01/12] storage_conf: Fix the wrong error message
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
7:05 a.m.
New subject: [libvirt] [PATCH 02/12] storage_conf: Don't leak "uuid" in virStoragePoolDefParseAuthCephx
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
6:46 a.m.
New subject: [libvirt] [PATCH 02/12] storage_conf: Don't leak "uuid" in virStoragePoolDefParseAuthCephx
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
7:05 a.m.
New subject: [libvirt] [PATCH 03/12] storage_conf: Remove the useless casting
---
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
7:04 a.m.
New subject: [libvirt] [PATCH 03/12] storage_conf: Remove the useless casting
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
7:05 a.m.
New subject: [libvirt] [PATCH 04/12] storage_conf: Use xmlStrEqual instead of STREQ
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
7:05 a.m.
New subject: [libvirt] [PATCH 05/12] storage_conf: Put "%s" at the same line with error type
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
7:05 a.m.
New subject: [libvirt] [PATCH 05/12] storage_conf: Put "%s" at the same line with error type
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
7:05 a.m.
New subject: [libvirt] [PATCH 06/12] storage_conf: Fix the error type
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
9:35 a.m.
New subject: [libvirt] [PATCH 06/12] storage_conf: Fix the error type
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.
7:05 a.m.
New subject: [libvirt] [PATCH 07/12] storage_conf: Improve the memory deallocation of pool def parsing
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
6:13 a.m.
New subject: [libvirt] [PATCH 07/12] storage_conf: Improve the memory deallocation of pool def parsing
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
7:05 a.m.
New subject: [libvirt] [PATCH 08/12] storage_conf: Improve the memory deallocation of virStorageVolDefParseXML
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
6:13 a.m.
New subject: [libvirt] [PATCH 08/12] storage_conf: Improve the memory deallocation of virStorageVolDefParseXML
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
7:05 a.m.
New subject: [libvirt] [PATCH 09/12] storage_conf: Use NULLSTR instead
---
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
7:23 a.m.
New subject: [libvirt] [PATCH 09/12] storage_conf: Use NULLSTR instead
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
7:05 a.m.
New subject: [libvirt] [PATCH 10/12] storage_conf: Use VIR_STRDUP instead of strdup
---
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
9:09 a.m.
New subject: [libvirt] [PATCH 10/12] storage_conf: Use VIR_STRDUP instead of strdup
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
7:05 a.m.
New subject: [libvirt] [PATCH 11/12] storage_conf: Improve error messages
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
6:13 a.m.
New subject: [libvirt] [PATCH 11/12] storage_conf: Improve error messages
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
7:05 a.m.
New subject: [libvirt] [PATCH 12/12] storage_conf: Use uid_t/gid_t instead of int to cast the value
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
7:44 a.m.
New subject: [libvirt] [PATCH 12/12] storage_conf: Use uid_t/gid_t instead of int to cast the value
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)
9:11 a.m.
New subject: [libvirt] [PATCH 12/12] storage_conf: Use uid_t/gid_t instead of int to cast the value
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
5:29 a.m.
New subject: [libvirt] [PATCH 12/12] storage_conf: Use uid_t/gid_t instead of int to cast the value
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
4:09 a.m.
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(-)
4198
days inactive
4205
days old
28 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Osier Yang