Re: [PATCH 5/7] conf: Introduce virStoragePoolLoadAllState && virStoragePoolLoadState

[John Ferlan <jferlan@xxxxxxxxxx>]

On 03/24/2015 06:06 AM, Erik Skultety wrote:
> These functions operate exactly the same as
> virStoragePoolLoadAllConfigs.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
> ---
>  src/conf/storage_conf.c      | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/storage_conf.h      |  7 ++++
>  src/libvirt_private.syms     |  1 +
>  src/storage/storage_driver.c | 11 ++++++
>  4 files changed, 109 insertions(+)
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 5d984f3..b158e30 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1863,6 +1863,96 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
>  }
>  
>  
> +virStoragePoolObjPtr
> +virStoragePoolLoadState(virStoragePoolObjListPtr pools,
> +                        const char *stateDir,
> +                        const char *name)
> +{
> +    char *stateFile = NULL;
> +    virStoragePoolDefPtr def = NULL;
> +    virStoragePoolObjPtr pool = NULL;
> +    xmlDocPtr xml = NULL;
> +    xmlXPathContextPtr ctxt = NULL;
> +    xmlNodePtr node = NULL;
> +
> +    if (!(stateFile = virFileBuildPath(stateDir, name, ".xml")))
> +        goto cleanup;
> +
> +    if (!(xml = virXMLParseCtxt(stateFile, NULL, _("(pool status)"), &ctxt)))

s/status/state  ?


> +        goto cleanup;
> +
> +    if (!(node = virXPathNode("//pool", ctxt))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Could not find any 'pool' element in status file"));

s/status/state   ?

> +        goto cleanup;
> +    }
> +
> +    ctxt->node = node;
> +    if (!(def = virStoragePoolDefParseXML(ctxt)))
> +        goto cleanup;
> +
> +    if (!STREQ(name, def->name)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Storage pool status file '%s' does not match "

s/status/state

> +                         "pool name '%s'"),
> +                       stateFile, def->name);

Coverity found - if we jump to cleanup here, def is leaked.

> +        goto cleanup;
> +    }
> +
> +    /* create the object */
> +    if (!(pool = virStoragePoolObjAssignDef(pools, def)))
> +        goto cleanup;

Add the def = NULL here since success ObjAssignDef consumes it

> +
> +    /* XXX: future handling of some additional usefull status data,
> +     * for now, if a status file for a pool exists, the pool will be marked
> +     * as active
> +     */
> +
> +    pool->active = 1;
> +
> + cleanup:
> +    VIR_FREE(stateFile);
> +    xmlFree(xml);
> +    xmlXPathFreeContext(ctxt);

Then add here:

    virStoragePoolDefFree(def);

> +    return pool;
> +}
> +
> +
> +int
> +virStoragePoolLoadAllState(virStoragePoolObjListPtr pools,
> +                           const char *stateDir)
> +{
> +    DIR *dir;
> +    struct dirent *entry;
> +    int ret = -1;
> +
> +    if (!(dir = opendir(stateDir))) {
> +        if (errno == ENOENT)
> +            return 0;
> +
> +        virReportSystemError(errno, _("Failed to open dir '%s'"), stateDir);
> +        return -1;
> +    }
> +
> +    while ((ret = virDirRead(dir, &entry, stateDir)) > 0) {
> +        virStoragePoolObjPtr pool;
> +
> +        if (entry->d_name[0] == '.')
> +            continue;
> +
> +        if (!virFileStripSuffix(entry->d_name, ".xml"))
> +            continue;
> +
> +        if (!(pool = virStoragePoolLoadState(pools, stateDir, entry->d_name)))
> +            continue;
> +        virStoragePoolObjUnlock(pool);
> +    }
> +
> +    closedir(dir);
> +    return ret;
> +}
> +
> +
>  int
>  virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools,
>                               const char *configDir,
> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
> index 99b2f4a..1f84504 100644
> --- a/src/conf/storage_conf.h
> +++ b/src/conf/storage_conf.h
> @@ -318,6 +318,13 @@ int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools,
>                                   const char *configDir,
>                                   const char *autostartDir);
>  
> +int virStoragePoolLoadAllState(virStoragePoolObjListPtr pools,
> +                               const char *stateDir);
> +
> +virStoragePoolObjPtr
> +virStoragePoolLoadState(virStoragePoolObjListPtr pools,
> +                        const char *stateDir,
> +                        const char *name);
>  virStoragePoolObjPtr
>  virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools,
>                              const unsigned char *uuid);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 689a08f..9bc8de8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -798,6 +798,7 @@ virStoragePoolFormatFileSystemNetTypeToString;
>  virStoragePoolFormatFileSystemTypeToString;
>  virStoragePoolGetVhbaSCSIHostParent;
>  virStoragePoolLoadAllConfigs;
> +virStoragePoolLoadAllState;
>  virStoragePoolObjAssignDef;
>  virStoragePoolObjClearVols;
>  virStoragePoolObjDeleteDef;
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 9bd93d2..d09acce 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -191,6 +191,17 @@ storageStateInitialize(bool privileged,
>      }
>      driver->privileged = privileged;
>  
> +    if (virFileMakePath(driver->stateDir) < 0) {
> +        virReportError(errno,
> +                       _("cannot create directory %s"),
> +                       driver->stateDir);
> +        goto error;
> +    }
> +
> +    if (virStoragePoolLoadAllState(&driver->pools,
> +                                   driver->stateDir) < 0)
> +        goto error;
> +
>      if (virStoragePoolLoadAllConfigs(&driver->pools,
>                                       driver->configDir,
>                                       driver->autostartDir) < 0)
> 


What about storageStateReload ?  IOW the other place where
virStoragePoolLoadAllConfigs is called.

John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list