[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 08/03/2011 05:29 AM, Daniel P. Berrange wrote:
On Wed, Aug 03, 2011 at 12:52:50AM +0800, Lei Li wrote:
On 08/02/2011 07:11 PM, Daniel P. Berrange wrote:
On Mon, Aug 01, 2011 at 02:12:51PM -0600, Eric Blake wrote:
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)
No, this API is flawed because def->target.path is not required to
be unique for all types of storage pool.

Daniel
Yes, in the beginning it seems like target->path is not required to be unique. But for this bug https://bugzilla.redhat.com/show_bug.cgi?id=611823
you reported, you said that "For example, if two directory pools point to the same directory, and one pool is used to create a volume,
  the other pool will remain unaware of the new volume until it is refreshed."  And I have test it when use 'virsh pool-define/create' it will create more
than two pools not two have the same directory. I think maybe you should look at the description of the bug first.
This API virStoragePoolObjFindByPath() just provide a method to search pool obj by path and can be use to avoid duplicate target path to fix this bug you mentioned.
Ah a slight misunderstanding here. There are quite a few different pieces
of metadata with storage pools, and in some cases they are the same.

  - name/uuid - usual unique metadata for a storage pool
  - source - the data for the source varies according to storage pool type
      - hostname
      - directory path
      - adaptor
      - device path
      - source name
      - initiator IQN

  - target - a path that controls how storage volume paths are formed

I think your misunderstanding is because for 'directory' storage pools,
the source directory path is actually the same as the target path.

So if you want to do uniqueness checking for storage pools, you need
todo it based on the source information, rather than the target path.
The checks will of course need to be slightly different for each
storage pool type.

Regards,
Daniel
Hi Daniel,

I seriously considered your comments and look at the document about storage pool and volume XML format.
Yes, there are kinds type of pools, but the main goal of the bug #611823 you reported is to avoid an
inconsistent view of it's volume created on the same pool. The source directory info for each type of pool
maybe different, but if the target.path controls how storage volume paths are formed, why should we just
check the target.path to avoid this issue?


--
Lei


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