On 12/13/2016 08:30 PM, John Ferlan wrote:
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).
In virReadFCHost the method virFileReadAll is used to read the file fabric_name. This method triggers virReportSystemError(errno, _("Failed to open file '%s'"), path); if the file cannot be opened. As a result virReadFCHost returns NULL into nodeDeviceSysfsGetSCSIHostCaps which causes cleanup to be called resulting in the removal of the fc_host capability. I will post a patch for fixing this issue.
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.
My point is that your patch starts using the optional fabric_name as (optional) input parameter in the API. It seems that the wwnn/wwpn pair provides the most specific search option anyway and always works where as the fabric_wwn might work only for some. Anyway, you are right, that it can be done if it is all optional and the fix for the fc_host capability is mostly independent of this series especially since the fabric_wwn tag is only written out and never read in from xml anyway!
John
[...]
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294