[libvirt] [PATCH 0/2] Add more duplicate scsi_host/fc_host adapter checks
https://bugzilla.redhat.com/show_bug.cgi?id=1159180 Currently libvirt only detects duplicate fc_host & scsi_host adapter sources when the incoming source type definition is the same as the pool type. This misses the even more oddball cases where a scsi_host/fc_host definition is using a scsi_hostN where the type of the definition isn't the same as the type of the pool. It's a fairly twisted and perhaps edge kind of case. A 'fc_host' has two scsi_hostN values that need to be compared against existing 'scsi_host' pools - first the fc_host "parent" scsi_hostN (if it exists or was provided) and second the vHBA scsi_hostN as determined from the wwnn/wwpn. If a 'fc_host' has a 'parent' attribute of a scsi_hostN that's already in use by some 'scsi_host' pool or if it's vHBA (as created via nodedev-create) was (for some unknown reason) used by someone for a 'scsi_host' pool, then we have a duplicate. A 'scsi_host' conversely needs to check against using either the fc_host 'parent' or vHBA. If a 'scsi_host' is using a scsi_hostN of some existing fc_host 'parent' attribute or vHBA, then we have a duplicate. To make matters more complex, the 'fc_host' 'parent' attribute doesn't have to be provided in the fc_host XML. If not provided, then it can be determined if the vHBA already exists (either via nodedev-create or a running fc_host pool). This means moving the code that was created for fc_host startup to find the vHBA parent into the storage_backend_scsi code in order for it to be used there instead. I did try moving into virutil with no success since that code wasn't very happy to be calling the virNodeDevice* API's. The only oddball case that cannot be tested for in this scenario is if the incoming fc_host definition isn't using a nodedev-create'd vHBA and it doesn't provide a 'parent' attribute, then it is possible that the vportCreate startup code will find "an available" scsi_hostN that was (again for some strange reason) already being used by some 'scsi_host' pool. Added some more verbiage to the documentation to dissuade usage of a 'scsi_host' for an FC capable scsi_hostN as well as to encourage usage of the 'parent' attribute in a mixed environment John Ferlan (2): storage: Move and rename getVhbaSCSIHostParent storage: Add mixed fc_host/scsi_host duplicate adapter source checks docs/formatstorage.html.in | 26 +++++- src/conf/storage_conf.c | 178 ++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 8 +- src/libvirt_private.syms | 1 + src/parallels/parallels_storage.c | 2 +- src/storage/storage_backend_scsi.c | 66 +------------- src/storage/storage_driver.c | 4 +- 7 files changed, 215 insertions(+), 70 deletions(-) -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1159180 Move the API from the backend to storage_conf and rename it to virStoragePoolGetVhbaSCSIHostParent. A future patch will need to use this functionality from storage_conf Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 66 ++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 5 +++ src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 66 ++------------------------------------ 4 files changed, 74 insertions(+), 64 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6ad37f8..751c0c0 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2078,6 +2078,72 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, return ret; } +/* + * virStoragePoolGetVhbaSCSIHostParent: + * + * Using the Node Device Driver, find the host# name found via wwnn/wwpn + * lookup in the fc_host sysfs tree (e.g. virGetFCHostNameByWWN) to get + * the parent 'scsi_host#'. + * + * @conn: Connection pointer (must be non-NULL on entry) + * @name: Pointer a string from a virGetFCHostNameByWWN (e.g., "host#") + * + * Returns a "scsi_host#" string of the parent of the vHBA + */ +char * +virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn, + const char *name) +{ + char *nodedev_name = NULL; + virNodeDevicePtr device = NULL; + char *xml = NULL; + virNodeDeviceDefPtr def = NULL; + char *vhba_parent = NULL; + virErrorPtr savedError = NULL; + + VIR_DEBUG("conn=%p, name=%s", conn, name); + + /* We get passed "host#" from the return from virGetFCHostNameByWWN, + * so we need to adjust that to what the nodedev lookup expects + */ + if (virAsprintf(&nodedev_name, "scsi_%s", name) < 0) + goto cleanup; + + /* Compare the scsi_host for the name with the provided parent + * if not the same, then fail + */ + if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) { + virReportError(VIR_ERR_XML_ERROR, + _("Cannot find '%s' in node device database"), + nodedev_name); + goto cleanup; + } + + if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) + goto cleanup; + + if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) + goto cleanup; + + /* The caller checks whether the returned value is NULL or not + * before continuing + */ + ignore_value(VIR_STRDUP(vhba_parent, def->parent)); + + cleanup: + if (!vhba_parent) + savedError = virSaveLastError(); + VIR_FREE(nodedev_name); + virNodeDeviceDefFree(def); + VIR_FREE(xml); + virNodeDeviceFree(device); + if (savedError) { + virSetError(savedError); + virFreeError(savedError); + } + return vhba_parent; +} + static int getSCSIHostNumber(virStoragePoolSourceAdapter adapter, unsigned int *hostnum) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 765f681..228bb1c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -30,6 +30,7 @@ # include "virbitmap.h" # include "virthread.h" # include "device_conf.h" +# include "node_device_conf.h" # include <libxml/tree.h> @@ -389,6 +390,10 @@ int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, unsigned int check_active); +char *virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn, + const char *name) + ATTRIBUTE_NONNULL(1); + int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0864618..ccf1cea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -740,6 +740,7 @@ virStoragePoolDefParseString; virStoragePoolFormatDiskTypeToString; virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; +virStoragePoolGetVhbaSCSIHostParent; virStoragePoolLoadAllConfigs; virStoragePoolObjAssignDef; virStoragePoolObjClearVols; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c952b71..4be525d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -34,7 +34,6 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" -#include "viraccessapicheck.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -548,67 +547,6 @@ getAdapterName(virStoragePoolSourceAdapter adapter) /* * Using the host# name found via wwnn/wwpn lookup in the fc_host - * sysfs tree to get the parent 'scsi_host#'. On entry we need 'conn' - * set. We won't get here from the autostart path since the caller - * will return true before calling this function. For the shutdown - * path we won't be able to delete the vport. - */ -static char * ATTRIBUTE_NONNULL(1) -getVhbaSCSIHostParent(virConnectPtr conn, - const char *name) -{ - char *nodedev_name = NULL; - virNodeDevicePtr device = NULL; - char *xml = NULL; - virNodeDeviceDefPtr def = NULL; - char *vhba_parent = NULL; - virErrorPtr savedError = NULL; - - VIR_DEBUG("conn=%p, name=%s", conn, name); - - /* We get passed "host#" from the return from virGetFCHostNameByWWN, - * so we need to adjust that to what the nodedev lookup expects - */ - if (virAsprintf(&nodedev_name, "scsi_%s", name) < 0) - goto cleanup; - - /* Compare the scsi_host for the name with the provided parent - * if not the same, then fail - */ - if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) { - virReportError(VIR_ERR_XML_ERROR, - _("Cannot find '%s' in node device database"), - nodedev_name); - goto cleanup; - } - - if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) - goto cleanup; - - if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) - goto cleanup; - - /* The caller checks whether the returned value is NULL or not - * before continuing - */ - ignore_value(VIR_STRDUP(vhba_parent, def->parent)); - - cleanup: - if (!vhba_parent) - savedError = virSaveLastError(); - VIR_FREE(nodedev_name); - virNodeDeviceDefFree(def); - VIR_FREE(xml); - virNodeDeviceFree(device); - if (savedError) { - virSetError(savedError); - virFreeError(savedError); - } - return vhba_parent; -} - -/* - * Using the host# name found via wwnn/wwpn lookup in the fc_host * sysfs tree to get the parent 'scsi_host#' to ensure it matches. */ static bool @@ -625,7 +563,7 @@ checkVhbaSCSIHostParent(virConnectPtr conn, if (!conn) return true; - if (!(vhba_parent = getVhbaSCSIHostParent(conn, name))) + if (!(vhba_parent = virStoragePoolGetVhbaSCSIHostParent(conn, name))) goto cleanup; if (STRNEQ(parent_name, vhba_parent)) { @@ -777,7 +715,7 @@ deleteVport(virConnectPtr conn, if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) goto cleanup; } else { - if (!(vhba_parent = getVhbaSCSIHostParent(conn, name))) + if (!(vhba_parent = virStoragePoolGetVhbaSCSIHostParent(conn, name))) goto cleanup; if (virGetSCSIHostNumber(vhba_parent, &parent_host) < 0) -- 1.9.3
On 18.11.2014 22:26, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1159180
Move the API from the backend to storage_conf and rename it to virStoragePoolGetVhbaSCSIHostParent. A future patch will need to use this functionality from storage_conf
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 66 ++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 5 +++ src/libvirt_private.syms | 1 + src/storage/storage_backend_scsi.c | 66 ++------------------------------------ 4 files changed, 74 insertions(+), 64 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6ad37f8..751c0c0 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2078,6 +2078,72 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, return ret; }
+/* + * virStoragePoolGetVhbaSCSIHostParent: + * + * Using the Node Device Driver, find the host# name found via wwnn/wwpn + * lookup in the fc_host sysfs tree (e.g. virGetFCHostNameByWWN) to get + * the parent 'scsi_host#'. + * + * @conn: Connection pointer (must be non-NULL on entry) + * @name: Pointer a string from a virGetFCHostNameByWWN (e.g., "host#") + * + * Returns a "scsi_host#" string of the parent of the vHBA + */ +char * +virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn, + const char *name) +{ + char *nodedev_name = NULL; + virNodeDevicePtr device = NULL; + char *xml = NULL; + virNodeDeviceDefPtr def = NULL; + char *vhba_parent = NULL; + virErrorPtr savedError = NULL; + + VIR_DEBUG("conn=%p, name=%s", conn, name); + + /* We get passed "host#" from the return from virGetFCHostNameByWWN, + * so we need to adjust that to what the nodedev lookup expects + */ + if (virAsprintf(&nodedev_name, "scsi_%s", name) < 0) + goto cleanup; + + /* Compare the scsi_host for the name with the provided parent + * if not the same, then fail + */ + if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) { + virReportError(VIR_ERR_XML_ERROR, + _("Cannot find '%s' in node device database"), + nodedev_name); + goto cleanup; + } + + if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) + goto cleanup; + + if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) + goto cleanup; + + /* The caller checks whether the returned value is NULL or not + * before continuing + */ + ignore_value(VIR_STRDUP(vhba_parent, def->parent)); + + cleanup: + if (!vhba_parent) + savedError = virSaveLastError(); + VIR_FREE(nodedev_name); + virNodeDeviceDefFree(def); + VIR_FREE(xml); + virNodeDeviceFree(device); + if (savedError) { + virSetError(savedError); + virFreeError(savedError); + } + return vhba_parent;
While you're at this, would it make sense to: 1) s/virNodeDeviceFree/virObjectUnref/ 2) drop savedError? Firstly, virNodeDeviceFree() must be guarded with check for device != NULL (yep, our public API madness where free(NULL) is not accepted). Moreover, since it is a public API it resets the error object, which you don't want to and hence you save the error. But using the very same function that virNodeDeviceFree() is using, you will achieve the same goal and without any temporary variables and cleaner code. Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1159180 The virStoragePoolSourceFindDuplicate only checks the incoming definition against the same type of pool as the def; however, for "scsi_host" and "fc_host" adapter pools, it's possible that either some pool "scsi_host" adapter definition is already using the scsi_hostN that the "fc_host" adapter definition wants to use or some "fc_host" pool adapter definition is using a vHBA scsi_hostN or parent scsi_hostN that an incoming "scsi_host" definition is trying to use. This patch adds the mismatched type checks and adds extraneous comments to describe what each check is determining. This patch also modifies the documentation to be describe what scsi_hostN devices a "scsi_host" source adapter should use and which to avoid. It also updates the parent definition to specifically call out that for mixed environments it's better to define which parent to use so that the duplicate pool checks can be done properly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 26 ++++++++- src/conf/storage_conf.c | 112 +++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 3 +- src/parallels/parallels_storage.c | 2 +- src/storage/storage_driver.c | 4 +- 5 files changed, 141 insertions(+), 6 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 29c1931..de786b8 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -157,6 +157,25 @@ compatibility, this attribute is optional <b>only</b> for the "scsi_host" adapter, but is mandatory for the "fc_host" adapter. <span class="since">Since 1.0.5</span> + A "fc_host" capable scsi_hostN can be determined by using + <code>virsh nodedev-list --cap fc_host</code>. + <span class="since">Since 1.2.8</span> + <p> + Note: Regardless of whether a "scsi_host" adapter type is defined + using a <code>name</code> or a <code>parentaddr</code>, it + should refer to a real scsi_host adapter as found through a + <code>virsh nodedev-list scsi_host</code> and <code>virsh + nodedev-dumpxml scsi_hostN</code> on one of the scsi_host's + displayed. It should not refer to a "fc_host" capable scsi_hostN + nor should it refer to the vHBA created for some "fc_host" + adapter. For a vHBA the <code>nodedev-dumpxml</code> + output parent setting will be the "fc_host" capable scsi_hostN + value. Additionally, do not refer to an iSCSI scsi_hostN for the + "scsi_host" source. An iSCSI scsi_hostN's + <code>nodedev-dumpxml</code> output parent field is generally + "computer". This is a libvirt created parent value indicating + no parent was defined for the node device. + </p> </dd> </dl> <dl> @@ -187,7 +206,12 @@ the vHBA created by 'virsh nodedev-create', rather it is the parent of that vHBA. If the value is not provided, libvirt will determine the parent based either finding the wwnn,wwpn - defined for an existing scsi_host or by creating a vHBA. + defined for an existing scsi_host or by creating a vHBA. Providing + the parent attribute is also useful for the duplicate pool + definition checks. This is more important in environments where + both the "fc_host" and "scsi_host" source adapter pools are being + used in order to ensure a new definition doesn't duplicate using + the scsi_hostN of some existing storage pool. <span class="since">Since 1.0.4</span> </dd> <dt><code>managed</code></dt> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 751c0c0..dd09877 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2176,6 +2176,83 @@ getSCSIHostNumber(virStoragePoolSourceAdapter adapter, VIR_FREE(name); return ret; } + +/* + * matchFCHostToSCSIHost: + * + * @conn: Connection pointer + * @fc_adapter: fc_host adapter (either def or pool->def) + * @scsi_hostnum: Already determined "scsi_pool" hostnum + * + * Returns true/false whether there is a match between the incoming + * fc_adapter host# and the scsi_host host# + */ +static bool +matchFCHostToSCSIHost(virConnectPtr conn, + virStoragePoolSourceAdapter fc_adapter, + unsigned int scsi_hostnum) +{ + char *name = NULL; + char *parent_name = NULL; + unsigned int fc_hostnum; + + /* If we have a parent defined, get it's hostnum, and compare to the + * scsi_hostnum. If they are the same, then we have a match + */ + if (fc_adapter.data.fchost.parent && + virGetSCSIHostNumber(fc_adapter.data.fchost.parent, &fc_hostnum) == 0 && + scsi_hostnum == fc_hostnum) + return true; + + /* If we find an fc_adapter name, then either libvirt created a vHBA + * for this fc_host or a 'virsh nodedev-create' generated a vHBA. + */ + if ((name = virGetFCHostNameByWWN(NULL, fc_adapter.data.fchost.wwnn, + fc_adapter.data.fchost.wwpn))) { + + /* Get the scsi_hostN for the vHBA in order to see if it + * matches our scsi_hostnum + */ + if (virGetSCSIHostNumber(name, &fc_hostnum) == 0 && + scsi_hostnum == fc_hostnum) { + VIR_FREE(name); + return true; + } + + /* We weren't provided a parent, so we have to query the node + * device driver in order to ascertain the parent of the vHBA. + * If the parent fc_hostnum is the same as the scsi_hostnum, we + * have a match. + */ + if (conn && !fc_adapter.data.fchost.parent) { + parent_name = virStoragePoolGetVhbaSCSIHostParent(conn, name); + if (parent_name) { + if (virGetSCSIHostNumber(parent_name, &fc_hostnum) == 0 && + scsi_hostnum == fc_hostnum) { + VIR_FREE(parent_name); + VIR_FREE(name); + return true; + } + VIR_FREE(parent_name); + } else { + /* Throw away the error and fall through */ + virResetLastError(); + VIR_DEBUG("Could not determine parent vHBA"); + } + } + VIR_FREE(name); + } + + /* NB: Lack of a name means that this vHBA hasn't yet been created, + * which means our scsi_host cannot be using the vHBA. Furthermore, + * lack of a provided parent means libvirt is going to choose the + * "best" fc_host capable adapter based on availabilty. That could + * conflict with an existing scsi_host definition, but there's no + * way to know that now. + */ + return false; +} + static bool matchSCSIAdapterParent(virStoragePoolObjPtr pool, virStoragePoolDefPtr def) @@ -2200,7 +2277,8 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool, int -virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, +virStoragePoolSourceFindDuplicate(virConnectPtr conn, + virStoragePoolObjListPtr pools, virStoragePoolDefPtr def) { size_t i; @@ -2260,6 +2338,38 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, break; if (pool_hostnum == def_hostnum) matchpool = pool; + } else if (pool->def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST && + def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + unsigned int scsi_hostnum; + + /* Get the scsi_hostN for the scsi_host source adapter def */ + if (getSCSIHostNumber(def->source.adapter, + &scsi_hostnum) < 0) + break; + + if (matchFCHostToSCSIHost(conn, pool->def->source.adapter, + scsi_hostnum)) { + matchpool = pool; + break; + } + + } else if (pool->def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST && + def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + unsigned int scsi_hostnum; + + if (getSCSIHostNumber(pool->def->source.adapter, + &scsi_hostnum) < 0) + break; + + if (matchFCHostToSCSIHost(conn, def->source.adapter, + scsi_hostnum)) { + matchpool = pool; + break; + } } break; case VIR_STORAGE_POOL_ISCSI: diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 228bb1c..2c9eaed 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -394,7 +394,8 @@ char *virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn, const char *name) ATTRIBUTE_NONNULL(1); -int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, +int virStoragePoolSourceFindDuplicate(virConnectPtr conn, + virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); void virStoragePoolObjLock(virStoragePoolObjPtr obj); diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index e1b6ea8..4a16325 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -719,7 +719,7 @@ parallelsStoragePoolDefineXML(virConnectPtr conn, if (virStoragePoolObjIsDuplicate(&privconn->pools, def, 0) < 0) goto cleanup; - if (virStoragePoolSourceFindDuplicate(&privconn->pools, def) < 0) + if (virStoragePoolSourceFindDuplicate(conn, &privconn->pools, def) < 0) goto cleanup; if (parallelsStoragePoolGetAlloc(def)) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9b89a67..0a02cbd 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -608,7 +608,7 @@ storagePoolCreateXML(virConnectPtr conn, if (virStoragePoolObjIsDuplicate(&driver->pools, def, 1) < 0) goto cleanup; - if (virStoragePoolSourceFindDuplicate(&driver->pools, def) < 0) + if (virStoragePoolSourceFindDuplicate(conn, &driver->pools, def) < 0) goto cleanup; if ((backend = virStorageBackendForType(def->type)) == NULL) @@ -667,7 +667,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStoragePoolObjIsDuplicate(&driver->pools, def, 0) < 0) goto cleanup; - if (virStoragePoolSourceFindDuplicate(&driver->pools, def) < 0) + if (virStoragePoolSourceFindDuplicate(conn, &driver->pools, def) < 0) goto cleanup; if (virStorageBackendForType(def->type) == NULL) -- 1.9.3
ping... Tks John On 11/18/2014 04:26 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1159180
Currently libvirt only detects duplicate fc_host & scsi_host adapter sources when the incoming source type definition is the same as the pool type. This misses the even more oddball cases where a scsi_host/fc_host definition is using a scsi_hostN where the type of the definition isn't the same as the type of the pool. It's a fairly twisted and perhaps edge kind of case.
A 'fc_host' has two scsi_hostN values that need to be compared against existing 'scsi_host' pools - first the fc_host "parent" scsi_hostN (if it exists or was provided) and second the vHBA scsi_hostN as determined from the wwnn/wwpn. If a 'fc_host' has a 'parent' attribute of a scsi_hostN that's already in use by some 'scsi_host' pool or if it's vHBA (as created via nodedev-create) was (for some unknown reason) used by someone for a 'scsi_host' pool, then we have a duplicate.
A 'scsi_host' conversely needs to check against using either the fc_host 'parent' or vHBA. If a 'scsi_host' is using a scsi_hostN of some existing fc_host 'parent' attribute or vHBA, then we have a duplicate.
To make matters more complex, the 'fc_host' 'parent' attribute doesn't have to be provided in the fc_host XML. If not provided, then it can be determined if the vHBA already exists (either via nodedev-create or a running fc_host pool). This means moving the code that was created for fc_host startup to find the vHBA parent into the storage_backend_scsi code in order for it to be used there instead. I did try moving into virutil with no success since that code wasn't very happy to be calling the virNodeDevice* API's.
The only oddball case that cannot be tested for in this scenario is if the incoming fc_host definition isn't using a nodedev-create'd vHBA and it doesn't provide a 'parent' attribute, then it is possible that the vportCreate startup code will find "an available" scsi_hostN that was (again for some strange reason) already being used by some 'scsi_host' pool.
Added some more verbiage to the documentation to dissuade usage of a 'scsi_host' for an FC capable scsi_hostN as well as to encourage usage of the 'parent' attribute in a mixed environment
John Ferlan (2): storage: Move and rename getVhbaSCSIHostParent storage: Add mixed fc_host/scsi_host duplicate adapter source checks
docs/formatstorage.html.in | 26 +++++- src/conf/storage_conf.c | 178 ++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 8 +- src/libvirt_private.syms | 1 + src/parallels/parallels_storage.c | 2 +- src/storage/storage_backend_scsi.c | 66 +------------- src/storage/storage_driver.c | 4 +- 7 files changed, 215 insertions(+), 70 deletions(-)
On 18.11.2014 22:26, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1159180
Currently libvirt only detects duplicate fc_host & scsi_host adapter sources when the incoming source type definition is the same as the pool type. This misses the even more oddball cases where a scsi_host/fc_host definition is using a scsi_hostN where the type of the definition isn't the same as the type of the pool. It's a fairly twisted and perhaps edge kind of case.
A 'fc_host' has two scsi_hostN values that need to be compared against existing 'scsi_host' pools - first the fc_host "parent" scsi_hostN (if it exists or was provided) and second the vHBA scsi_hostN as determined from the wwnn/wwpn. If a 'fc_host' has a 'parent' attribute of a scsi_hostN that's already in use by some 'scsi_host' pool or if it's vHBA (as created via nodedev-create) was (for some unknown reason) used by someone for a 'scsi_host' pool, then we have a duplicate.
A 'scsi_host' conversely needs to check against using either the fc_host 'parent' or vHBA. If a 'scsi_host' is using a scsi_hostN of some existing fc_host 'parent' attribute or vHBA, then we have a duplicate.
To make matters more complex, the 'fc_host' 'parent' attribute doesn't have to be provided in the fc_host XML. If not provided, then it can be determined if the vHBA already exists (either via nodedev-create or a running fc_host pool). This means moving the code that was created for fc_host startup to find the vHBA parent into the storage_backend_scsi code in order for it to be used there instead. I did try moving into virutil with no success since that code wasn't very happy to be calling the virNodeDevice* API's.
The only oddball case that cannot be tested for in this scenario is if the incoming fc_host definition isn't using a nodedev-create'd vHBA and it doesn't provide a 'parent' attribute, then it is possible that the vportCreate startup code will find "an available" scsi_hostN that was (again for some strange reason) already being used by some 'scsi_host' pool.
Added some more verbiage to the documentation to dissuade usage of a 'scsi_host' for an FC capable scsi_hostN as well as to encourage usage of the 'parent' attribute in a mixed environment
John Ferlan (2): storage: Move and rename getVhbaSCSIHostParent storage: Add mixed fc_host/scsi_host duplicate adapter source checks
docs/formatstorage.html.in | 26 +++++- src/conf/storage_conf.c | 178 ++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 8 +- src/libvirt_private.syms | 1 + src/parallels/parallels_storage.c | 2 +- src/storage/storage_backend_scsi.c | 66 +------------- src/storage/storage_driver.c | 4 +- 7 files changed, 215 insertions(+), 70 deletions(-)
ACK series. But see my comment to 1/2. Michal
On 11/18/2014 04:26 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1159180
Currently libvirt only detects duplicate fc_host & scsi_host adapter sources when the incoming source type definition is the same as the pool type. This misses the even more oddball cases where a scsi_host/fc_host definition is using a scsi_hostN where the type of the definition isn't the same as the type of the pool. It's a fairly twisted and perhaps edge kind of case.
A 'fc_host' has two scsi_hostN values that need to be compared against existing 'scsi_host' pools - first the fc_host "parent" scsi_hostN (if it exists or was provided) and second the vHBA scsi_hostN as determined from the wwnn/wwpn. If a 'fc_host' has a 'parent' attribute of a scsi_hostN that's already in use by some 'scsi_host' pool or if it's vHBA (as created via nodedev-create) was (for some unknown reason) used by someone for a 'scsi_host' pool, then we have a duplicate.
A 'scsi_host' conversely needs to check against using either the fc_host 'parent' or vHBA. If a 'scsi_host' is using a scsi_hostN of some existing fc_host 'parent' attribute or vHBA, then we have a duplicate.
To make matters more complex, the 'fc_host' 'parent' attribute doesn't have to be provided in the fc_host XML. If not provided, then it can be determined if the vHBA already exists (either via nodedev-create or a running fc_host pool). This means moving the code that was created for fc_host startup to find the vHBA parent into the storage_backend_scsi code in order for it to be used there instead. I did try moving into virutil with no success since that code wasn't very happy to be calling the virNodeDevice* API's.
The only oddball case that cannot be tested for in this scenario is if the incoming fc_host definition isn't using a nodedev-create'd vHBA and it doesn't provide a 'parent' attribute, then it is possible that the vportCreate startup code will find "an available" scsi_hostN that was (again for some strange reason) already being used by some 'scsi_host' pool.
Added some more verbiage to the documentation to dissuade usage of a 'scsi_host' for an FC capable scsi_hostN as well as to encourage usage of the 'parent' attribute in a mixed environment
John Ferlan (2): storage: Move and rename getVhbaSCSIHostParent storage: Add mixed fc_host/scsi_host duplicate adapter source checks
docs/formatstorage.html.in | 26 +++++- src/conf/storage_conf.c | 178 ++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 8 +- src/libvirt_private.syms | 1 + src/parallels/parallels_storage.c | 2 +- src/storage/storage_backend_scsi.c | 66 +------------- src/storage/storage_driver.c | 4 +- 7 files changed, 215 insertions(+), 70 deletions(-)
I adjusted patch1 to follow Michal's request to regarding virNodeDeviceFree and savedError and then pushed the patches. Thanks - John Ironically I'm also putting together something for that virXXXFree issue...
participants (2)
-
John Ferlan -
Michal Privoznik