[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 linux vnet ibm com>
> Signed-off-by: Bjoern Walk <bwalk linux vnet ibm com>
> ---
>  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 redhat com>

I can make the suggested changes if you'd like or you can send a v2 or a
squash diff...  Let me know

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]