[libvirt] [PATCH 3/5] storage_conf: Various fixes or improvements on pool def parsing

virStorageDefParsePerms: * Use uid_t/gid_t to do casting virStoragePoolDefParseSource: * Improve the error message for source "name" parsing * Remove the useless casting (const char *) virStoragePoolDefParseAuthChap: * Fix the wrong error message virStoragePoolDefParseAuthCephx: * Don't leak "uuid" virStoragePoolDefParseXML: * Remove the useless casting "(const char *)" * Use VIR_ERR_XML_ERROR for pool 'type' parsing * Remove the xmlFree() * s/tmppath/path/, * Create "virStoragePoolDefPtr def", and use ret to track the return value; frees the strings at "cleanup" label instead if freeing them in the middle. * Other small changes. --- src/conf/storage_conf.c | 169 ++++++++++++++++++++++++------------------------ 1 file changed, 85 insertions(+), 84 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8fa805b..dd55d2c 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 host 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; } @@ -466,10 +466,12 @@ 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, - "%s", _("missing auth username attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth username attribute")); return -1; } @@ -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; + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid auth secret uuid")); + goto cleanup; } auth->secret.uuidUsable = true; } else { auth->secret.uuidUsable = false; } - return 0; + ret = 0; +cleanup: + VIR_FREE(uuid); + return ret; } static int @@ -526,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; } @@ -559,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; @@ -595,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; @@ -667,7 +672,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, } else { virReportError(VIR_ERR_XML_ERROR, _("unknown auth type '%s'"), - (const char *)authType); + authType); goto cleanup; } } @@ -768,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; @@ -780,22 +785,22 @@ 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; + perms->uid = (uid_t)v; } if (virXPathNode("./group", ctxt) == NULL) { 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; + perms->gid = (gid_t)v; } /* NB, we're ignoring missing labels here - they'll simply inherit */ @@ -811,85 +816,81 @@ static virStoragePoolDefPtr virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { virStoragePoolOptionsPtr options; - virStoragePoolDefPtr ret; + virStoragePoolDefPtr ret = NULL; + virStoragePoolDefPtr def; xmlNodePtr source_node; char *type = NULL; char *uuid = NULL; - char *tmppath; + char *target_path = NULL; - if (VIR_ALLOC(ret) < 0) { + if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; } type = virXPathString("string(./@type)", ctxt); - if ((ret->type = virStoragePoolTypeFromString((const char *)type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage pool type %s"), (const char*)type); + if ((def->type = virStoragePoolTypeFromString(type)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown storage pool type %s"), type); goto cleanup; } - xmlFree(type); - type = NULL; - - if ((options = virStoragePoolOptionsForPoolType(ret->type)) == NULL) { + if ((options = virStoragePoolOptionsForPoolType(def->type)) == NULL) goto cleanup; - } source_node = virXPathNode("./source", ctxt); if (source_node) { - if (virStoragePoolDefParseSource(ctxt, &ret->source, ret->type, + if (virStoragePoolDefParseSource(ctxt, &def->source, def->type, source_node) < 0) goto cleanup; } - ret->name = virXPathString("string(./name)", ctxt); - if (ret->name == NULL && + def->name = virXPathString("string(./name)", ctxt); + if (def->name == NULL && 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")); + def->name = def->source.name; + if (def->name == NULL) { + 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")); + if (virUUIDGenerate(def->uuid) < 0) { + 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")); + if (virUUIDParse(uuid, def->uuid) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed uuid element")); goto cleanup; } - VIR_FREE(uuid); } if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { - if (!ret->source.nhost) { - virReportError(VIR_ERR_XML_ERROR, - "%s", + if (!def->source.nhost) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source host name")); goto cleanup; } } if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) { - if (!ret->source.dir) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool source path")); + if (!def->source.dir) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source path")); goto cleanup; } } + if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) { - if (ret->source.name == NULL) { + if (def->source.name == NULL) { /* source name defaults to pool name */ - ret->source.name = strdup(ret->name); - if (ret->source.name == NULL) { + def->source.name = strdup(def->name); + if (def->source.name == NULL) { virReportOOMError(); goto cleanup; } @@ -897,30 +898,30 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } if (options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) { - if (!ret->source.adapter.type) { + if (!def->source.adapter.type) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source adapter")); goto cleanup; } - if (ret->source.adapter.type == + if (def->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - if (!ret->source.adapter.data.fchost.wwnn || - !ret->source.adapter.data.fchost.wwpn) { + if (!def->source.adapter.data.fchost.wwnn || + !def->source.adapter.data.fchost.wwpn) { virReportError(VIR_ERR_XML_ERROR, "%s", _("'wwnn' and 'wwpn' must be specified for adapter " "type 'fchost'")); goto cleanup; } - if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) || - !virValidateWWN(ret->source.adapter.data.fchost.wwpn)) + if (!virValidateWWN(def->source.adapter.data.fchost.wwnn) || + !virValidateWWN(def->source.adapter.data.fchost.wwpn)) goto cleanup; - } else if (ret->source.adapter.type == + } else if (def->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")); + if (!def->source.adapter.data.name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source adapter name")); goto cleanup; } } @@ -928,9 +929,9 @@ 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")); + if (!def->source.ndevice) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source device name")); goto cleanup; } } @@ -938,29 +939,29 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) /* 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) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing storage pool target path")); + if (!(target_path = virXPathString("string(./target/path)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool target path")); goto cleanup; } - ret->target.path = virFileSanitizePath(tmppath); - VIR_FREE(tmppath); - if (!ret->target.path) + def->target.path = virFileSanitizePath(target_path); + if (!def->target.path) goto cleanup; - if (virStorageDefParsePerms(ctxt, &ret->target.perms, + if (virStorageDefParsePerms(ctxt, &def->target.perms, "./target/permissions", DEFAULT_POOL_PERM_MODE) < 0) goto cleanup; } - return ret; - + ret = def; cleanup: VIR_FREE(uuid); - xmlFree(type); - virStoragePoolDefFree(ret); - return NULL; + VIR_FREE(type); + VIR_FREE(target_path); + if (!ret) + virStoragePoolDefFree(ret); + return ret; } virStoragePoolDefPtr -- 1.8.1.4