[libvirt] [PATCH v2 0/8] Duplicate storage pool source refactoring and checks
v1 here: http://www.redhat.com/archives/libvir-list/2015-April/msg00141.html Changes: Patches 1-3 are refactored into just 1 patch now Patch 2 is the old patch 4 and renames the function as requested and removes the comment Patch 3 is new - it adds a check for different port #'s to the new host source matching function. Patch 4 is the old patch 5, already ACK'd and only changed to use the new name Patch 5 is the old patch 6, already ACK'd Patches 6 & 7 are the old patches 7 & 8. They use the new name and the commit comment is adjusted Patch 8 is the old patch 9 and is unchanged, already ACK'd John Ferlan (8): storage: Refactor iSCSI Source matching storage: Create virStoragePoolSourceMatchSingleHost storage: Add check for different ports for host duplicate matching storage: Use virStoragePoolSourceMatchSingleHost for NETFS storage: Remove default from switch in virStoragePoolSourceFindDuplicate storage: Add duplicate host check for Sheepdog pool def storage: Add duplicate host check for Gluster pool def storage: Add duplicate devices check for zfs pool def src/conf/storage_conf.c | 62 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 16 deletions(-) -- 2.1.0
Create a separate iSCSI Source matching subroutine. Makes the calling code a bit cleaner as well as sets up for future patches which need to do better source hosts[0].name processing/checking. As part of the effort the logic will be inverted from a multi-level if statement to a series of single level checks for better readability and further separation Signed-off-by: John Ferlan <jferlan@redhat.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 990a528..351eea8 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2406,6 +2406,26 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, } +static bool +virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr matchpool, + virStoragePoolDefPtr def) +{ + virStoragePoolSourcePtr poolsrc = &matchpool->def->source; + virStoragePoolSourcePtr defsrc = &def->source; + + if (poolsrc->nhost != 1 && defsrc->nhost != 1) + return false; + + if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) + return false; + + if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn)) + return false; + + return true; +} + + int virStoragePoolSourceFindDuplicate(virConnectPtr conn, virStoragePoolObjListPtr pools, @@ -2505,17 +2525,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_ISCSI: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); if (matchpool) { - if (matchpool->def->source.nhost == 1 && def->source.nhost == 1) { - if (STREQ(matchpool->def->source.hosts[0].name, def->source.hosts[0].name)) { - if ((matchpool->def->source.initiator.iqn) && (def->source.initiator.iqn)) { - if (STREQ(matchpool->def->source.initiator.iqn, def->source.initiator.iqn)) - break; - matchpool = NULL; - } - break; - } - } - matchpool = NULL; + if (!virStoragePoolSourceISCSIMatch(matchpool, def)) + matchpool = NULL; } break; case VIR_STORAGE_POOL_FS: -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:05 -0400, John Ferlan wrote:
Create a separate iSCSI Source matching subroutine. Makes the calling code a bit cleaner as well as sets up for future patches which need to do better source hosts[0].name processing/checking.
As part of the effort the logic will be inverted from a multi-level if statement to a series of single level checks for better readability and further separation
Signed-off-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
Double sign-off.
--- src/conf/storage_conf.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)a
ACK, it looks much better now. Peter
Split out the nhost == 1 and hosts[0].name logic into a separate routine Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 351eea8..f609f85 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2405,6 +2405,16 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, return false; } +static bool +virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc, + virStoragePoolSourcePtr defsrc) +{ + if (poolsrc->nhost != 1 && defsrc->nhost != 1) + return false; + + return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name); +} + static bool virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr matchpool, @@ -2413,10 +2423,7 @@ virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr matchpool, virStoragePoolSourcePtr poolsrc = &matchpool->def->source; virStoragePoolSourcePtr defsrc = &def->source; - if (poolsrc->nhost != 1 && defsrc->nhost != 1) - return false; - - if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) + if (!virStoragePoolSourceMatchSingleHost(poolsrc, defsrc)) return false; if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn)) -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:06 -0400, John Ferlan wrote:
Split out the nhost == 1 and hosts[0].name logic into a separate routine
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
ACK, Peter
In virStoragePoolSourceMatchSingleHost, add a comparison for port number being different prior to checking the 'name' field. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f609f85..313098b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2412,6 +2412,9 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc, if (poolsrc->nhost != 1 && defsrc->nhost != 1) return false; + if (poolsrc->hosts[0].port != defsrc->hosts[0].port) + return false; + return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name); } -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:07 -0400, John Ferlan wrote:
In virStoragePoolSourceMatchSingleHost, add a comparison for port number being different prior to checking the 'name' field.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 3 +++ 1 file changed, 3 insertions(+)
ACK
Rather than have duplicate code doing the same check, have the netfs matching processing code use the new virStoragePoolSourceMatchSingleHost. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 313098b..bb89bb7 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2464,9 +2464,9 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = pool; break; case VIR_STORAGE_POOL_NETFS: - if ((STREQ(pool->def->source.dir, def->source.dir)) \ - && (pool->def->source.nhost == 1 && def->source.nhost == 1) \ - && (STREQ(pool->def->source.hosts[0].name, def->source.hosts[0].name))) + if (STREQ(pool->def->source.dir, def->source.dir) && + virStoragePoolSourceMatchSingleHost(&pool->def->source, + &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_SCSI: -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:08 -0400, John Ferlan wrote:
Rather than have duplicate code doing the same check, have the netfs matching processing code use the new virStoragePoolSourceMatchSingleHost.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 313098b..bb89bb7 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2464,9 +2464,9 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = pool; break; case VIR_STORAGE_POOL_NETFS: - if ((STREQ(pool->def->source.dir, def->source.dir)) \ - && (pool->def->source.nhost == 1 && def->source.nhost == 1) \ - && (STREQ(pool->def->source.hosts[0].name, def->source.hosts[0].name))) + if (STREQ(pool->def->source.dir, def->source.dir) && + virStoragePoolSourceMatchSingleHost(&pool->def->source,
NETFS pools don't use port for some reason, but it should be fine to use the same function for it.
+ &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_SCSI:
ACK, Peter
So that we can cover all the cases. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index bb89bb7..1fadff4 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2458,7 +2458,7 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, virStoragePoolObjLock(pool); - switch (pool->def->type) { + switch ((virStoragePoolType)pool->def->type) { case VIR_STORAGE_POOL_DIR: if (STREQ(pool->def->target.path, def->target.path)) matchpool = pool; @@ -2544,7 +2544,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_DISK: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; - default: + case VIR_STORAGE_POOL_MPATH: + case VIR_STORAGE_POOL_RBD: + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_ZFS: + case VIR_STORAGE_POOL_LAST: break; } virStoragePoolObjUnlock(pool); -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:09 -0400, John Ferlan wrote:
So that we can cover all the cases.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
ACK if I've not acked it in the previous version. Peter
Check the proposed pool source host XML definition against existing sheepdog pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 1fadff4..2b2104d 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2544,9 +2544,13 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_DISK: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; + case VIR_STORAGE_POOL_SHEEPDOG: + if (virStoragePoolSourceMatchSingleHost(&pool->def->source, + &def->source)) + matchpool = pool; + break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_SHEEPDOG: case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:10 -0400, John Ferlan wrote:
Check the proposed pool source host XML definition against existing sheepdog pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
ACK, Peter
Check the proposed pool source host XML definition against existing gluster pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2b2104d..0a50f57 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2545,13 +2545,13 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_GLUSTER: if (virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: break; -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:11 -0400, John Ferlan wrote:
Check the proposed pool source host XML definition against existing gluster pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2b2104d..0a50f57 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2545,13 +2545,13 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_GLUSTER:
Gluster still can host multiple pools on a single host:port combination. You also need to check that pool->def->source.dir are same among the two pool definitions.
if (virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: break;
ACK if you add the directory check. Peter
On 04/15/2015 03:57 AM, Peter Krempa wrote:
On Mon, Apr 13, 2015 at 17:21:11 -0400, John Ferlan wrote:
Check the proposed pool source host XML definition against existing gluster pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2b2104d..0a50f57 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2545,13 +2545,13 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_GLUSTER:
Gluster still can host multiple pools on a single host:port combination. You also need to check that pool->def->source.dir are same among the two pool definitions.
if (virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: break;
ACK if you add the directory check.
So then GLUSTER is similar to NETFS then: case VIR_STORAGE_POOL_NETFS: if (STREQ(pool->def->source.dir, def->source.dir) && virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; And I could just move the GLUSTER to that same case, right? John
On Wed, Apr 15, 2015 at 06:47:43 -0400, John Ferlan wrote:
On 04/15/2015 03:57 AM, Peter Krempa wrote:
On Mon, Apr 13, 2015 at 17:21:11 -0400, John Ferlan wrote:
Check the proposed pool source host XML definition against existing gluster pools to ensure the incoming definition doesn't use the same source host XML definition as an existing pool.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2b2104d..0a50f57 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2545,13 +2545,13 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_GLUSTER:
Gluster still can host multiple pools on a single host:port combination. You also need to check that pool->def->source.dir are same among the two pool definitions.
if (virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: break;
ACK if you add the directory check.
So then GLUSTER is similar to NETFS then:
case VIR_STORAGE_POOL_NETFS: if (STREQ(pool->def->source.dir, def->source.dir) && virStoragePoolSourceMatchSingleHost(&pool->def->source, &def->source)) matchpool = pool;
And I could just move the GLUSTER to that same case, right?
Yes, exactly. Peter
Check proposed pool definitions to ensure they aren't trying to use the same devices as currently defined definitions - disallow the duplicate Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0a50f57..9e7c575 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2542,6 +2542,7 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_FS: case VIR_STORAGE_POOL_LOGICAL: case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_ZFS: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: @@ -2552,7 +2553,6 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, break; case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: - case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_LAST: break; } -- 2.1.0
On Mon, Apr 13, 2015 at 17:21:12 -0400, John Ferlan wrote:
Check proposed pool definitions to ensure they aren't trying to use the same devices as currently defined definitions - disallow the duplicate
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK, Peter
participants (2)
-
John Ferlan -
Peter Krempa