9:59 a.m.
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 -=|