[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH: 9/16: main internal storage driver



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  -=| 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]