Re: [libvirt] [PATCH 04/11] nodedev: Add the ability to create vHBA by parent wwnn/wwpn or fabric_wwn
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
[...]