[libvirt] [PATCH] Source control for storage pool

2011-09-01 Thread Lei Li 
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

2011-09-01 Thread Daniel P. Berrange 
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