On 01/13/2017 06:56 AM, Boris Fiuczynski wrote:
> fabric_name is one of many fc_host attributes in Linux that is optional
> and left to the low-level driver to decide if it is implemented.
> The zfcp device driver does not provide a fabric name for an fcp host.
> 
> This patch removes the requirement for a fabric name by making it optional.
> 
> Signed-off-by: Boris Fiuczynski <fiu...@linux.vnet.ibm.com>
> ---
>  docs/formatnode.html.in                   |  2 +-
>  docs/news.xml                             | 12 +++++++++
>  docs/schemas/nodedev.rng                  |  8 +++---
>  src/libvirt_private.syms                  |  1 +
>  src/node_device/node_device_linux_sysfs.c |  6 ++---
>  src/util/virutil.c                        | 23 ++++++++++++++++
>  src/util/virutil.h                        |  5 ++++
>  tests/fchostdata/fc_host/host6/node_name  |  1 +
>  tests/fchostdata/fc_host/host6/port_name  |  1 +
>  tests/fchostdata/fc_host/host6/port_state |  1 +
>  tests/fchosttest.c                        | 44 
> ++++++++++++++++++++++++++++---
>  11 files changed, 94 insertions(+), 10 deletions(-)
>  create mode 100644 tests/fchostdata/fc_host/host6/node_name
>  create mode 100644 tests/fchostdata/fc_host/host6/port_name
>  create mode 100644 tests/fchostdata/fc_host/host6/port_state
> 
> diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
> index e7b1140..f8d0e12 100644
> --- a/docs/formatnode.html.in
> +++ b/docs/formatnode.html.in
> @@ -254,7 +254,7 @@
>                  number of vport in use. <code>max_vports</code> shows the
>                  maximum vports the HBA supports. "fc_host" implies following
>                  sub-elements: <code>wwnn</code>, <code>wwpn</code>, and
> -                <code>fabric_wwn</code>.
> +                optionally <code>fabric_wwn</code>.
>                </dd>
>              </dl>
>            </dd>
> diff --git a/docs/news.xml b/docs/news.xml
> index 50c3b3e..a645953 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -144,6 +144,18 @@
>      <section title="Bug fixes">
>        <change>
>          <summary>
> +          nodedev: Fabric name must not be required for fc_host capability
> +        </summary>
> +        <description>
> +          fabric_name is one of many fc_host attributes in Linux that is
> +          optional and left to the low-level driver to decide if it is
> +          implemented. For example the zfcp device driver does not provide a
> +          fabric name for an fcp host. The requirement for the existence of
> +          a fabric name has been removed by making it optional.
> +        </description>
> +      </change>
> +      <change>
> +        <summary>
>            qemu: Correct GetBlockInfo values
>          </summary>
>          <description>
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index 9c98402..b100a6e 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -367,9 +367,11 @@
>        <ref name='wwn'/>
>      </element>
>  
> -    <element name='fabric_wwn'>
> -      <ref name='wwn'/>
> -    </element>
> +    <optional>
> +      <element name='fabric_wwn'>
> +        <ref name='wwn'/>
> +      </element>
> +    </optional>
>    </define>
>  
>    <define name='capsvports'>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 70ed87b..43f460f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2717,6 +2717,7 @@ virParseOwnershipIds;
>  virParseVersionString;
>  virPipeReadUntilEOF;
>  virReadFCHost;
> +virReadFCHostOption;

I don't think the Option; API is necessary

