12:39 p.m.
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
12:39 p.m.
New subject: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
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(a)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
4:27 a.m.
New subject: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
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(a)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
2:35 p.m.
New subject: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
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(a)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
1:43 a.m.
New subject: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
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(a)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
8:21 p.m.
New subject: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
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(a)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
12:39 p.m.
New subject: [libvirt] [PATCH 2/9] storage: Refactor matchISCSISource
Continuing on the same reduction them, foobar'd
Signed-off-by: John Ferlan <jferlan(a)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
12:39 p.m.
New subject: [libvirt] [PATCH 3/9] storage: Invert logic for matchISCSISource
Invert the logic for better readability/flow and futher separation
Signed-off-by: John Ferlan <jferlan(a)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
5:12 a.m.
New subject: [libvirt] [PATCH 3/9] storage: Invert logic for matchISCSISource
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(a)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
12:39 p.m.
New subject: [libvirt] [PATCH 4/9] storage: Create matchPoolSourceHost
Split out the nhost == 1 and hosts[0].name logic into a separate routine
Signed-off-by: John Ferlan <jferlan(a)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
5:18 a.m.
New subject: [libvirt] [PATCH 4/9] storage: Create matchPoolSourceHost
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(a)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
3:12 p.m.
New subject: [libvirt] [PATCH 4/9] storage: Create matchPoolSourceHost
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(a)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:
12:39 p.m.
New subject: [libvirt] [PATCH 5/9] storage: Use matchPoolSourceHost for NETFS
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(a)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
5:19 a.m.
New subject: [libvirt] [PATCH 5/9] storage: Use matchPoolSourceHost for NETFS
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(a)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
3:16 p.m.
New subject: [libvirt] [PATCH 5/9] storage: Use matchPoolSourceHost for NETFS
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(a)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
12:39 p.m.
New subject: [libvirt] [PATCH 6/9] storage: Remove default from switch in virStoragePoolSourceFindDuplicate
So that we can cover all the cases.
Signed-off-by: John Ferlan <jferlan(a)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
5:19 a.m.
New subject: [libvirt] [PATCH 6/9] storage: Remove default from switch in virStoragePoolSourceFindDuplicate
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(a)redhat.com>
---
src/conf/storage_conf.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
ACK,
Peter
12:39 p.m.
New subject: [libvirt] [PATCH 7/9] storage: Add duplicate host check for Sheepdog pool def
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(a)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
5:23 a.m.
New subject: [libvirt] [PATCH 7/9] storage: Add duplicate host check for Sheepdog pool def
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(a)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
4:07 p.m.
New subject: [libvirt] [PATCH 7/9] storage: Add duplicate host check for Sheepdog pool def
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(a)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
12:39 p.m.
New subject: [libvirt] [PATCH 8/9] storage: Add duplicate host check for Gluster pool def
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(a)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
5:24 a.m.
New subject: [libvirt] [PATCH 8/9] storage: Add duplicate host check for Gluster pool def
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(a)redhat.com>
---
src/conf/storage_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Same problem as 7/9.
Peter
12:39 p.m.
New subject: [libvirt] [PATCH 9/9] storage: Add duplicate devices check for zfs pool def
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(a)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
5:27 a.m.
New subject: [libvirt] [PATCH 9/9] storage: Add duplicate devices check for zfs pool def
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(a)redhat.com>
---
src/conf/storage_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK,
Peter
3509
days inactive
3522
days old
23 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa