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 -=|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list