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

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



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 redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/


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