Disallow providing the wwnn/wwpn of the HBA in the adapter XML:
<adapter type='fc_host' [parent='scsi_hostN'] wwnn='HBA_wwnn'
wwpn='HBA_wwpn'/>
This should be considered a configuration error since a vHBA
would not be created. In order to use the HBA as the backing the
following XML should be used:
<adapter type='scsi_host' name='scsi_hostN'/>
This also alters the caller such that the @parent_name param
into checkParent can be NULL so as to confirm that at least
the provided wwnn/wwpn found a vHBA instead of an HBA.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
docs/formatstorage.html.in | 27 +++++++++++++----------
src/storage/storage_backend_scsi.c | 45 ++++++++++++++++++++++++++++----------
2 files changed, 49 insertions(+), 23 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 4946ddf..27578e8 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -207,18 +207,21 @@
</dl>
<dl>
<dt><code>wwnn</code> and <code>wwpn</code></dt>
- <dd>The "World Wide Node Name" (<code>wwnn</code>) and "World Wide
- Port Name" (<code>wwpn</code>) are used by the "fc_host" adapter
- to uniquely identify the device in the Fibre Channel storage fabric
- (the device can be either a HBA or vHBA). Both wwnn and wwpn should
- be specified. Use the command 'virsh nodedev-dumpxml' to determine
- how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and
- wwpn have very specific numerical format requirements based on the
- hypervisor being used, thus care should be taken if you decide to
- generate your own to follow the standards; otherwise, the pool
- will fail to start with an opaque error message indicating failure
- to write to the vport_create file during vport create/delete due
- to "No such file or directory".
+ <dd>The required "World Wide Node Name" (<code>wwnn</code>) and
+ "World Wide Port Name" (<code>wwpn</code>) are used by the
+ "fc_host" adapter to uniquely identify the vHBA device in the
+ Fibre Channel storage fabric. If the vHBA device already exists
+ as a Node Device, then libvirt will use it; otherwise, the vHBA
+ will be created using the provided values. It is considered a
+ configuration error use the values from the HBA as those would
+ be for a "scsi_host" <code>type</code> pool instead. The
+ <code>wwnn</code> and <code>wwpn</code> have very specific
+ format requirements based on the hypervisor being used, thus
+ care should be taken if you decide to generate your own to
+ follow the standards; otherwise, the pool will fail to start
+ with an opaque error message indicating failure to write to
+ the vport_create file during vport create/delete due to
+ "No such file or directory".
<span class="since">Since 1.0.4</span>
</dd>
</dl>
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index af12889..c802738 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -231,22 +231,47 @@ checkParent(virConnectPtr conn,
if (!conn)
return true;
- if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("parent '%s' is not properly formatted"),
- parent_name);
+ /* If there's a parent_name, then make sure it's valid */
+ if (parent_name) {
+ if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' is not properly formatted"),
+ parent_name);
+ goto cleanup;
+ }
+
+ if (!virVHBAPathExists(NULL, host_num)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("parent '%s' is not an fc_host for the wwnn/wwpn"),
+ parent_name);
+ goto cleanup;
+ }
+ }
+
+ 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 (!virVHBAPathExists(NULL, host_num)) {
+ /* If scsi_host_name is vport capable, then it's an HBA. This is
+ * a configuration error as the wwnn/wwpn should only be for a vHBA */
+ if (virVHBAIsVportCapable(NULL, host_num)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("parent '%s' is not an fc_host for the wwnn/wwpn"),
- parent_name);
+ _("the wwnn/wwpn for '%s' are assigned to an HBA"),
+ scsi_host_name);
goto cleanup;
}
- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+ /* No parent name, then no need to get/compare against vhba_parent */
+ if (!parent_name) {
+ retval = true;
goto cleanup;
+ }
if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
goto cleanup;
@@ -288,9 +313,7 @@ createVport(virConnectPtr conn,
* this pool and we don't have to create the vHBA
*/
if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
- /* If a parent was provided, let's make sure the 'name' we've
- * retrieved has the same parent. If not this will cause failure. */
- if (!fchost->parent || checkParent(conn, name, fchost->parent))
+ if (checkParent(conn, name, fchost->parent))
ret = 0;
goto cleanup;
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 20, 2017 at 03:48:49PM -0400, John Ferlan wrote:
> Disallow providing the wwnn/wwpn of the HBA in the adapter XML:
>
> <adapter type='fc_host' [parent='scsi_hostN'] wwnn='HBA_wwnn'
> wwpn='HBA_wwpn'/>
>
> This should be considered a configuration error since a vHBA
> would not be created. In order to use the HBA as the backing the
> following XML should be used:
>
> <adapter type='scsi_host' name='scsi_hostN'/>
>
> This also alters the caller such that the @parent_name param
> into checkParent can be NULL so as to confirm that at least
> the provided wwnn/wwpn found a vHBA instead of an HBA.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> docs/formatstorage.html.in | 27 +++++++++++++----------
> src/storage/storage_backend_scsi.c | 45 ++++++++++++++++++++++++++++----------
> 2 files changed, 49 insertions(+), 23 deletions(-)
>
> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> index 4946ddf..27578e8 100644
> --- a/docs/formatstorage.html.in
> +++ b/docs/formatstorage.html.in
> @@ -207,18 +207,21 @@
> </dl>
> <dl>
> <dt><code>wwnn</code> and <code>wwpn</code></dt>
> - <dd>The "World Wide Node Name" (<code>wwnn</code>) and "World Wide
> - Port Name" (<code>wwpn</code>) are used by the "fc_host" adapter
> - to uniquely identify the device in the Fibre Channel storage fabric
> - (the device can be either a HBA or vHBA). Both wwnn and wwpn should
> - be specified. Use the command 'virsh nodedev-dumpxml' to determine
> - how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and
> - wwpn have very specific numerical format requirements based on the
> - hypervisor being used, thus care should be taken if you decide to
> - generate your own to follow the standards; otherwise, the pool
> - will fail to start with an opaque error message indicating failure
> - to write to the vport_create file during vport create/delete due
> - to "No such file or directory".
> + <dd>The required "World Wide Node Name" (<code>wwnn</code>) and
> + "World Wide Port Name" (<code>wwpn</code>) are used by the
> + "fc_host" adapter to uniquely identify the vHBA device in the
> + Fibre Channel storage fabric. If the vHBA device already exists
> + as a Node Device, then libvirt will use it; otherwise, the vHBA
> + will be created using the provided values. It is considered a
> + configuration error use the values from the HBA as those would
> + be for a "scsi_host" <code>type</code> pool instead. The
> + <code>wwnn</code> and <code>wwpn</code> have very specific
> + format requirements based on the hypervisor being used, thus
> + care should be taken if you decide to generate your own to
> + follow the standards; otherwise, the pool will fail to start
> + with an opaque error message indicating failure to write to
> + the vport_create file during vport create/delete due to
> + "No such file or directory".
> <span class="since">Since 1.0.4</span>
> </dd>
> </dl>
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index af12889..c802738 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -231,22 +231,47 @@ checkParent(virConnectPtr conn,
> if (!conn)
> return true;
>
> - if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("parent '%s' is not properly formatted"),
> - parent_name);
> + /* If there's a parent_name, then make sure it's valid */
> + if (parent_name) {
> + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("parent '%s' is not properly formatted"),
> + parent_name);
> + goto cleanup;
> + }
> +
> + if (!virVHBAPathExists(NULL, host_num)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("parent '%s' is not an fc_host for the wwnn/wwpn"),
> + parent_name);
> + goto cleanup;
> + }
> + }
^Here you're handling the device's parent - it's existence
> +
> + 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 (!virVHBAPathExists(NULL, host_num)) {
> + /* If scsi_host_name is vport capable, then it's an HBA. This is
> + * a configuration error as the wwnn/wwpn should only be for a vHBA */
> + if (virVHBAIsVportCapable(NULL, host_num)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("parent '%s' is not an fc_host for the wwnn/wwpn"),
> - parent_name);
> + _("the wwnn/wwpn for '%s' are assigned to an HBA"),
> + scsi_host_name);
> goto cleanup;
> }
^Here you're handling the device itself.
>
> - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> + /* No parent name, then no need to get/compare against vhba_parent */
> + if (!parent_name) {
> + retval = true;
> goto cleanup;
> + }
>
And ^here you're again handling the device's parent - it's !existence and
perform some further parent device validation in the already existing hunk
below. Would it make more sense to first verify the device whether it's a vHBA,
if not then throw an error and then, if @parent_name wasn't supplied, return
successfully otherwise do both the parent existence check and validation, both
in a large "else" block. The checks make sense and work as described, I just
find it more understandable - the logic would be IMHO more continuous, as
opposed "scattered" (is that even the right term for this? But I think you get
the picture...)
if (scsi_host_name is vport capable)
# it's an HBA
error
return failure
if (!parent_name)
# no parent to validate
return success
else
if (parent doesn't exist)
error - not an fc_host
return failure
if (provided_parent != actual_parent)
error - invalid parent
return failure
return success
Erik.
> if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> goto cleanup;
> @@ -288,9 +313,7 @@ createVport(virConnectPtr conn,
> * this pool and we don't have to create the vHBA
> */
> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> - /* If a parent was provided, let's make sure the 'name' we've
> - * retrieved has the same parent. If not this will cause failure. */
> - if (!fchost->parent || checkParent(conn, name, fchost->parent))
> + if (checkParent(conn, name, fchost->parent))
> ret = 0;
>
> goto cleanup;
> --
> 2.9.4
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/24/2017 11:00 AM, Erik Skultety wrote:
> On Thu, Jul 20, 2017 at 03:48:49PM -0400, John Ferlan wrote:
>> Disallow providing the wwnn/wwpn of the HBA in the adapter XML:
>>
>> <adapter type='fc_host' [parent='scsi_hostN'] wwnn='HBA_wwnn'
>> wwpn='HBA_wwpn'/>
>>
>> This should be considered a configuration error since a vHBA
>> would not be created. In order to use the HBA as the backing the
>> following XML should be used:
>>
>> <adapter type='scsi_host' name='scsi_hostN'/>
>>
>> This also alters the caller such that the @parent_name param
>> into checkParent can be NULL so as to confirm that at least
>> the provided wwnn/wwpn found a vHBA instead of an HBA.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> docs/formatstorage.html.in | 27 +++++++++++++----------
>> src/storage/storage_backend_scsi.c | 45 ++++++++++++++++++++++++++++----------
>> 2 files changed, 49 insertions(+), 23 deletions(-)
>>
>> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
>> index 4946ddf..27578e8 100644
>> --- a/docs/formatstorage.html.in
>> +++ b/docs/formatstorage.html.in
>> @@ -207,18 +207,21 @@
>> </dl>
>> <dl>
>> <dt><code>wwnn</code> and <code>wwpn</code></dt>
>> - <dd>The "World Wide Node Name" (<code>wwnn</code>) and "World Wide
>> - Port Name" (<code>wwpn</code>) are used by the "fc_host" adapter
>> - to uniquely identify the device in the Fibre Channel storage fabric
>> - (the device can be either a HBA or vHBA). Both wwnn and wwpn should
>> - be specified. Use the command 'virsh nodedev-dumpxml' to determine
>> - how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and
>> - wwpn have very specific numerical format requirements based on the
>> - hypervisor being used, thus care should be taken if you decide to
>> - generate your own to follow the standards; otherwise, the pool
>> - will fail to start with an opaque error message indicating failure
>> - to write to the vport_create file during vport create/delete due
>> - to "No such file or directory".
>> + <dd>The required "World Wide Node Name" (<code>wwnn</code>) and
>> + "World Wide Port Name" (<code>wwpn</code>) are used by the
>> + "fc_host" adapter to uniquely identify the vHBA device in the
>> + Fibre Channel storage fabric. If the vHBA device already exists
>> + as a Node Device, then libvirt will use it; otherwise, the vHBA
>> + will be created using the provided values. It is considered a
>> + configuration error use the values from the HBA as those would
>> + be for a "scsi_host" <code>type</code> pool instead. The
>> + <code>wwnn</code> and <code>wwpn</code> have very specific
>> + format requirements based on the hypervisor being used, thus
>> + care should be taken if you decide to generate your own to
>> + follow the standards; otherwise, the pool will fail to start
>> + with an opaque error message indicating failure to write to
>> + the vport_create file during vport create/delete due to
>> + "No such file or directory".
>> <span class="since">Since 1.0.4</span>
>> </dd>
>> </dl>
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index af12889..c802738 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -231,22 +231,47 @@ checkParent(virConnectPtr conn,
>> if (!conn)
>> return true;
>>
>> - if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
>> - virReportError(VIR_ERR_XML_ERROR,
>> - _("parent '%s' is not properly formatted"),
>> - parent_name);
>> + /* If there's a parent_name, then make sure it's valid */
>> + if (parent_name) {
>> + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
>> + virReportError(VIR_ERR_XML_ERROR,
>> + _("parent '%s' is not properly formatted"),
>> + parent_name);
>> + goto cleanup;
>> + }
>> +
>> + if (!virVHBAPathExists(NULL, host_num)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("parent '%s' is not an fc_host for the wwnn/wwpn"),
>> + parent_name);
>> + goto cleanup;
>> + }
>> + }
>
> ^Here you're handling the device's parent - it's existence
>
>> +
>> + 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 (!virVHBAPathExists(NULL, host_num)) {
>> + /* If scsi_host_name is vport capable, then it's an HBA. This is
>> + * a configuration error as the wwnn/wwpn should only be for a vHBA */
>> + if (virVHBAIsVportCapable(NULL, host_num)) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> - _("parent '%s' is not an fc_host for the wwnn/wwpn"),
>> - parent_name);
>> + _("the wwnn/wwpn for '%s' are assigned to an HBA"),
>> + scsi_host_name);
>> goto cleanup;
>> }
>
> ^Here you're handling the device itself.
>
>>
>> - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> + /* No parent name, then no need to get/compare against vhba_parent */
>> + if (!parent_name) {
>> + retval = true;
>> goto cleanup;
>> + }
>>
>
> And ^here you're again handling the device's parent - it's !existence and
> perform some further parent device validation in the already existing hunk
> below. Would it make more sense to first verify the device whether it's a vHBA,
> if not then throw an error and then, if @parent_name wasn't supplied, return
> successfully otherwise do both the parent existence check and validation, both
> in a large "else" block. The checks make sense and work as described, I just
> find it more understandable - the logic would be IMHO more continuous, as
> opposed "scattered" (is that even the right term for this? But I think you get
> the picture...)
True - I was also trying to keep diffs to a minimum while also trying to
preserve both sets of error checking while considering patch 3 ordering.
Patch 3 adds the parent_name check above "scsi_host/vhba_parent" because
I didn't want to stick the logic between the virAsprintf and
GetParentName call and it felt awkward to needlessly get vhba_parent
without first checking if @parent_name was going to fail.
But you're right I can do better on this and stop making checkParent be
the slave to too many masters. I'll push the first 3 and repost
something a bit different that I think will be more palatable.
Thanks -
John
FWIW: I did originally have the logic swapped like shown below, but the
patch 4 diffs got really ugly...
>
> if (scsi_host_name is vport capable)
> # it's an HBA
> error
> return failure
>
> if (!parent_name)
> # no parent to validate
> return success
> else
> if (parent doesn't exist)
> error - not an fc_host
> return failure
>
> if (provided_parent != actual_parent)
> error - invalid parent
> return failure
>
> return success
>
> Erik.
>
>> if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
>> goto cleanup;
>> @@ -288,9 +313,7 @@ createVport(virConnectPtr conn,
>> * this pool and we don't have to create the vHBA
>> */
>> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
>> - /* If a parent was provided, let's make sure the 'name' we've
>> - * retrieved has the same parent. If not this will cause failure. */
>> - if (!fchost->parent || checkParent(conn, name, fchost->parent))
>> + if (checkParent(conn, name, fchost->parent))
>> ret = 0;
>>
>> goto cleanup;
>> --
>> 2.9.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.