[libvirt] [PATCH] Source control for storage pool
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 li...@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; +} +} +} +} +} else { +/* For the remain three pool type 'fs''logical''disk' that all use device path */ +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; +} void virStoragePoolObjLock(virStoragePoolObjPtr obj) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 271441a..a8562a6 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -388,6 +388,9 @@ int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, unsigned int check_active); +int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def); + void virStoragePoolObjLock(virStoragePoolObjPtr obj); void virStoragePoolObjUnlock(virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f03e30..5899d56 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -948,6 +948,7 @@ virStoragePoolObjDeleteDef; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; virStoragePoolObjIsDuplicate; +virStoragePoolSourceFindDuplicate; virStoragePoolObjListFree; virStoragePoolObjLock; virStoragePoolObjRemove; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 68cac1f..c05b74e 100644 ---
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 li...@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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list