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 <fiuczy@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
