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'/>

So add a check prior to the checkParent call to validate that
the provided wwnn/wwpn resolves to a vHBA and not an HBA.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---

 v2: https://www.redhat.com/archives/libvir-list/2017-July/msg01019.html

 Changes since v2, from review, simplify logic even more.


 docs/formatstorage.html.in         | 27 +++++++++++++++------------
 src/storage/storage_backend_scsi.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 12 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..575e6a6 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -211,6 +211,33 @@ getAdapterName(virStorageAdapterPtr adapter)
 }
 
 
+/**
+ * @name: Name from a wwnn/wwpn lookup
+ *
+ * Validate that the @name fetched from the wwnn/wwpn is a vHBA
+ * and not an HBA as that should be a configuration error. It's only
+ * possible to use an existing wwnn/wwpn of a vHBA because that's
+ * what someone would have created using the node device create via XML
+ * functionality. Using the HBA "just because" it has a wwnn/wwpn and
+ * the characteristics of a vHBA is just not valid
+ *
+ * Returns true if the @name is OK, false on error
+ */
+static bool
+checkName(const char *name)
+{
+    unsigned int host_num;
+
+    if (virSCSIHostGetNumber(name, &host_num) &&
+        virVHBAIsVportCapable(NULL, host_num))
+        return true;
+
+    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                   _("the wwnn/wwpn for '%s' are assigned to an HBA"), name);
+    return false;
+}
+
+
 /*
  * 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.
@@ -288,6 +315,9 @@ createVport(virConnectPtr conn,
      * this pool and we don't have to create the vHBA
      */
     if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+        if (!(checkName(name)))
+            goto cleanup;
+
         /* 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))
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> +/**
> + * @name: Name from a wwnn/wwpn lookup
> + *
> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA
> + * and not an HBA as that should be a configuration error. It's only
> + * possible to use an existing wwnn/wwpn of a vHBA because that's
> + * what someone would have created using the node device create via XML
> + * functionality. Using the HBA "just because" it has a wwnn/wwpn and
> + * the characteristics of a vHBA is just not valid
> + *
> + * Returns true if the @name is OK, false on error
> + */
> +static bool
> +checkName(const char *name)
> +{
> +    unsigned int host_num;
> +
> +    if (virSCSIHostGetNumber(name, &host_num) &&

I expressed my doubts about ^this if it fails...I was wrong, if that fails,
we've got other problems, IOW virVHBAGetHostByWWN always must feed us a valid
name, since it's fetching it from the filesystem, so we should not have any
problem parsing it, thus extracting @host_num from it.

So, I'm sorry I didn't see it right away, ACK.

Erik

> +        virVHBAIsVportCapable(NULL, host_num))
> +        return true;
> +
> +    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                   _("the wwnn/wwpn for '%s' are assigned to an HBA"), name);
> +    return false;
> +}
> +
> +
>  /*
>   * 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.
> @@ -288,6 +315,9 @@ createVport(virConnectPtr conn,
>       * this pool and we don't have to create the vHBA
>       */
>      if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> +        if (!(checkName(name)))
> +            goto cleanup;
> +
>          /* 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))
> --
> 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