[libvirt] [PATCH] conf: storage: Fix duplicate check for gluster pools
The pool name has to be the same too to warrant rejecting a pool definition as duplicate. This regression was introduced in commit 2184ade3a0546b915252cb3b6a5dc88e9a8d2ccf. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1236438 --- src/conf/storage_conf.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4bbed4f..4fa065d 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2471,13 +2471,22 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, if (STREQ(pool->def->target.path, def->target.path)) matchpool = pool; break; - case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_GLUSTER: + if (STREQ(pool->def->source.name, def->source.name) && + STREQ(pool->def->source.dir, def->source.dir) && + virStoragePoolSourceMatchSingleHost(&pool->def->source, + &def->source)) + matchpool = pool; + break; + + case VIR_STORAGE_POOL_NETFS: if (STREQ(pool->def->source.dir, def->source.dir) && virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; break; + case VIR_STORAGE_POOL_SCSI: if (pool->def->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST && -- 2.4.1
On 06/30/2015 04:20 AM, Peter Krempa wrote:
The pool name has to be the same too to warrant rejecting a pool definition as duplicate. This regression was introduced in commit 2184ade3a0546b915252cb3b6a5dc88e9a8d2ccf.
How did it check duplicates before that commit? The difference there shows no checks being done on gluster at all... So semantically, not sure it's a regression... That commit just added a check to ensure the host names and 'dir' weren't the same... If anything it didn't go "far enough" to check the source.name.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1236438 --- src/conf/storage_conf.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
While rereading virStorageBackendGlusterOpen, I'm now wondering... Does 'dir' have to be provided? The virStorageBackendGlusterOpen indicates that 'dir' could be NULL and would default to "/" ACK (although perhaps the STREQ_NULLABLE may need to be used for dir) John
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4bbed4f..4fa065d 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2471,13 +2471,22 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, if (STREQ(pool->def->target.path, def->target.path)) matchpool = pool; break; - case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_GLUSTER: + if (STREQ(pool->def->source.name, def->source.name) && + STREQ(pool->def->source.dir, def->source.dir) && + virStoragePoolSourceMatchSingleHost(&pool->def->source, + &def->source)) + matchpool = pool; + break; + + case VIR_STORAGE_POOL_NETFS: if (STREQ(pool->def->source.dir, def->source.dir) && virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; break; + case VIR_STORAGE_POOL_SCSI: if (pool->def->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST &&
On Tue, Jun 30, 2015 at 06:48:45 -0400, John Ferlan wrote:
On 06/30/2015 04:20 AM, Peter Krempa wrote:
The pool name has to be the same too to warrant rejecting a pool definition as duplicate. This regression was introduced in commit 2184ade3a0546b915252cb3b6a5dc88e9a8d2ccf.
How did it check duplicates before that commit? The difference there
It was not checked in any way :), thus ...
shows no checks being done on gluster at all... So semantically, not sure it's a regression... That commit just added a check to ensure the host names and 'dir' weren't the same... If anything it didn't go "far enough" to check the source.name.
If you have a volume with different name on the same server and you want to use the same path there then libvirt will forbid that and that would not be forbidden before. $ git desc 2184ade3a0546b915252cb3b6a5dc88e9a8d2ccf v1.2.14-168-g2184ade As it was released already it is a regression in legitimate behaviour.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1236438 --- src/conf/storage_conf.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
While rereading virStorageBackendGlusterOpen, I'm now wondering... Does 'dir' have to be provided? The virStorageBackendGlusterOpen indicates that 'dir' could be NULL and would default to "/"
ACK (although perhaps the STREQ_NULLABLE may need to be used for dir)
You are right, "dir" actually at least according to the code in the gluster backend can be NULL, so I'll add the _NULLABLE part.
John
Thanks. Peter
participants (2)
-
John Ferlan -
Peter Krempa