Re: [libvirt] [PATCH 6/7] node_device: introduce new capability FC_RPORT

On 05/22/2017 02:38 AM, Bjoern Walk wrote: > Similar to scsi_host and fc_host, there is a relation between a > scsi_target and its transport specific fc_remote_port. Let's expose this > relation and relevant information behind it. > > An example for a virsh nodedev-dumpxml: > > virsh # nodedev-dumpxml scsi_target0_0_0 > <device> > <name>scsi_target0_0_0</name> > <path>/sys/devices/[...]/host0/rport-0:0-0/target0:0:0</path> > <parent>scsi_host0</parent> > <capability type='scsi_target'> > <target>target0:0:0</target> > <capability type='fc_remote_port'> > <rport>rport-0:0-0</rport> > <wwpn>0x9d73bc45f0e21a86</wwpn> > </capability> > </capability> > </device> > > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; > --- > docs/schemas/nodedev.rng | 20 ++++++++ > src/conf/node_device_conf.c | 75 +++++++++++++++++++++++++++- > src/conf/node_device_conf.h | 7 +++ > src/node_device/node_device_driver.c | 5 +- > src/node_device/node_device_linux_sysfs.c | 56 +++++++++++++++++++++ > src/node_device/node_device_linux_sysfs.h | 2 + > src/node_device/node_device_udev.c | 4 +- > tests/nodedevschemadata/scsi_target1_0_0.xml | 12 +++++ > tests/nodedevxml2xmltest.c | 1 + > 9 files changed, 178 insertions(+), 4 deletions(-) > create mode 100644 tests/nodedevschemadata/scsi_target1_0_0.xml > > diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng > index 87bfb0c28..5bcf31787 100644 > --- a/docs/schemas/nodedev.rng > +++ b/docs/schemas/nodedev.rng > @@ -458,6 +458,20 @@ > </optional> > </define> > > + <define name='capsfcrport'> > + <attribute name='type'> > + <value>fc_remote_port</value> > + </attribute> > + > + <element name='rport'> > + <text/> > + </element> > + > + <element name='wwpn'> > + <ref name='wwn'/> NB: 'wwn' is a string... > + </element> > + </define> > + > <define name='capscsitarget'> > <attribute name='type'> > <value>scsi_target</value> > @@ -466,6 +480,12 @@ > <element name='target'> > <text/> > </element> > + > + <optional> > + <element name='capability'> > + <ref name='capsfcrport'/> > + </element> > + </optional> > </define> > > <define name='capscsi'> > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 28d4907f5..ed003d37b 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -563,6 +563,16 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) > case VIR_NODE_DEV_CAP_SCSI_TARGET: > virBufferEscapeString(&buf, "<target>%s</target>\n", > data->scsi_target.name); > + if (data->scsi_target.flags & VIR_NODE_DEV_CAP_FLAG_FC_RPORT) { > + virBufferAddLit(&buf, "<capability type='fc_remote_port'>\n"); > + virBufferAdjustIndent(&buf, 2); > + virBufferAsprintf(&buf, "<rport>%s</rport>\n", > + data->scsi_target.rport); > + virBufferAsprintf(&buf, "<wwpn>0x%llx</wwpn>\n", > + data->scsi_target.wwpn); If you don't convert to a number, then you don't need to perform this conversion as it's just a %s print. > + virBufferAdjustIndent(&buf, -2); > + virBufferAddLit(&buf, "</capability>\n"); > + } > break; > case VIR_NODE_DEV_CAP_SCSI: > virNodeDeviceCapSCSIDefFormat(&buf, data); > @@ -932,8 +942,10 @@ virNodeDevCapSCSITargetParseXML(xmlXPathContextPtr ctxt, > xmlNodePtr node, > virNodeDevCapSCSITargetPtr scsi_target) > { > - xmlNodePtr orignode; > - int ret = -1; > + xmlNodePtr orignode, *nodes = NULL; > + int ret = -1, n = 0; > + size_t i; > + char *type = NULL, *wwpn = NULL; > > orignode = ctxt->node; > ctxt->node = node; > @@ -946,10 +958,68 @@ virNodeDevCapSCSITargetParseXML(xmlXPathContextPtr ctxt, > goto out; > } > > + if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) > + goto out; > + > + for (i = 0; i < n; ++i) { > + type = virXMLPropString(nodes[i], "type"); > + > + if (!type) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("missing type for SCSI target capability for '%s'"), > + def->name); > + goto out; > + } > + > + if (STREQ(type, "fc_remote_port")) { > + xmlNodePtr orignode2; > + > + scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT; > + > + orignode2 = ctxt->node; > + ctxt->node = nodes[i]; Like the virNodeDevCapSCSIHostParseXML I assume this only parses the first 'fc_remote_port'... Trying to think whether a VIR_FREE would be necessary here (and perhaps from SCSIHost code just in case there were more than one <rport> or <wwpn> provided...

> + > + if (virNodeDevCapsDefParseString("string(./rport[1])", > + ctxt, > + &scsi_target->rport) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("missing rport name for '%s'"), def->name); > + goto out; > + } > + > + if (virNodeDevCapsDefParseString("string(./wwpn[1])", > + ctxt, &wwpn) < 0) { If you don't do the conversion, this could just be a straight parse into scsi_target->wwpn > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("missing wwpn identifier for '%s'"), > + def->name); > + goto out; > + } > + > + if (virStrToLong_ullp(wwpn, NULL, 16, &scsi_target->wwpn) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("invalid wwpn identifier '%s' for '%s'"), > + wwpn, def->name); Why even both with this conversion?

> + goto out; > + } > + > + ctxt->node = orignode2; > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unknown SCSI target capability type '%s' for '%s'"), > + type, def->name); > + goto out; > + } > + > + VIR_FREE(type); > + VIR_FREE(wwpn); > + } > + > ret = 0; > > out: > ctxt->node = orignode; > + VIR_FREE(type); > + VIR_FREE(wwpn); If you don't do the conversion, then this VIR_FREE is unnecessary. Coverity complained that nodes was leaked, so I added a VIR_FREE(nodes); here. > return ret; > } > > @@ -2132,6 +2202,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) > break; > case VIR_NODE_DEV_CAP_SCSI_TARGET: > VIR_FREE(data->scsi_target.name); > + VIR_FREE(data->scsi_target.rport); Without the conversion there's a VIR_FREE here for wwpn > break; > case VIR_NODE_DEV_CAP_SCSI: > VIR_FREE(data->scsi.type); > diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h > index 285841edc..f5267efda 100644 > --- a/src/conf/node_device_conf.h > +++ b/src/conf/node_device_conf.h > @@ -93,6 +93,10 @@ typedef enum { > } virNodeDevSCSIHostCapFlags; > > typedef enum { > + VIR_NODE_DEV_CAP_FLAG_FC_RPORT = (1 << 0), > +} virNodeDevSCSITargetCapsFlags; > + > +typedef enum { > VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION = (1 << 0), > VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION = (1 << 1), > VIR_NODE_DEV_CAP_FLAG_PCIE = (1 << 2), > @@ -227,6 +231,9 @@ typedef struct _virNodeDevCapSCSITarget virNodeDevCapSCSITarget; > typedef virNodeDevCapSCSITarget *virNodeDevCapSCSITargetPtr; > struct _virNodeDevCapSCSITarget { > char *name; > + unsigned int flags; /* enum virNodeDevSCSITargetCapsFlags */ > + char *rport; > + unsigned long long wwpn; and this would be a char *wwpn; > }; > > typedef struct _virNodeDevCapSCSI virNodeDevCapSCSI; > diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c > index 286af26bc..acb8b89af 100644 > --- a/src/node_device/node_device_driver.c > +++ b/src/node_device/node_device_driver.c > @@ -56,6 +56,10 @@ static int update_caps(virNodeDeviceObjPtr dev) > case VIR_NODE_DEV_CAP_SCSI_HOST: > nodeDeviceSysfsGetSCSIHostCaps(&cap->data.scsi_host); > break; > + case VIR_NODE_DEV_CAP_SCSI_TARGET: > + nodeDeviceSysfsGetSCSITargetCaps(dev->def->sysfs_path, > + &cap->data.scsi_target); > + break; > case VIR_NODE_DEV_CAP_NET: > if (virNetDevGetLinkInfo(cap->data.net.ifname, &cap->data.net.lnk) < 0) > return -1; > @@ -76,7 +80,6 @@ static int update_caps(virNodeDeviceObjPtr dev) > case VIR_NODE_DEV_CAP_SYSTEM: > case VIR_NODE_DEV_CAP_USB_DEV: > case VIR_NODE_DEV_CAP_USB_INTERFACE: > - case VIR_NODE_DEV_CAP_SCSI_TARGET: > case VIR_NODE_DEV_CAP_SCSI: > case VIR_NODE_DEV_CAP_STORAGE: > case VIR_NODE_DEV_CAP_FC_HOST: > diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c > index 1b7aa9435..90452fb29 100644 > --- a/src/node_device/node_device_linux_sysfs.c > +++ b/src/node_device/node_device_linux_sysfs.c > @@ -26,11 +26,13 @@ > #include <sys/stat.h> > #include <stdlib.h> > > +#include "dirname.h" > #include "node_device_driver.h" > #include "node_device_hal.h" > #include "node_device_linux_sysfs.h" > #include "virerror.h" > #include "viralloc.h" > +#include "virfcp.h" > #include "virlog.h" > #include "virfile.h" > #include "virscsihost.h" > @@ -124,6 +126,54 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host) > } > > > +int > +nodeDeviceSysfsGetSCSITargetCaps(const char *sysfsPath, > + virNodeDevCapSCSITargetPtr scsi_target) > +{ > + int ret = -1; > + char *dir = NULL, *rport = NULL, *wwpn = NULL; > + > + VIR_DEBUG("Checking if '%s' is an FC remote port", scsi_target->name); > + > + /* /sys/devices/[...]/host0/rport-0:0-0/target0:0:0 -> rport-0:0-0 */ > + if (!(dir = mdir_name(sysfsPath))) > + return -1; > + > + if (VIR_STRDUP(rport, last_component(dir)) < 0) > + goto cleanup; > + > + if (!virFCIsCapableRport(rport)) > + goto cleanup; > + > + VIR_FREE(scsi_target->rport); > + scsi_target->rport = rport; VIR_STEAL_PTR(scsi_target->rport, rport);

> + > + if (virFCReadRportValue(scsi_target->rport, "port_name", &wwpn) < 0) { > + VIR_WARN("Failed to read port_name for '%s'", scsi_target->rport); > + goto cleanup; > + } > + > + if (virStrToLong_ullp(wwpn, NULL, 16, &scsi_target->wwpn) < 0) { > + VIR_WARN("Failed to parse value of port_name '%s'", wwpn); > + goto cleanup; > + } > + > + scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT; > + ret = 0; > + > + cleanup: > + if (ret < 0) { > + VIR_FREE(scsi_target->rport); > + scsi_target->wwpn = 0; > + scsi_target->flags &= ~VIR_NODE_DEV_CAP_FLAG_FC_RPORT; > + } Coverity complained that rport could be leaked, so I added the VIR_STEAL_PTR above and a VIR_FREE(rport); here. > + VIR_FREE(dir); > + VIR_FREE(wwpn); > + > + return ret; > +} > + > + > static int > nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath, > virNodeDevCapPCIDevPtr pci_dev) > @@ -230,6 +280,12 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host ATTRIBUTE_UNUS > return -1; > } > Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt;

I can make the suggested changes if you'd like or you can send a v2 or a squash diff... Let me know John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list