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