[libvirt] [PATCH 0/4] Allow some cross pool type source device path checks
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
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@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
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@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
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@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
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@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
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(-)
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
participants (2)
-
John Ferlan -
Michal Privoznik