>  virReadSCSIUniqueId;
>  virScaleInteger;
>  virSetBlocking;
> diff --git a/src/node_device/node_device_linux_sysfs.c 
> b/src/node_device/node_device_linux_sysfs.c
> index be99c41..1bb5b74 100644
> --- a/src/node_device/node_device_linux_sysfs.c
> +++ b/src/node_device/node_device_linux_sysfs.c
> @@ -72,10 +72,10 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
>          VIR_FREE(d->scsi_host.wwnn);
>          VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
>  
> -        if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) {
> -            VIR_WARN("Failed to read fabric WWN for host%d",
> +        if (!(tmp = virReadFCHostOption(NULL, d->scsi_host.host,
> +                                        "fabric_name", false))) {
> +            VIR_INFO("Optional fabric WWN for host%d not available",
>                       d->scsi_host.host);
> -            goto cleanup;
>          }

Just make this:

    if ((tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) {
        VIR_FREE(d->scsi_host.fabric_wwn);
        VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
    }

and remove the "need" for a virReadFCHostOption function...

>          VIR_FREE(d->scsi_host.fabric_wwn);
>          VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index aeaa7f9..5bfbf37 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2019,6 +2019,26 @@ virReadFCHost(const char *sysfs_prefix,
>                int host,
>                const char *entry)
>  {
> +   return virReadFCHostOption(sysfs_prefix, host, entry, true);
> +}
> +
> +/* virReadFCHostOption:
> + * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
> + * @host: Host number, E.g. 5 of "fc_host/host5"
> + * @entry: Name of the sysfs entry to read
> + * @required: If true reports error when entry not found else no error
> + *
> + * Read the value of sysfs "fc_host" entry.
> + *
> + * Returns result as a string on success, caller must free @result after
> + * Otherwise returns NULL.
> + */
> +char *
> +virReadFCHostOption(const char *sysfs_prefix,
> +                    int host,
> +                    const char *entry,
> +                    bool required)
> +{
>      char *sysfs_path = NULL;
>      char *p = NULL;
>      char *buf = NULL;
> @@ -2029,6 +2049,9 @@ virReadFCHost(const char *sysfs_prefix,
>                      host, entry) < 0)
>          goto cleanup;
>  
> +    if (!required && !virFileExists(sysfs_path))
> +        goto cleanup;
> +

As a "first"/separate patch, modify virReadFCHost to perform the "if
!virFileExists(sysfs_path)" anyway since that's a good check and will
avoid the error message when/if the open fails in virFileReadAll. In
that same patch - feel free to modify the comment....

Since the callers of virReadFCHost "handle" the NULL return not having
an extraneous message when the file doesn't exist wouldn't be a bad
thing. There are VIR_WARN/DEBUG messages from callers anyway.

You'll also note most callers of nodeDeviceSysfsGetSCSIHostCaps ignore
the return status anyway (the old hal code being the only one that cares)...


>      if (virFileReadAll(sysfs_path, 1024, &buf) < 0)
>          goto cleanup;
>  
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 3fbd7b0..db86052 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -186,6 +186,11 @@ char *virReadFCHost(const char *sysfs_prefix,
>                      int host,
>                      const char *entry)
>      ATTRIBUTE_NONNULL(3);
> +char *virReadFCHostOption(const char *sysfs_prefix,
> +                          int host,
> +                          const char *entry,
> +                          bool required)
> +    ATTRIBUTE_NONNULL(3);
>  
>  bool virIsCapableFCHost(const char *sysfs_prefix, int host);
>  bool virIsCapableVport(const char *sysfs_prefix, int host);
> diff --git a/tests/fchostdata/fc_host/host6/node_name 
> b/tests/fchostdata/fc_host/host6/node_name
> new file mode 100644
> index 0000000..73a91bd
> --- /dev/null
> +++ b/tests/fchostdata/fc_host/host6/node_name
> @@ -0,0 +1 @@
> +0x2002001b32a9da4e
> diff --git a/tests/fchostdata/fc_host/host6/port_name 
> b/tests/fchostdata/fc_host/host6/port_name
> new file mode 100644
> index 0000000..f25abeb
> --- /dev/null
> +++ b/tests/fchostdata/fc_host/host6/port_name
> @@ -0,0 +1 @@
> +0x2102001b32a9da4e
> diff --git a/tests/fchostdata/fc_host/host6/port_state 
> b/tests/fchostdata/fc_host/host6/port_state
> new file mode 100644
> index 0000000..b73dd46
> --- /dev/null
> +++ b/tests/fchostdata/fc_host/host6/port_state
> @@ -0,0 +1 @@
> +Online
> diff --git a/tests/fchosttest.c b/tests/fchosttest.c
> index a08a2e8..01c20df 100644
> --- a/tests/fchosttest.c
> +++ b/tests/fchosttest.c
> @@ -29,13 +29,16 @@ static char *fchost_prefix;
>  
>  #define TEST_FC_HOST_PREFIX fchost_prefix
>  #define TEST_FC_HOST_NUM 5
> +#define TEST_FC_HOST_NUM_NO_FAB 6
>  
>  /* Test virIsCapableFCHost */
>  static int
>  test1(const void *data ATTRIBUTE_UNUSED)
>  {
>      if (virIsCapableFCHost(TEST_FC_HOST_PREFIX,
> -                           TEST_FC_HOST_NUM))
> +                           TEST_FC_HOST_NUM) &&
> +        virIsCapableFCHost(TEST_FC_HOST_PREFIX,
> +                           TEST_FC_HOST_NUM_NO_FAB))
>          return 0;
>  
>      return -1;
> @@ -76,8 +79,8 @@ test3(const void *data ATTRIBUTE_UNUSED)
>                                 "port_name")))
>          goto cleanup;
>  
> -    if (!(fabric_wwn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM,
> -                                     "fabric_name")))
> +    if (!(fabric_wwn = virReadFCHostOption(TEST_FC_HOST_PREFIX, 
> TEST_FC_HOST_NUM,
> +                                           "fabric_name", false)))

This hunk is thus unnecessary..

>          goto cleanup;
>  
>      if (!(max_vports = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM,
> @@ -148,6 +151,39 @@ test5(const void *data ATTRIBUTE_UNUSED)
>      return ret;
>  }
>  
> +/* Test virReadFCHost fabric name optional */
> +static int
> +test6(const void *data ATTRIBUTE_UNUSED)
> +{
> +    const char *expect_wwnn = "2002001b32a9da4e";
> +    const char *expect_wwpn = "2102001b32a9da4e";
> +    char *wwnn = NULL;
> +    char *wwpn = NULL;
> +    int ret = -1;
> +
> +    if (!(wwnn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM_NO_FAB,
> +                               "node_name")))
> +        return -1;
> +
> +    if (!(wwpn = virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM_NO_FAB,
> +                               "port_name")))
> +        goto cleanup;
> +
> +    if (virReadFCHostOption(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM_NO_FAB,

and this just changes to virReadFCHost w/o the false parameter

> +                            "fabric_name", false))
> +        goto cleanup;
> +
> +    if (STRNEQ(expect_wwnn, wwnn) ||
> +        STRNEQ(expect_wwpn, wwpn))
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(wwnn);
> +    VIR_FREE(wwpn);
> +    return ret;
> +}
> +
>  static int
>  mymain(void)
>  {
> @@ -169,6 +205,8 @@ mymain(void)
>          ret = -1;
>      if (virTestRun("test5", test5, NULL) < 0)
>          ret = -1;
> +    if (virTestRun("test6", test6, NULL) < 0)
> +        ret = -1;
>  
>   cleanup:
>      VIR_FREE(fchost_prefix);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to