[libvirt] [PATCH 0/9] Duplicate storage pool source refactoring and checks
A precursor to some changes I'm working on - I figured it was better not to hoard the patches and then drop a 15 patch series... Anyway, patches 1-4 refactor the iSCSI <source> duplicate checks into a separate subroutine. While I was at it, I realized the Host checks are duplicate of the NETFS check - so I further separated things out. As I was looking at other possible defs I realized not all of the proposed storage pool defs go through duplicate checks because the switch statement had the 'default:' case - so I removed that, added all the options with the hope that new storage pool types will add their own checking. This left me with a bit of sleuthing for cases I added. I was able to determine that Sheepdog and Gluster both have the possibility of "a" single source host, so I added that check as it doesn't seem like it would be a good idea to have two storage pools looking at the same gluster/sheepdog source For the zfs pool it seems that it's like the DISK, LOGICAL, and FS pools with regard to not having duplicate devices in the pool For mpath there doesn't seem to be any need, but I could be wrong For rbd it wasn't completely clear what to check since multiple hosts are allowed for a single pool - I suppose a proposed definition should check that none of it's definitions match, but then again I'm not clear so I left it alone John Ferlan (9): storage: Refactor iSCSI Source matching storage: Refactor matchISCSISource storage: Invert logic for matchISCSISource storage: Create matchPoolSourceHost storage: Use matchPoolSourceHost 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 | 59 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 43 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 Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b1898b..4a38416 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, } +static bool +matchISCSISource(virStoragePoolObjPtr matchpool, + virStoragePoolDefPtr def) +{ + 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)) + return true; + else + return false; + } + return true; + } + } + return false; +} + + int virStoragePoolSourceFindDuplicate(virConnectPtr conn, virStoragePoolObjListPtr pools, @@ -2390,17 +2412,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 (!matchISCSISource(matchpool, def)) + matchpool = NULL; } break; case VIR_STORAGE_POOL_FS: -- 2.1.0
On Thu, Apr 02, 2015 at 13:39:38 -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
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b1898b..4a38416 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, }
+static bool +matchISCSISource(virStoragePoolObjPtr matchpool,
Please use the virStorageConf... prefix for the new function.
+ virStoragePoolDefPtr def) +{ + 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)) + return true; + else + return false;
Um, how about return STREQ(... ?
+ } + return true; + } + } + return false; +} + + int virStoragePoolSourceFindDuplicate(virConnectPtr conn, virStoragePoolObjListPtr pools, @@ -2390,17 +2412,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 (!matchISCSISource(matchpool, def)) + matchpool = NULL; } break; case VIR_STORAGE_POOL_FS:
ACK if you rename the function and remove the redundant if. Peter
On 04/13/2015 05:27 AM, Peter Krempa wrote:
On Thu, Apr 02, 2015 at 13:39:38 -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
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b1898b..4a38416 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, }
+static bool +matchISCSISource(virStoragePoolObjPtr matchpool,
Please use the virStorageConf... prefix for the new function.
How about "virStoragePoolSourceISCSIMatch"
+ virStoragePoolDefPtr def) +{ + 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)) + return true; + else + return false;
Um, how about return STREQ(... ?
myopia, but in the long run it won't matter as I agree with your view to merge patches 1-3 (something I probably thought along the way but didn't type as I was trying to show the transformation for at least the review) John
+ } + return true; + } + } + return false; +} + + int virStoragePoolSourceFindDuplicate(virConnectPtr conn, virStoragePoolObjListPtr pools, @@ -2390,17 +2412,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 (!matchISCSISource(matchpool, def)) + matchpool = NULL; } break; case VIR_STORAGE_POOL_FS:
ACK if you rename the function and remove the redundant if.
Peter
On Mon, Apr 13, 2015 at 15:35:46 -0400, John Ferlan wrote:
On 04/13/2015 05:27 AM, Peter Krempa wrote:
On Thu, Apr 02, 2015 at 13:39:38 -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
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b1898b..4a38416 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, }
+static bool +matchISCSISource(virStoragePoolObjPtr matchpool,
Please use the virStorageConf... prefix for the new function.
How about "virStoragePoolSourceISCSIMatch"
Sounds good to me. Peter
On 04/14/2015 02:43 AM, Peter Krempa wrote:
On Mon, Apr 13, 2015 at 15:35:46 -0400, John Ferlan wrote:
On 04/13/2015 05:27 AM, Peter Krempa wrote:
On Thu, Apr 02, 2015 at 13:39:38 -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
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b1898b..4a38416 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, }
+static bool +matchISCSISource(virStoragePoolObjPtr matchpool,
Please use the virStorageConf... prefix for the new function.
How about "virStoragePoolSourceISCSIMatch"
Sounds good to me.
I posted a v2 to make viewing easier: http://www.redhat.com/archives/libvir-list/2015-April/msg00519.html I'll wait for ACK's from that series just to be safe Tks - John
Continuing on the same reduction them, foobar'd Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4a38416..fa7a7f9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2295,13 +2295,12 @@ static bool matchISCSISource(virStoragePoolObjPtr matchpool, virStoragePoolDefPtr def) { - 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)) + virStoragePoolSourcePtr poolsrc = &matchpool->def->source; + virStoragePoolSourcePtr defsrc = &def->source; + if (poolsrc->nhost == 1 && defsrc->nhost == 1) { + if (STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) { + if (poolsrc->initiator.iqn && defsrc->initiator.iqn) { + if (STREQ(poolsrc->initiator.iqn, defsrc->initiator.iqn)) return true; else return false; -- 2.1.0
Invert the logic for better readability/flow and futher separation Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index fa7a7f9..e4cb54b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2297,18 +2297,19 @@ matchISCSISource(virStoragePoolObjPtr matchpool, { virStoragePoolSourcePtr poolsrc = &matchpool->def->source; virStoragePoolSourcePtr defsrc = &def->source; - if (poolsrc->nhost == 1 && defsrc->nhost == 1) { - if (STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) { - if (poolsrc->initiator.iqn && defsrc->initiator.iqn) { - if (STREQ(poolsrc->initiator.iqn, defsrc->initiator.iqn)) - return true; - else - return false; - } - return true; - } - } - return false; + + + /* NB: nhost cannot be > 1 */ + if (poolsrc->nhost == 0 || defsrc->nhost == 0) + 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; } -- 2.1.0
On Thu, Apr 02, 2015 at 13:39:40 -0400, John Ferlan wrote:
Invert the logic for better readability/flow and futher separation
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index fa7a7f9..e4cb54b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2297,18 +2297,19 @@ matchISCSISource(virStoragePoolObjPtr matchpool, { virStoragePoolSourcePtr poolsrc = &matchpool->def->source; virStoragePoolSourcePtr defsrc = &def->source; - if (poolsrc->nhost == 1 && defsrc->nhost == 1) { - if (STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) { - if (poolsrc->initiator.iqn && defsrc->initiator.iqn) { - if (STREQ(poolsrc->initiator.iqn, defsrc->initiator.iqn)) - return true; - else - return false; - } - return true; - } - } - return false; + + + /* NB: nhost cannot be > 1 */
Remove this comment ...
+ if (poolsrc->nhost == 0 || defsrc->nhost == 0)
change the conditions to != 1 so that the comment is redundant.
+ 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; }
... and squash this together with patches 1 and 2. Doing the refactor right away is probably better a in this case. 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 | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e4cb54b..b3e930b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2290,6 +2290,17 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, return false; } +static bool +matchPoolSourceHost(virStoragePoolSourcePtr poolsrc, + virStoragePoolSourcePtr defsrc) +{ + /* NB: nhost cannot be > 1 */ + if (poolsrc->nhost == 0 || defsrc->nhost == 0) + return false; + + return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name); +} + static bool matchISCSISource(virStoragePoolObjPtr matchpool, @@ -2299,11 +2310,7 @@ matchISCSISource(virStoragePoolObjPtr matchpool, virStoragePoolSourcePtr defsrc = &def->source; - /* NB: nhost cannot be > 1 */ - if (poolsrc->nhost == 0 || defsrc->nhost == 0) - return false; - - if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) + if (!matchPoolSourceHost(poolsrc, defsrc)) return false; if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn)) -- 2.1.0
On Thu, Apr 02, 2015 at 13:39:41 -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 | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e4cb54b..b3e930b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2290,6 +2290,17 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, return false; }
+static bool +matchPoolSourceHost(virStoragePoolSourcePtr poolsrc,
Same compliant for the function name as in 1/9.
+ virStoragePoolSourcePtr defsrc) +{ + /* NB: nhost cannot be > 1 */ + if (poolsrc->nhost == 0 || defsrc->nhost == 0) + return false;
And this condition can be made explicitly state the same without the need for the comment. ACK with the name and condition changed. Peter
On 04/13/2015 06:18 AM, Peter Krempa wrote:
On Thu, Apr 02, 2015 at 13:39:41 -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 | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e4cb54b..b3e930b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2290,6 +2290,17 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, return false; }
+static bool +matchPoolSourceHost(virStoragePoolSourcePtr poolsrc,
Same compliant for the function name as in 1/9.
So how about: virStoragePoolSourceMatchSingleHost As you probably eventually realized I punted on the multiple host case of NBD - although it probably needs similar logic, but I was intent on doing the cases where only one host was used by the backends. BTW: Where this is headed is we currently only match the host[0] by string, but that's not good enough for networks where one can use a 'name' or a 'number' (and then all sorts of fun with ipv4 v ipv6). John
+ virStoragePoolSourcePtr defsrc) +{ + /* NB: nhost cannot be > 1 */ + if (poolsrc->nhost == 0 || defsrc->nhost == 0) + return false;
And this condition can be made explicitly state the same without the need for the comment.
ACK with the name and condition changed.
Peter
So up through this point the diff to master would be: +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, + virStoragePoolDefPtr def) +{ + virStoragePoolSourcePtr poolsrc = &matchpool->def->source; + virStoragePoolSourcePtr defsrc = &def->source; + + if (!virStoragePoolSourceMatchSingleHost(poolsrc, defsrc)) + return false; + + if (STRNEQ_NULLABLE(poolsrc->initiator.iqn, defsrc->initiator.iqn)) + return false; + + return true; +} + int virStoragePoolSourceFindDuplicate(virConnectPtr conn, @@ -2505,17 +2532,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:
Rather than have duplicate code doing the same check, have the netfs matching processing code use the new matchPoolSourceHost. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index b3e930b..6ed0aa9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2348,9 +2348,8 @@ 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) && + matchPoolSourceHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_SCSI: -- 2.1.0
On Thu, Apr 02, 2015 at 13:39:42 -0400, John Ferlan wrote:
Rather than have duplicate code doing the same check, have the netfs matching processing code use the new matchPoolSourceHost.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index b3e930b..6ed0aa9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2348,9 +2348,8 @@ 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) && + matchPoolSourceHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_SCSI:
ACK, no semantic change. (But of course the patch will need a change after renaming the function. Peter
On 04/13/2015 06:19 AM, Peter Krempa wrote:
On Thu, Apr 02, 2015 at 13:39:42 -0400, John Ferlan wrote:
Rather than have duplicate code doing the same check, have the netfs matching processing code use the new matchPoolSourceHost.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index b3e930b..6ed0aa9 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2348,9 +2348,8 @@ 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) && + matchPoolSourceHost(&pool->def->source, &def->source)) matchpool = pool; break; case VIR_STORAGE_POOL_SCSI:
ACK, no semantic change. (But of course the patch will need a change after renaming the function.
Right - I'll wait of course for your acceptance of the name I used and adjust from there. John
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 6ed0aa9..5f1c151 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2342,7 +2342,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; @@ -2427,7 +2427,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 Thu, Apr 02, 2015 at 13:39:43 -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, Peter
Check proposed pool definitions to ensure they aren't trying to use the same host as currently defined definitions - disallow the duplicate Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5f1c151..5db7478 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2427,9 +2427,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_DISK: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; + case VIR_STORAGE_POOL_SHEEPDOG: + if (matchPoolSourceHost(&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 Thu, Apr 02, 2015 at 13:39:44 -0400, John Ferlan wrote:
Check proposed pool definitions to ensure they aren't trying to use the same host as currently defined definitions - disallow the duplicate
This statement is invalid. Multiple pols can be hosted on a single host. The check needs to do better than just check the host name. Port and pool path may differ denoting a different pool. Btw same host can be described using multiple host strings so it also isn't absolute.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5f1c151..5db7478 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2427,9 +2427,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_DISK: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; + case VIR_STORAGE_POOL_SHEEPDOG: + if (matchPoolSourceHost(&pool->def->source, &def->source)) + matchpool = pool; + break;
Peter
On 04/13/2015 06:23 AM, Peter Krempa wrote:
On Thu, Apr 02, 2015 at 13:39:44 -0400, John Ferlan wrote:
Check proposed pool definitions to ensure they aren't trying to use the same host as currently defined definitions - disallow the duplicate
This statement is invalid. Multiple pols can be hosted on a single host.
Hmm - brain shorthand... How about: 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.
The check needs to do better than just check the host name. Port and pool path may differ denoting a different pool.
Hmm.. yes 'port' is something I could add to virStoragePoolSourceMatchSingleHost and it's also extendable to iSCSI... doesn't make sense for NETFS, but would also be usable for gluster I'll squeeze in a patch in order to handle.
Btw same host can be described using multiple host strings so it also isn't absolute.
Yep... That's where we're trying to get, but it takes a bit to get there! For example, I use "192.168.122.1' for my '<host name='...'/> string; however, if I add to /etc/hosts: 192.168.122.1 test1 and then use 'test1' in a different definition - the new code will fail to match, but they are essentially the same thing... There's a bz for that which I'm working to fix, but was trying to avoid a 20 patch series to do so... Gotta start somewhere. John BTW: It gets worse once IPv6 is added into the mix.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5f1c151..5db7478 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2427,9 +2427,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, case VIR_STORAGE_POOL_DISK: matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; + case VIR_STORAGE_POOL_SHEEPDOG: + if (matchPoolSourceHost(&pool->def->source, &def->source)) + matchpool = pool; + break;
Peter
Check proposed pool definitions to ensure they aren't trying to use the same host 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 5db7478..e1599a4 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2428,12 +2428,12 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def); break; case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_GLUSTER: if (matchPoolSourceHost(&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 Thu, Apr 02, 2015 at 13:39:45 -0400, John Ferlan wrote:
Check proposed pool definitions to ensure they aren't trying to use the same host 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(-)
Same problem as 7/9. 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 e1599a4..b90af3a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2425,6 +2425,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: @@ -2434,7 +2435,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 Thu, Apr 02, 2015 at 13:39:46 -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