[libvirt] [Libvirt] [PATCH v2] Fix bug #611823 prohibit pools with duplicate storage
Make sure the unique storage pool defined and create from different directory to avoid inconsistent version of volume pool created. 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(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 995f9a6..9078f78 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1317,6 +1317,21 @@ virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, return NULL; } +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; +} + void virStoragePoolObjClearVols(virStoragePoolObjPtr pool) { @@ -1707,6 +1722,27 @@ cleanup: return ret; } +int virStoragePoolTargetDuplicate(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def) +{ + int ret = 1; + virStoragePoolObjPtr pool = NULL; + + /* Check the pool list if defined target path already exist */ + pool = virStoragePoolObjFindByPath(pools, def->target.path); + if (pool) { + virStorageReportError(VIR_ERR_OPERATION_FAILED, + _("target path '%s' is already in use"), + pool->def->target.path); + ret = -1; + goto cleanup; + } + +cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + return ret; +} void virStoragePoolObjLock(virStoragePoolObjPtr obj) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 271441a..454c43d 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -335,6 +335,8 @@ virStoragePoolObjPtr virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, const unsigned char *uuid); virStoragePoolObjPtr virStoragePoolObjFindByName(virStoragePoolObjListPtr pools, const char *name); +virStoragePoolObjPtr virStoragePoolObjFindByPath(virStoragePoolObjListPtr pools, + const char *path); virStorageVolDefPtr virStorageVolDefFindByKey(virStoragePoolObjPtr pool, const char *key); @@ -387,6 +389,8 @@ char *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def); int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, unsigned int check_active); +int virStoragePoolTargetDuplicate(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 830222b..37afaf2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -937,7 +937,9 @@ virStoragePoolObjClearVols; virStoragePoolObjDeleteDef; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; +virStoragePoolObjFindByPath; virStoragePoolObjIsDuplicate; +virStoragePoolTargetDuplicate; virStoragePoolObjListFree; virStoragePoolObjLock; virStoragePoolObjRemove; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9c353e3..b757911 100644 --- a/src/storage/storage_driver.c +++ 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; + if ((backend = virStorageBackendForType(def->type)) == NULL) goto cleanup; @@ -589,6 +592,9 @@ storagePoolDefine(virConnectPtr conn, if (virStoragePoolObjIsDuplicate(&driver->pools, def, 0) < 0) goto cleanup; + if (virStoragePoolTargetDuplicate(&driver->pools, def) < 0) + goto cleanup; + if (virStorageBackendForType(def->type) == NULL) goto cleanup; -- 1.7.1
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
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 -- |: 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 :|
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,
On 08/02/2011 07:11 PM, Daniel P. Berrange wrote: 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. BTW, I found that there are 3 method provide ways to search by 'key','name','path' in storage volume also. -- Lei
On Wed, Aug 03, 2011 at 12:52:50AM +0800, Lei Li 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,
On 08/02/2011 07:11 PM, Daniel P. Berrange wrote: 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 -- |: 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 :|
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 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,
On 08/02/2011 07:11 PM, Daniel P. Berrange wrote: 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
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 And you said in the bug description that "The simplest example is two directory pools that point to the same directory, but iSCSI and other pool types behave similarly." Based on your description, step to reproduce and expected results, I look at the code about process of storage pool, I agree with your conclusion. But now I was confused for your comment "target.path is not required to be unique for all types of storage pool".
-- Lei
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Lei Li