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

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



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 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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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