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

Re: [libvirt] [PATCH] Source control for storage pool



On Thu, Sep 01, 2011 at 02:49:04PM +0800, Lei Li wrote:
> Fix bug #611823 storage driver should prohibit pools with duplicate underlying
> storage.
> 
> Add API virStoragePoolSourceFindDuplicate() to do uniqueness check based on
> source location infomation for pool type.
> 
> 
> Signed-off-by: Lei Li <lilei linux vnet ibm com>
> ---
>  src/conf/storage_conf.c      |   73 ++++++++++++++++++++++++++++++++++++++++++
>  src/conf/storage_conf.h      |    3 ++
>  src/libvirt_private.syms     |    1 +
>  src/storage/storage_driver.c |    6 +++
>  4 files changed, 83 insertions(+), 0 deletions(-)
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 8d14e87..698334e 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1701,6 +1701,79 @@ cleanup:
>      return ret;
>  }
>  
> +int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
> +                                      virStoragePoolDefPtr def)
> +{
> +    int i, j, k;
> +    int ret = 1;
> +    virStoragePoolObjPtr pool = NULL;
> +    virStoragePoolObjPtr matchpool = NULL;
> +
> +    /* Check the pool list for duplicate underlying storage */
> +    for (i = 0; i < pools->count; i++) {
> +        pool = pools->objs[i];
> +        if (def->type != pool->def->type)
> +            continue;
> +
> +        virStoragePoolObjLock(pool);
> +        if (pool->def->type == VIR_STORAGE_POOL_DIR) {
> +            if (STREQ(pool->def->target.path, def->target.path)) {
> +                matchpool = pool;
> +                goto cleanup;
> +            }
> +        } else if (pool->def->type == VIR_STORAGE_POOL_NETFS) {
> +            if ((STREQ(pool->def->source.dir, def->source.dir)) \
> +                && (STREQ(pool->def->source.host.name, def->source.host.name))) {
> +                matchpool = pool;
> +                goto cleanup;
> +            }
> +        } else if (pool->def->type == VIR_STORAGE_POOL_SCSI) {
> +            if (STREQ(pool->def->source.adapter, def->source.adapter)) {
> +                matchpool = pool;
> +                goto cleanup;
> +            }
> +        } else if (pool->def->type == VIR_STORAGE_POOL_ISCSI) {
> +            for (j = 0; j < pool->def->source.ndevice; j++) {
> +                for (k = 0; k < def->source.ndevice; k++) {
> +                    if (pool->def->source.initiator.iqn) {
> +                        if ((STREQ(pool->def->source.devices[j].path, def->source.devices[k].path)) \
> +                            && (STREQ(pool->def->source.initiator.iqn, def->source.initiator.iqn))) {
> +                            matchpool = pool;
> +                            goto cleanup;
> +                        }
> +                    } else if (pool->def->source.host.name) {
> +                        if ((STREQ(pool->def->source.devices[j].path, def->source.devices[k].path)) \
> +                            && (STREQ(pool->def->source.host.name, def->source.host.name))) {
> +                            matchpool = pool;
> +                            goto cleanup;
> +                        }
> +                    }

Slight change needed here. The 'source.host.name' is compulsory and always
present for iSCSI. So you always need to comper path + host.name. The IQN
is optional, so should onlybe compared if it is present in *both* pools.
Your current code can SEGV if  def->source.initiator.iqn is NULL.

> +        } else {
> +            /* For the remain three pool type 'fs''logical''disk' that all use device path */

We might add further pool types later, and we don't want this branch accidentally
executed for them. I think it would thus be preferrable to turn this long if/else
into a switch() block. Then explicitly list FS, LOGICAL & DISK here, and have a
separate 'default:' block which is a no-op.

> +            if (pool->def->source.ndevice) {
> +                for (j = 0; j < pool->def->source.ndevice; j++) {
> +                    for (k = 0; k < def->source.ndevice; k++) {
> +                        if (STREQ(pool->def->source.devices[j].path, def->source.devices[k].path)) {
> +                            matchpool = pool;
> +                            goto cleanup;
> +                        }
> +                    }
> +                }
> +            }
> +        }
> +        virStoragePoolObjUnlock(pool);
> +    }
> +cleanup:
> +    if (matchpool) {
> +           virStoragePoolObjUnlock(matchpool);
> +           virStorageReportError(VIR_ERR_OPERATION_FAILED,
> +                                 _("pool source location info is duplicate"));
> +           ret = -1;
> +    }
> +    return ret;
> +}

Aside from those comments, this looks like the right approach to the
problem

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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