Re: [libvirt] [PATCH v3 4/4] storage: Disallow usage of the HBA for a fc_host backing

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(a)redhat.com&gt; > --- > 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list