On Mon, Feb 18, 2008 at 09:51:28AM -0500, Daniel Veillard wrote:
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 ?
Yes a (harmless) bug -- I'll updated the numbers to remove the gap
+ VIR_STORAGE_BACKEND_POOL_SOURCE_DIR = (1<<3), + VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER = (1<<4), +};
+ 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.
Yes will fix that.
+ 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.
Ok, reasonable enough.
+ 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.
Yes, I did 'make syntax-check' but seems to have missed those
+ 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
Well the format field is optional - not all pools support volumes with special formats. Fs/dir driver supports raw, qcow, vmdk, etc. The iSCSI driver though doesn't support any - you just get a block device, no choice, so the formatFromString and formatToString functions are not needed.
+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
The caller will never pass in a 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.
Yep, we know the caller won't pass in a NULL in this internal method.
[...]
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.
This isn't really error reporting - its just for a few harmless debug messages. We don't have any API for propagating log messages back to the application, though I've proposed it once before...
+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.
Yep, good idea.
+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
Hmm, yes, should return -1 if deletePool is not supported by the driver
+ +static int storagePoolRefresh(virStoragePoolPtr obj, + unsigned int flags ATTRIBUTE_UNUSED) {
we assume backend->refreshPool is always there, maybe a test is in order.
refreshPool & refreshVol are the two compulsory drivers methods for the backend, all the rest are optional. There's not much point in checking because anyone adding a new driver will immediately find that they're required because nothing will work without them Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|