https://bugzilla.redhat.com/show_bug.cgi?id=1458708
When originally designed in commit id '42a021c1', providing the
'parent' attribute to the <adapter type='fc_host' wwnn='vHBA_wwnn'
wwpn='vHBA_wwpn'/> was checked to make sure that the "parent" of
the wwnn/wwpn matched that of the provided parent "just in case"
someone created the node device first, then defined the storage
pool using that node device afterwards. The result is to not
perform the vport_create call when the scsi_host represented by
the wwnn/wwpn already exists since it would fail.
Eventually someone came up with the brilliant idea to provide
wwnn/wwpn of an HBA instead of a vHBA, e.g. <adapter type='fc_host'
wwnn='HBA_wwnn' wwpn='HBA_wwpn'/>. This is the same as creating a
storage pool backed to the HBA that just happens to also be vport
(vHBA) capable. The logic to bypass the vport_create call was the
same as the vHBA code since the wwn's already exist. Once that was
determined to work, adding in the 'parent' attribute caused a problem
for the DeleteVport code, which was fixed by commit id '2c8e30ee7e'.
The next test tried was providing a valid pair of wwns that would
find the scsi_host HBA, but providing the wrong name for the 'parent'
attribute. This caused a different failure because at DeleteVport
time if a parent is provided it was assumed valid especially since
the CreateVport code would check that by calling virVHBAPathExists.
So alter the checkParent code to first ensure that the provided
parent_name is a valid HBA/vHBA, then check if the found scsi_host
is an HBA or a vHBA and make the appropriate check against the
parent_name similar to the check made in virNodeDeviceDeleteVport.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/storage/storage_backend_scsi.c | 47 ++++++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 7 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 365ab77..d225e4f 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -220,6 +220,7 @@ checkParent(virConnectPtr conn,
const char *name,
const char *parent_name)
{
+ unsigned int host_num;
char *scsi_host_name = NULL;
char *vhba_parent = NULL;
bool retval = false;
@@ -230,20 +231,52 @@ checkParent(virConnectPtr conn,
if (!conn)
return true;
- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+ if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' is not properly formatted"), name);
goto cleanup;
+ }
- if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+ if (!virVHBAPathExists(NULL, host_num)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ ("parent '%s' is not a vHBA/HBA"), parent_name);
goto cleanup;
+ }
- if (STRNEQ(parent_name, vhba_parent)) {
- virReportError(VIR_ERR_XML_ERROR,
- _("Parent attribute '%s' does not match parent '%s' "
- "determined for the '%s' wwnn/wwpn lookup."),
- parent_name, vhba_parent, name);
+ if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+ goto cleanup;
+
+ if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("host name '%s' is not properly formatted"), name);
goto cleanup;
}
+ /* If scsi_host_name is vport capable, then it's an HBA, thus compare
+ * only against the parent_name; otherwise, as long as the scsi_host_name
+ * path exists, then the scsi_host_name is a vHBA in which case we need
+ * to compare against it's parent. */
+ if (virVHBAIsVportCapable(NULL, host_num)) {
+ if (STRNEQ(parent_name, scsi_host_name)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("parent HBA '%s' doesn't match the wwnn/wwpn "
+ "scsi_host '%s'"),
+ parent_name, scsi_host_name);
+ goto cleanup;
+ }
+ } else {
+ if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+ goto cleanup;
+
+ if (STRNEQ(parent_name, vhba_parent)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("parent vHBA '%s' doesn't match the wwnn/wwpn "
+ "scsi_host '%s' parent '%s'"),
+ parent_name, scsi_host_name, vhba_parent);
+ goto cleanup;
+ }
+ }
+
retval = true;
cleanup:
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 18, 2017 at 11:10:40AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1458708 > > When originally designed in commit id '42a021c1', providing the > 'parent' attribute to the <adapter type='fc_host' wwnn='vHBA_wwnn' > wwpn='vHBA_wwpn'/> was checked to make sure that the "parent" of > the wwnn/wwpn matched that of the provided parent "just in case" > someone created the node device first, then defined the storage > pool using that node device afterwards. The result is to not > perform the vport_create call when the scsi_host represented by > the wwnn/wwpn already exists since it would fail. Okay, so we can both agree that these scenarios are rather unlikely. Why can't we just ignore the 'parent' attribute completely once we find out that a device with corresponding wwnn and wwpn already exist? Delete wouldn't be a problem in that case either, we do have a functional logic querying the parent automatically if none had been provided at the time of creation of the pool. Erik > > Eventually someone came up with the brilliant idea to provide > wwnn/wwpn of an HBA instead of a vHBA, e.g. <adapter type='fc_host' > wwnn='HBA_wwnn' wwpn='HBA_wwpn'/>. This is the same as creating a > storage pool backed to the HBA that just happens to also be vport > (vHBA) capable. The logic to bypass the vport_create call was the > same as the vHBA code since the wwn's already exist. Once that was > determined to work, adding in the 'parent' attribute caused a problem > for the DeleteVport code, which was fixed by commit id '2c8e30ee7e'. > > The next test tried was providing a valid pair of wwns that would > find the scsi_host HBA, but providing the wrong name for the 'parent' > attribute. This caused a different failure because at DeleteVport > time if a parent is provided it was assumed valid especially since > the CreateVport code would check that by calling virVHBAPathExists. > > So alter the checkParent code to first ensure that the provided > parent_name is a valid HBA/vHBA, then check if the found scsi_host > is an HBA or a vHBA and make the appropriate check against the > parent_name similar to the check made in virNodeDeviceDeleteVport. > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/19/2017 08:42 AM, Erik Skultety wrote: > On Tue, Jul 18, 2017 at 11:10:40AM -0400, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1458708 >> >> When originally designed in commit id '42a021c1', providing the >> 'parent' attribute to the <adapter type='fc_host' wwnn='vHBA_wwnn' >> wwpn='vHBA_wwpn'/> was checked to make sure that the "parent" of >> the wwnn/wwpn matched that of the provided parent "just in case" >> someone created the node device first, then defined the storage >> pool using that node device afterwards. The result is to not >> perform the vport_create call when the scsi_host represented by >> the wwnn/wwpn already exists since it would fail. > > Okay, so we can both agree that these scenarios are rather unlikely. Why can't > we just ignore the 'parent' attribute completely once we find out that a device > with corresponding wwnn and wwpn already exist? Delete wouldn't be a problem in > that case either, we do have a functional logic querying the parent > automatically if none had been provided at the time of creation of the pool. > > Erik > Nothing quickly sprang to mind until I read the storage pool section on @parent and remembered that it's useful to avoid multiple pools using the same source as the duplicate pool checks done as part of virStoragePoolObjSourceMatchTypeISCSI John >> >> Eventually someone came up with the brilliant idea to provide >> wwnn/wwpn of an HBA instead of a vHBA, e.g. <adapter type='fc_host' >> wwnn='HBA_wwnn' wwpn='HBA_wwpn'/>. This is the same as creating a >> storage pool backed to the HBA that just happens to also be vport >> (vHBA) capable. The logic to bypass the vport_create call was the >> same as the vHBA code since the wwn's already exist. Once that was >> determined to work, adding in the 'parent' attribute caused a problem >> for the DeleteVport code, which was fixed by commit id '2c8e30ee7e'. >> >> The next test tried was providing a valid pair of wwns that would >> find the scsi_host HBA, but providing the wrong name for the 'parent' >> attribute. This caused a different failure because at DeleteVport >> time if a parent is provided it was assumed valid especially since >> the CreateVport code would check that by calling virVHBAPathExists. >> >> So alter the checkParent code to first ensure that the provided >> parent_name is a valid HBA/vHBA, then check if the found scsi_host >> is an HBA or a vHBA and make the appropriate check against the >> parent_name similar to the check made in virNodeDeviceDeleteVport. >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 365ab77..d225e4f 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -220,6 +220,7 @@ checkParent(virConnectPtr conn,
> const char *name,
> const char *parent_name)
> {
> + unsigned int host_num;
> char *scsi_host_name = NULL;
> char *vhba_parent = NULL;
> bool retval = false;
> @@ -230,20 +231,52 @@ checkParent(virConnectPtr conn,
> if (!conn)
> return true;
>
> - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("parent '%s' is not properly formatted"), name);
> goto cleanup;
> + }
>
> - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> + if (!virVHBAPathExists(NULL, host_num)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + ("parent '%s' is not a vHBA/HBA"), parent_name);
Under what circumstances can parent be a ^^^^vHBA??
> goto cleanup;
> + }
>
> - if (STRNEQ(parent_name, vhba_parent)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Parent attribute '%s' does not match parent '%s' "
> - "determined for the '%s' wwnn/wwpn lookup."),
> - parent_name, vhba_parent, name);
> + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> + goto cleanup;
> +
> + if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("host name '%s' is not properly formatted"), name);
> goto cleanup;
> }
>
> + /* If scsi_host_name is vport capable, then it's an HBA, thus compare
> + * only against the parent_name; otherwise, as long as the scsi_host_name
> + * path exists, then the scsi_host_name is a vHBA in which case we need
> + * to compare against it's parent. */
> + if (virVHBAIsVportCapable(NULL, host_num)) {
I truly find it pointless to try to do any kind of parent validation on HBA
device. Can a HBA device actually have a scsi_host parent? If not, then we
should only validate whether the device given by its wwnn and wwpn exist (we
already know that since we're here) is vport capable and then if the 'parent'
attribute has been defined, then return an error about invalid configuration,
rather than trying to compare parent names in that matter.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/19/2017 09:21 AM, Erik Skultety wrote:
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index 365ab77..d225e4f 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -220,6 +220,7 @@ checkParent(virConnectPtr conn,
>> const char *name,
>> const char *parent_name)
>> {
>> + unsigned int host_num;
>> char *scsi_host_name = NULL;
>> char *vhba_parent = NULL;
>> bool retval = false;
>> @@ -230,20 +231,52 @@ checkParent(virConnectPtr conn,
>> if (!conn)
>> return true;
>>
>> - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("parent '%s' is not properly formatted"), name);
>> goto cleanup;
>> + }
>>
>> - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
>> + if (!virVHBAPathExists(NULL, host_num)) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + ("parent '%s' is not a vHBA/HBA"), parent_name);
>
> Under what circumstances can parent be a ^^^^vHBA??
>
The function tests whether "/sys/class/fc_host/host%d" exists... On my
system:
ls /sys/class/fc_host
host3 host4 host8
where host3 and host4 are HBA and host8 is vHBA
The difference between them? host3/host4 have files "vport_create",
"vport_delete", "max_npiv_vports", "npiv_vports_inuse", and "issue_lip".
The XML for an HBA would be:
<adapter type='fc_host' [parent='scsi_host3'] wwnn='HBA_wwnn'
wwpn='HBA_wwpn'/>
and the vHBA:
<adapter type='fc_host' [parent='scsi_host3'] wwnn='vHBA_wwnn'
wwpn='vHBA_wwpn'/>
Both are legal XML.
>> goto cleanup;
>> + }
>>
>> - if (STRNEQ(parent_name, vhba_parent)) {
>> - virReportError(VIR_ERR_XML_ERROR,
>> - _("Parent attribute '%s' does not match parent '%s' "
>> - "determined for the '%s' wwnn/wwpn lookup."),
>> - parent_name, vhba_parent, name);
>> + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> + goto cleanup;
>> +
>> + if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("host name '%s' is not properly formatted"), name);
>> goto cleanup;
>> }
>>
>> + /* If scsi_host_name is vport capable, then it's an HBA, thus compare
>> + * only against the parent_name; otherwise, as long as the scsi_host_name
>> + * path exists, then the scsi_host_name is a vHBA in which case we need
>> + * to compare against it's parent. */
>> + if (virVHBAIsVportCapable(NULL, host_num)) {
>
> I truly find it pointless to try to do any kind of parent validation on HBA
> device. Can a HBA device actually have a scsi_host parent? If not, then we
> should only validate whether the device given by its wwnn and wwpn exist (we
> already know that since we're here) is vport capable and then if the 'parent'
> attribute has been defined, then return an error about invalid configuration,
> rather than trying to compare parent names in that matter.
>
> Erik
>
vport capability is essentially the presence of "vport_create" in the
/sys/class/fc_host/host# or /sys/class/scsi_host/host# path.
An HBA has some sort of a PCI_* parent based on it's address.
For this particular bug, someone provided a parent and it was wrong, so
that's what's being checked.
Although I agree the insanity of the checks for someone providing wrong
data is annoying; however, if not checked then it was possible to create
a storage pool backed to the HBA defined by the wwnn/wwpn provided;
however, when it comes time to destroy it, the code ended up calling the
VPORT_DELETE and obviously failing, causing the command to fail to
remove the pool. And there was no way to remove the pool with the
invalid parent name. So at startup, let's make sure if the parent name
is provided that it's correct.
These start checks essentially mimic the destroy tests. One could argue
though that the failure paths in shutdown for virSCSIHostGetNumber and
virNodeDeviceGetParentName should never happen, but assuming that is
paramount to someone finding a way to make it happen.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.