9:45 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1233129
Refactor the virStoragePoolSourceFindDuplicateDevices code a bit to
make it a bit easier to read, then allow pool def type's that use a
pool source device to define the source device to perform cross pool
source device conflicts.
John Ferlan (4):
conf: Introduce virStoragePoolObjSourceMatchTypeDIR
conf: Introduce virStoragePoolObjSourceMatchTypeISCSI
conf: Introduce virStoragePoolObjSourceMatchTypeDEVICE
conf: Check for storage conflicts across pool types
src/conf/virstorageobj.c | 241 +++++++++++++++++++++++++++--------------------
1 file changed, 140 insertions(+), 101 deletions(-)
--
2.9.3
9:45 a.m.
New subject: [libvirt] [PATCH 1/4] conf: Introduce virStoragePoolObjSourceMatchTypeDIR
Refactor virStoragePoolObjSourceFindDuplicate into smaller units
separated by the "supported" pool source type. The DIR, GLUSTER,
and NETFS pools all can use "<source>... <dir='%s'/>...
</source>".
Alter the logic slightly to return the matching pool or NULL rather
than setting matchpool = pool and break. Easier to read that way.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virstorageobj.c | 40 +++++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 34f2eb7..19898a3 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -697,6 +697,30 @@ virStoragePoolSourceISCSIMatch(virStoragePoolObjPtr matchpool,
}
+static virStoragePoolObjPtr
+virStoragePoolObjSourceMatchTypeDIR(virStoragePoolObjPtr pool,
+ virStoragePoolDefPtr def)
+{
+ if (pool->def->type == VIR_STORAGE_POOL_DIR) {
+ if (STREQ(pool->def->target.path, def->target.path))
+ return pool;
+ } else if (pool->def->type == VIR_STORAGE_POOL_GLUSTER) {
+ if (STREQ(pool->def->source.name, def->source.name) &&
+ STREQ_NULLABLE(pool->def->source.dir, def->source.dir) &&
+ virStoragePoolSourceMatchSingleHost(&pool->def->source,
+ &def->source))
+ return pool;
+ } else if (pool->def->type == VIR_STORAGE_POOL_NETFS) {
+ if (STREQ(pool->def->source.dir, def->source.dir) &&
+ virStoragePoolSourceMatchSingleHost(&pool->def->source,
+ &def->source))
+ return pool;
+ }
+
+ return NULL;
+}
+
+
int
virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
virStoragePoolObjListPtr pools,
@@ -723,23 +747,9 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
switch ((virStoragePoolType)pool->def->type) {
case VIR_STORAGE_POOL_DIR:
- if (STREQ(pool->def->target.path, def->target.path))
- matchpool = pool;
- break;
-
case VIR_STORAGE_POOL_GLUSTER:
- if (STREQ(pool->def->source.name, def->source.name) &&
- STREQ_NULLABLE(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;
+ matchpool = virStoragePoolObjSourceMatchTypeDIR(pool, def);
break;
case VIR_STORAGE_POOL_SCSI:
--
2.9.3
9:45 a.m.
New subject: [libvirt] [PATCH 2/4] conf: Introduce virStoragePoolObjSourceMatchTypeISCSI
In the effort to reduce the virStoragePoolObjSourceFindDuplicate logic,
create a new helper which will handle all the ISCSI type differences.
Alter things just a little bit to return NULL or pool rather than
using breaks and matchpool = pool, then break. Also rather than creating
variables withing the if...else if... conditions, have them all at the
top of the function to make things a bit easier to read.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virstorageobj.c | 142 +++++++++++++++++++++++------------------------
1 file changed, 68 insertions(+), 74 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 19898a3..a62d1d7 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -721,6 +721,72 @@ virStoragePoolObjSourceMatchTypeDIR(virStoragePoolObjPtr pool,
}
+static virStoragePoolObjPtr
+virStoragePoolObjSourceMatchTypeISCSI(virStoragePoolObjPtr pool,
+ virStoragePoolDefPtr def,
+ virConnectPtr conn)
+{
+ virStorageAdapterPtr pool_adapter = &pool->def->source.adapter;
+ virStorageAdapterPtr def_adapter = &def->source.adapter;
+ virStorageAdapterSCSIHostPtr pool_scsi_host;
+ virStorageAdapterSCSIHostPtr def_scsi_host;
+ virStorageAdapterFCHostPtr pool_fchost;
+ virStorageAdapterFCHostPtr def_fchost;
+ unsigned int pool_hostnum;
+ unsigned int def_hostnum;
+ unsigned int scsi_hostnum;
+
+ if (pool_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
+ def_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) {
+ pool_fchost = &pool_adapter->data.fchost;
+ def_fchost = &def_adapter->data.fchost;
+
+ if (STREQ(pool_fchost->wwnn, def_fchost->wwnn) &&
+ STREQ(pool_fchost->wwpn, def_fchost->wwpn))
+ return pool;
+ } else if (pool_adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST &&
+ def_adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) {
+ pool_scsi_host = &pool_adapter->data.scsi_host;
+ def_scsi_host = &def_adapter->data.scsi_host;
+
+ if (pool_scsi_host->has_parent &&
+ def_scsi_host->has_parent &&
+ matchSCSIAdapterParent(pool_scsi_host, def_scsi_host))
+ return pool;
+
+ if (getSCSIHostNumber(pool_scsi_host, &pool_hostnum) < 0 ||
+ getSCSIHostNumber(def_scsi_host, &def_hostnum) < 0)
+ return NULL;
+ if (pool_hostnum == def_hostnum)
+ return pool;
+ } else if (pool_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
+ def_adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) {
+ pool_fchost = &pool_adapter->data.fchost;
+ def_scsi_host = &def_adapter->data.scsi_host;
+
+ /* Get the scsi_hostN for the scsi_host source adapter def */
+ if (getSCSIHostNumber(def_scsi_host, &scsi_hostnum) < 0)
+ return NULL;
+
+ if (matchFCHostToSCSIHost(conn, pool_fchost, scsi_hostnum))
+ return pool;
+
+ } else if (pool_adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST &&
+ def_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) {
+ pool_scsi_host = &pool_adapter->data.scsi_host;
+ def_fchost = &def_adapter->data.fchost;
+
+ if (getSCSIHostNumber(pool_scsi_host, &scsi_hostnum) < 0)
+ return NULL;
+
+ if (matchFCHostToSCSIHost(conn, def_fchost, scsi_hostnum))
+ return pool;
+ }
+
+ return NULL;
+}
+
+
int
virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
virStoragePoolObjListPtr pools,
@@ -730,8 +796,6 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
int ret = 1;
virStoragePoolObjPtr pool = NULL;
virStoragePoolObjPtr matchpool = NULL;
- virStorageAdapterPtr pool_adapter;
- virStorageAdapterPtr def_adapter;
/* Check the pool list for duplicate underlying storage */
for (i = 0; i < pools->count; i++) {
@@ -753,79 +817,9 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
break;
case VIR_STORAGE_POOL_SCSI:
- pool_adapter = &pool->def->source.adapter;
- def_adapter = &def->source.adapter;
-
- if (pool_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
- def_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) {
- virStorageAdapterFCHostPtr pool_fchost =
- &pool_adapter->data.fchost;
- virStorageAdapterFCHostPtr def_fchost =
- &def_adapter->data.fchost;
-
- if (STREQ(pool_fchost->wwnn, def_fchost->wwnn) &&
- STREQ(pool_fchost->wwpn, def_fchost->wwpn))
- matchpool = pool;
- } else if (pool_adapter->type ==
- VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST &&
- def_adapter->type ==
- VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) {
- virStorageAdapterSCSIHostPtr pool_scsi_host =
- &pool_adapter->data.scsi_host;
- virStorageAdapterSCSIHostPtr def_scsi_host =
- &def_adapter->data.scsi_host;
- unsigned int pool_hostnum, def_hostnum;
-
- if (pool_scsi_host->has_parent &&
- def_scsi_host->has_parent &&
- matchSCSIAdapterParent(pool_scsi_host, def_scsi_host)) {
- matchpool = pool;
- break;
- }
-
- if (getSCSIHostNumber(pool_scsi_host, &pool_hostnum) < 0 ||
- getSCSIHostNumber(def_scsi_host, &def_hostnum) < 0)
- break;
- if (pool_hostnum == def_hostnum)
- matchpool = pool;
- } else if (pool_adapter->type ==
- VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
- def_adapter->type ==
- VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) {
- virStorageAdapterFCHostPtr pool_fchost =
- &pool_adapter->data.fchost;
- virStorageAdapterSCSIHostPtr def_scsi_host =
- &def_adapter->data.scsi_host;
- unsigned int scsi_hostnum;
-
- /* Get the scsi_hostN for the scsi_host source adapter def */
- if (getSCSIHostNumber(def_scsi_host, &scsi_hostnum) < 0)
- break;
-
- if (matchFCHostToSCSIHost(conn, pool_fchost, scsi_hostnum)) {
- matchpool = pool;
- break;
- }
-
- } else if (pool_adapter->type ==
- VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST &&
- def_adapter->type ==
- VIR_STORAGE_ADAPTER_TYPE_FC_HOST) {
- virStorageAdapterSCSIHostPtr pool_scsi_host =
- &pool_adapter->data.scsi_host;
- virStorageAdapterFCHostPtr def_fchost =
- &def_adapter->data.fchost;
- unsigned int scsi_hostnum;
-
- if (getSCSIHostNumber(pool_scsi_host, &scsi_hostnum) < 0)
- break;
-
- if (matchFCHostToSCSIHost(conn, def_fchost, scsi_hostnum)) {
- matchpool = pool;
- break;
- }
- }
+ matchpool = virStoragePoolObjSourceMatchTypeISCSI(pool, def, conn);
break;
+
case VIR_STORAGE_POOL_ISCSI:
matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
if (matchpool) {
--
2.9.3
9:45 a.m.
New subject: [libvirt] [PATCH 3/4] conf: Introduce virStoragePoolObjSourceMatchTypeDEVICE
Refactor virStoragePoolObjSourceFindDuplicate into smaller units
separated by the "supported" pool source type. The ISCSI, FS,
LOGICAL, DISK, and ZFS pools can use "<source>...
<device='%s'/>...
</source>".
Alter the logic slightly to return the matching pool or NULL rather
than setting matchpool = pool and break. Easier to read that way.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virstorageobj.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index a62d1d7..1623162 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -787,6 +787,28 @@ virStoragePoolObjSourceMatchTypeISCSI(virStoragePoolObjPtr pool,
}
+static virStoragePoolObjPtr
+virStoragePoolObjSourceMatchTypeDEVICE(virStoragePoolObjPtr pool,
+ virStoragePoolDefPtr def)
+{
+ virStoragePoolObjPtr matchpool = NULL;
+
+ if (pool->def->type == VIR_STORAGE_POOL_ISCSI) {
+ if ((matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def))) {
+ if (!virStoragePoolSourceISCSIMatch(matchpool, def))
+ return NULL;
+ }
+ return matchpool;
+ }
+
+ /* VIR_STORAGE_POOL_FS
+ * VIR_STORAGE_POOL_LOGICAL
+ * VIR_STORAGE_POOL_DISK
+ * VIR_STORAGE_POOL_ZFS */
+ return virStoragePoolSourceFindDuplicateDevices(pool, def);
+}
+
+
int
virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
virStoragePoolObjListPtr pools,
@@ -821,18 +843,13 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
break;
case VIR_STORAGE_POOL_ISCSI:
- matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
- if (matchpool) {
- if (!virStoragePoolSourceISCSIMatch(matchpool, def))
- matchpool = NULL;
- }
- break;
case VIR_STORAGE_POOL_FS:
case VIR_STORAGE_POOL_LOGICAL:
case VIR_STORAGE_POOL_DISK:
case VIR_STORAGE_POOL_ZFS:
- matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
+ matchpool = virStoragePoolObjSourceMatchTypeDEVICE(pool, def);
break;
+
case VIR_STORAGE_POOL_SHEEPDOG:
if (virStoragePoolSourceMatchSingleHost(&pool->def->source,
&def->source))
--
2.9.3
9:45 a.m.
New subject: [libvirt] [PATCH 4/4] conf: Check for storage conflicts across pool types
https://bugzilla.redhat.com/show_bug.cgi?id=1233129
The virStoragePoolObjSourceFindDuplicate logic used by PoolCreateXML
and PoolDefineXML avoids comparing the new definition against "other"
pool types. This can cause unexpected corruption if two different pool
source types used the same source device path. For example, a 'disk'
pool using source type device=/dev/sdc could be unwittingly overwritten
by using /dev/sdc for a 'logical' pool which also uses the source
device path.
So rather than blindly ignoring those checks when def->type !=
pool->def->type - have the pool->def->type switch logic handle the
check for which def->type's should be checked.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virstorageobj.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 1623162..ae327d9 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -794,6 +794,9 @@ virStoragePoolObjSourceMatchTypeDEVICE(virStoragePoolObjPtr pool,
virStoragePoolObjPtr matchpool = NULL;
if (pool->def->type == VIR_STORAGE_POOL_ISCSI) {
+ if (def->type != VIR_STORAGE_POOL_ISCSI)
+ return NULL;
+
if ((matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def))) {
if (!virStoragePoolSourceISCSIMatch(matchpool, def))
return NULL;
@@ -801,6 +804,9 @@ virStoragePoolObjSourceMatchTypeDEVICE(virStoragePoolObjPtr pool,
return matchpool;
}
+ if (def->type == VIR_STORAGE_POOL_ISCSI)
+ return NULL;
+
/* VIR_STORAGE_POOL_FS
* VIR_STORAGE_POOL_LOGICAL
* VIR_STORAGE_POOL_DISK
@@ -822,8 +828,6 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
/* Check the pool list for duplicate underlying storage */
for (i = 0; i < pools->count; i++) {
pool = pools->objs[i];
- if (def->type != pool->def->type)
- continue;
/* Don't match against ourself if re-defining existing pool ! */
if (STREQ(pool->def->name, def->name))
@@ -835,11 +839,14 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
case VIR_STORAGE_POOL_DIR:
case VIR_STORAGE_POOL_GLUSTER:
case VIR_STORAGE_POOL_NETFS:
- matchpool = virStoragePoolObjSourceMatchTypeDIR(pool, def);
+ if (def->type == pool->def->type)
+ matchpool = virStoragePoolObjSourceMatchTypeDIR(pool, def);
break;
case VIR_STORAGE_POOL_SCSI:
- matchpool = virStoragePoolObjSourceMatchTypeISCSI(pool, def, conn);
+ if (def->type == pool->def->type)
+ matchpool = virStoragePoolObjSourceMatchTypeISCSI(pool, def,
+ conn);
break;
case VIR_STORAGE_POOL_ISCSI:
@@ -847,22 +854,33 @@ virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
case VIR_STORAGE_POOL_LOGICAL:
case VIR_STORAGE_POOL_DISK:
case VIR_STORAGE_POOL_ZFS:
- matchpool = virStoragePoolObjSourceMatchTypeDEVICE(pool, def);
+ if (def->type == VIR_STORAGE_POOL_ISCSI ||
+ def->type == VIR_STORAGE_POOL_FS ||
+ def->type == VIR_STORAGE_POOL_LOGICAL ||
+ def->type == VIR_STORAGE_POOL_DISK ||
+ def->type == VIR_STORAGE_POOL_ZFS)
+ matchpool = virStoragePoolObjSourceMatchTypeDEVICE(pool, def);
break;
case VIR_STORAGE_POOL_SHEEPDOG:
- if (virStoragePoolSourceMatchSingleHost(&pool->def->source,
+ if (def->type == pool->def->type &&
+ virStoragePoolSourceMatchSingleHost(&pool->def->source,
&def->source))
matchpool = pool;
break;
+
case VIR_STORAGE_POOL_MPATH:
/* Only one mpath pool is valid per host */
- matchpool = pool;
+ if (def->type == pool->def->type)
+ matchpool = pool;
break;
+
case VIR_STORAGE_POOL_VSTORAGE:
- if (STREQ(pool->def->source.name, def->source.name))
+ if (def->type == pool->def->type &&
+ STREQ(pool->def->source.name, def->source.name))
matchpool = pool;
break;
+
case VIR_STORAGE_POOL_RBD:
case VIR_STORAGE_POOL_LAST:
break;
--
2.9.3
6:59 p.m.
ping?
Tks -
John
On 04/05/2017 10:45 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1233129
Refactor the virStoragePoolSourceFindDuplicateDevices code a bit to
make it a bit easier to read, then allow pool def type's that use a
pool source device to define the source device to perform cross pool
source device conflicts.
John Ferlan (4):
conf: Introduce virStoragePoolObjSourceMatchTypeDIR
conf: Introduce virStoragePoolObjSourceMatchTypeISCSI
conf: Introduce virStoragePoolObjSourceMatchTypeDEVICE
conf: Check for storage conflicts across pool types
src/conf/virstorageobj.c | 241 +++++++++++++++++++++++++++--------------------
1 file changed, 140 insertions(+), 101 deletions(-)
8:48 a.m.
On 04/05/2017 04:45 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1233129
Refactor the virStoragePoolSourceFindDuplicateDevices code a bit to
make it a bit easier to read, then allow pool def type's that use a
pool source device to define the source device to perform cross pool
source device conflicts.
John Ferlan (4):
conf: Introduce virStoragePoolObjSourceMatchTypeDIR
conf: Introduce virStoragePoolObjSourceMatchTypeISCSI
conf: Introduce virStoragePoolObjSourceMatchTypeDEVICE
conf: Check for storage conflicts across pool types
src/conf/virstorageobj.c | 241 +++++++++++++++++++++++++++--------------------
1 file changed, 140 insertions(+), 101 deletions(-)
ACK series.
Michal
2780
days inactive
2788
days old
6 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik