[libvirt] [Libvirt] [PATCH v2] Fix bug #611823 prohibit pools with duplicate storage

[Eric Blake]

On 07/31/2011 10:58 PM, Lei Li wrote:
> Make sure the unique storage pool defined and create from different directory to avoid inconsistent version of volume pool created.

Wrap your commit messages; typically at 70 columns or so (since 'git 
log' adds some indentation, but you want the end result to still fit in 
80 columns for legibility).

>
> Signed-off-by: Lei Li<lilei at linux.vnet.ibm.com>
> ---
>   src/conf/storage_conf.c      |   36 ++++++++++++++++++++++++++++++++++++
>   src/conf/storage_conf.h      |    4 ++++
>   src/libvirt_private.syms     |    2 ++
>   src/storage/storage_driver.c |    6 ++++++
>   4 files changed, 48 insertions(+), 0 deletions(-)
>

>
> +virStoragePoolObjPtr
> +virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools,
> +                            const char *path) {
> +    unsigned int i;
> +
> +    for (i = 0 ; i<  pools->count ; i++) {
> +        virStoragePoolObjLock(pools->objs[i]);
> +        if (STREQ(pools->objs[i]->def->target.path, path))
> +            return pools->objs[i];
> +        virStoragePoolObjUnlock(pools->objs[i]);
> +    }
> +
> +    return NULL;
> +}

This one is good; in fact, we may even want to expose it as a public 
API, parallel to other virStoragePoolLookupBy* functions (but not until 
after 0.9.4 is released)

>
> +int virStoragePoolTargetDuplicate(virStoragePoolObjListPtr pools,
> +                                  virStoragePoolDefPtr def)
> +{
> +    int ret = 1;
> +    virStoragePoolObjPtr pool = NULL;
> +
> +    /* Check the pool list if defined target path already exist */

s/exist/exists/

However, I'm not sure if this warrants a separate function.  Instead, 
should we just fold this additional logic search into 
virStoragePoolObjIsDuplicate?

> +++ b/src/libvirt_private.syms
> @@ -937,7 +937,9 @@ virStoragePoolObjClearVols;
>   virStoragePoolObjDeleteDef;
>   virStoragePoolObjFindByName;
>   virStoragePoolObjFindByUUID;
> +virStoragePoolObjFindByPath;
>   virStoragePoolObjIsDuplicate;
> +virStoragePoolTargetDuplicate;

Sort these lines (or just the virStoragePoolObjFindByPath, if we go with 
inlining the duplicate target search into virStoragePoolObjIsDuplicate).

> +++ b/src/storage/storage_driver.c
> @@ -536,6 +536,9 @@ storagePoolCreate(virConnectPtr conn,
>       if (virStoragePoolObjIsDuplicate(&driver->pools, def, 1)<  0)
>           goto cleanup;
>
> +    if (virStoragePoolTargetDuplicate(&driver->pools, def)<  0)
> +        goto cleanup;
> +

Given my consolidation ideas, you wouldn't even have to touch 
storage_driver.c.

While I like the idea, I think that it was first proposed too late after 
the 0.9.4 RC1, and it isn't a show-stopper bug, so I'd feel better 
deferring this to post-release and a v3 patch.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org