[libvirt] [PATCH 04/11] nodedev: Add the ability to create vHBA by parent wwnn/wwpn or fabric_wwn

[John Ferlan]

On 12/13/2016 10:58 AM, Boris Fiuczynski wrote:
> John,
> I have a concern regarding usage of the parent_fabric_name.
> As far I have been told there are a lot of fc_host attributes in Linux
> that are optional and left to the low-level driver to decide if
> implemented. fabric_name apparently happens to be one of these
> attributes and I know an architecture that has a driver (zfcp) that does
> not provide it.
> 
> Up until this patch the fabric_name was collected on the host during
> detection of the fc_host capability and provided during a nodedev dumpxml.

How does this patch change that?  All this patch does is allow one to
provide a "parent_wwnn/parent_wwpn" or "parent_fabric_name" for the
input XML to be passed to nodedev-create. So rather than just:

<device>
  <parent>scsi_host3</parent>
  <capability type='scsi_host'>
    <capability type='fc_host'>
    </capability>
  </capability>
</device>

One could provide:

<device>
  <name>vhba</name>
  <parent_wwnn>20000000c9848140</parent_wwnn>
  <parent_wwpn>10000000c9848140</parent_wwpn>
  <capability type='scsi_host'>
    <capability type='fc_host'>
    </capability>
  </capability>
</device>

or

<device>
  <name>vhba</name>
  <parent_fabric_wwn>2002000573de9a81</parent_fabric_wwn>
  <capability type='scsi_host'>
    <capability type='fc_host'>
    </capability>
  </capability>
</device>

for the 'virsh nodedev-create $FILE' command.

> I would like to propose making the existence of fabric_name option in
> the detection of the fc_host capability. In doing so it would also be
> required make the fabric_name xml tag optional in the xml of the nodedev
> dump. I am aware that this is a creating a slightly incompatible output
> but how else could that be fixed than making the tag optional? I can
> send a patch for this if you agree to the solution.

The biggest hurdle I face with NPIV/vHBA is that there are many
different options. I keep learning as I go on this stuff. It does seem
from what you're indicating that the fabric_name file just doesn't
exist. Although without having such a system available to me I have no
way of knowing for sure.

In any case, if "fabric_name" is not read, then 'scsi_host.fabric_wwn'
is left blank (see nodeDeviceSysfsGetSCSIHostCaps). There isn't an error
path that would cause a failure (just a debug message). If it is NULL,
then since the Format code uses virBufferEscapeString which shouldn't
format the XML for it. So given that, I suppose it's reasonable to
provide an "<optional>" tag for the RNG file (although similarly
wwnn/wwpn are also then optional). That seems reasonable (but unrelated
to this code).

Caveat - there is a udev bug/issue
(https://bugzilla.redhat.com/show_bug.cgi?id=1319544) where the
scsi_host for the vHBA is created before the files we look at have valid
data. It's one of those where the QE is doing repeated create/destroy
and udev hasn't really finished updating things before it sends the
create event.

> 
> The implications regarding the search by parent_fabric_wwn should be
> obvious.
> 

For the purpose of this patch set though, parent_fabric_wwn is a 'read
only' type value. Unless I messed up in the code, if it's not available
to the HBA/scsi_host on the host, then regardless of what's provided in
parent_fabric_wwn the parent won't be found by fabric_wwn/fabric_name. I
suppose I could add checks in virNodeDeviceDefParseXML that the provided
string "looks like" a WWN (eg if virValidateWWN call) as a crude form of
XML validation.


John

[...]