On Tue, Feb 12, 2008 at 04:36:04AM +0000, Daniel P. Berrange wrote:
> This patch provides the general purpose code for the storage driver.
> The storage_driver.{c,h} file implements the libvirt internal storage
> driver API. The storage_conf.{c,h} file takes care of parsing and
> formatting the XML for pools and volumes. This should be reusable by
> the test driver's impl of the storage APIs in the future. The final
> storage_backend.{c,h} file provides a 2nd level internal driver API.
> This allows the main storage driver to call into storage pool specific
> impls for iSCSI, disk, filesystem, LVM and more. The storage_backend.h
> definitions allow the specific drivers to declare particular parts of
> the generic pool XML which are mandatory. For example, a disk pool
> always requires a source device element.
[...]
> +/* Flags to indicate mandatory components in the pool source */
> +enum {
> + VIR_STORAGE_BACKEND_POOL_SOURCE_HOST = (1<<0),
> + VIR_STORAGE_BACKEND_POOL_SOURCE_DEVICE = (1<<1),
wondering about 1<<2 , is that the storage option your removed compared
to previous patches ?
> + VIR_STORAGE_BACKEND_POOL_SOURCE_DIR = (1<<3),
> + VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER = (1<<4),
> +};
[...]
> +static virStoragePoolDefPtr virStoragePoolDefParseDoc(virConnectPtr conn,
> + xmlXPathContextPtr ctxt, xmlNodePtr root) {
[...]
> + if (options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_DEVICE) {
> + xmlNodePtr *nodeset = NULL;
> + int nsource, i;
> +
> + if ((nsource = virXPathNodeSet("/pool/source/device", ctxt, &nodeset)) <= 0) {
> + virStorageReportError(conn, VIR_ERR_XML_ERROR, "cannot extract source devices");
> + goto cleanup;
> + }
> + if ((ret->source.devices = calloc(nsource, sizeof(*ret->source.devices))) == NULL) {
> + free(nodeset);
> + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "device");
> + goto cleanup;
> + }
> + for (i = 0 ; i < nsource ; i++) {
> + xmlChar *path = xmlGetProp(nodeset[i], BAD_CAST "path");
> + if (path == NULL) {
> + virStorageReportError(conn, VIR_ERR_XML_ERROR, "missing source device path");
hum nodeset is leaked here it seems.
> + goto cleanup;
> + }
> + ret->source.devices[i].path = (char *)path;
> + }
> + free(nodeset);
> + ret->source.ndevice = nsource;
> + }
[...]
> + authType = virXPathString("string(/pool/source/auth/@type)", ctxt);
> + if (authType == NULL) {
> + ret->source.authType = VIR_STORAGE_POOL_AUTH_NONE;
> + } else {
> + if (STREQ(authType, "chap")) {
> + ret->source.authType = VIR_STORAGE_POOL_AUTH_CHAP;
> + } else {
> + virStorageReportError(conn, VIR_ERR_XML_ERROR, "unknown auth type '%s'", (const char *)authType);
> + goto cleanup;
hum, the cleanup of authType is more complex than necessary, i would just
free(authType); here before goto cleanup;
> + }
> + free(authType);
> + authType = NULL;
remove this affectation
> + }
> +
[...]
> + return ret;
> +
> + cleanup:
> + free(uuid);
> + if (type)
> + xmlFree(type);
> + if (authType)
> + xmlFree(authType);
and remove this, basically keeping authType processing to just the block
where it is used.
> + virStoragePoolDefFree(ret);
> + return NULL;
> +}
> +
> +virStoragePoolDefPtr virStoragePoolDefParse(virConnectPtr conn,
> + const char *xmlStr, const char *filename) {
> + virStoragePoolDefPtr ret = NULL;
> + xmlDocPtr xml = NULL;
> + xmlNodePtr node = NULL;
> + xmlXPathContextPtr ctxt = NULL;
> +
[...]
> + xmlFreeDoc(xml);
> +
> + return ret;
> +
> + cleanup:
> + xmlXPathFreeContext(ctxt);
> + if (xml)
> + xmlFreeDoc(xml);
> + return NULL;
> +}
since we try to remove if (x) free(x) style, just call xmlFreeDoc(xml);
since xmlFreeDoc handles NULLs fine.
[...]
> + cleanup:
> + if (buf) virBufferFree(buf);
same here
> + return NULL;
[...]
> + ret->target.path = virXPathString("string(/volume/target/path)", ctxt);
> + if (options->formatFromString) {
> + char *format = virXPathString("string(/volume/target/format/@type)", ctxt);
> + if ((ret->target.format = (options->formatFromString)(conn, format)) < 0) {
So you have in a structure an optional function to do the formatting,
feels a bit strange, but okay
[...]
> + if (xml)
> + xmlFreeDoc(xml);
one more, they can probably be all cleaned up after the commit anyway
[...]
> +static virStoragePoolObjPtr
> +virStoragePoolObjLoad(virStorageDriverStatePtr driver,
> + const char *file,
> + const char *path,
> + const char *xml,
> + const char *autostartLink) {
> + virStoragePoolDefPtr def;
> + virStoragePoolObjPtr pool;
> +
> + if (!(def = virStoragePoolDefParse(NULL, xml, file))) {
> + virErrorPtr err = virGetLastError();
> + virStorageLog("Error parsing storage pool config '%s' : %s",
> + path, err->message);
> + return NULL;
> + }
> +
> + if (!virFileMatchesNameSuffix(file, def->name, ".xml")) {
> + virStorageLog("Storage Pool config filename '%s' does not match pool name '%s'",
> + path, def->name);
> + virStoragePoolDefFree(def);
> + return NULL;
> + }
> +
> + if (!(pool = virStoragePoolObjAssignDef(NULL, driver, def))) {
> + virStorageLog("Failed to load storage pool config '%s': out of memory", path);
> + virStoragePoolDefFree(def);
> + return NULL;
> + }
Shouldn't autostartLink and path be checked against NULL
> + pool->configFile = strdup(path);
> + if (pool->configFile == NULL) {
> + virStorageLog("Failed to load storage pool config '%s': out of memory", path);
> + virStoragePoolDefFree(def);
> + return NULL;
> + }
> + pool->autostartLink = strdup(autostartLink);
> + if (pool->autostartLink == NULL) {
> + virStorageLog("Failed to load storage pool config '%s': out of memory", path);
> + virStoragePoolDefFree(def);
> + return NULL;
> + }
Since that seems assumed in that function, okay it is not public, so
if internally we know the value is never NULL, fine, but otherwise we would
get a confusing error message.
[...]
> diff -r 77cf7f42edd4 src/storage_driver.c
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/src/storage_driver.c Thu Feb 07 12:59:40 2008 -0500
[...]
> +#define storageLog(msg...) fprintf(stderr, msg)
any way to integrate into normal error reporting ? But that should not
block from commiting to CVs.
[...]
> +static int storageListPools(virConnectPtr conn, char **const names, int nnames) {
> + virStorageDriverStatePtr driver = (virStorageDriverStatePtr)conn->storagePrivateData;
> + virStoragePoolObjPtr pool = driver->pools;
> + int got = 0, i;
> + while (pool && got < nnames) {
> + if (virStoragePoolObjIsActive(pool)) {
> + if (!(names[got] = strdup(pool->def->name))) {
> + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "names");
> + goto cleanup;
> + }
> + got++;
> + }
> + pool = pool->next;
> + }
> + return got;
> +
> + cleanup:
> + for (i = 0 ; i < got ; i++) {
> + free(names[i]);
> + names[i] = NULL;
> + }
Hum, that looks right, but when passed a array like that it may be a
good idea to memset(0) it it can help triggering errors early or the task
of debugging.
[...]
> +
> +static int storageListDefinedPools(virConnectPtr conn, char **const names, int nnames) {
> + virStorageDriverStatePtr driver = (virStorageDriverStatePtr)conn->storagePrivateData;
> + virStoragePoolObjPtr pool = driver->pools;
> + int got = 0, i;
> + while (pool && got < nnames) {
> + if (!virStoragePoolObjIsActive(pool)) {
> + if (!(names[got] = strdup(pool->def->name))) {
> + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "names");
> + goto cleanup;
> + }
> + got++;
> + }
> + pool = pool->next;
> + }
> + return got;
> +
> + cleanup:
> + for (i = 0 ; i < got ; i++) {
> + free(names[i]);
> + names[i] = NULL;
> + }
> + return -1;
> +}
Similar here.
[...]
> +static int storagePoolDelete(virStoragePoolPtr obj,
> + unsigned int flags) {
> + virStorageDriverStatePtr driver = (virStorageDriverStatePtr)obj->conn->storagePrivateData;
> + virStoragePoolObjPtr pool = virStoragePoolObjFindByUUID(driver, obj->uuid);
> + virStorageBackendPtr backend;
> +
> + if (!pool) {
> + virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
> + "no storage pool with matching uuid");
> + return -1;
> + }
> +
> + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
> + return -1;
> + }
> +
> + if (virStoragePoolObjIsActive(pool)) {
> + virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
> + "storage pool is still active");
> + return -1;
> + }
> +
> + if (backend->deletePool &&
> + backend->deletePool(obj->conn, pool, flags) < 0)
> + return -1;
> +
> + return 0;
> +}
So you return 0 is the backend has no deletePool defined, looks a bit strange
> +
> +static int storagePoolRefresh(virStoragePoolPtr obj,
> + unsigned int flags ATTRIBUTE_UNUSED) {
we assume backend->refreshPool is always there, maybe a test is in
order.
> + if ((ret = backend->refreshPool(obj->conn, pool)) < 0) {
> + if (backend->stopPool)
> + backend->stopPool(obj->conn, pool);
okay, huge, but looks fine.
One thing I realize is that it adds a lot of new strings for localization
so it's better to push early if we want to get decent translations.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list