[libvirt] [PATCH] nodedev: Fabric name must not be required for fc_host capability
Martin Kletzander
mkletzan at redhat.com
Thu Jan 12 16:16:30 UTC 2017
On Wed, Jan 11, 2017 at 03:05:19PM +0100, 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 at linux.vnet.ibm.com>
>Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>---
> docs/formatnode.html.in | 2 +-
> docs/schemas/nodedev.rng | 8 +++---
> src/node_device/node_device_linux_sysfs.c | 3 +--
> src/util/virutil.c | 4 +--
> tests/fchostdata/fc_host/host7/node_name | 1 +
> tests/fchostdata/fc_host/host7/port_name | 1 +
> tests/fchostdata/fc_host/host7/port_state | 1 +
> tests/fchosttest.c | 44 ++++++++++++++++++++++++++++---
> 8 files changed, 53 insertions(+), 11 deletions(-)
> create mode 100644 tests/fchostdata/fc_host/host7/node_name
> create mode 100644 tests/fchostdata/fc_host/host7/port_name
> create mode 100644 tests/fchostdata/fc_host/host7/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/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/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c
>index be99c41..e043744 100644
>--- a/src/node_device/node_device_linux_sysfs.c
>+++ b/src/node_device/node_device_linux_sysfs.c
>@@ -73,9 +73,8 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
> 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",
>+ VIR_INFO("Optional fabric WWN for host%d not available",
> d->scsi_host.host);
>- goto cleanup;
> }
> 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..ab61302 100644
>--- a/src/util/virutil.c
>+++ b/src/util/virutil.c
>@@ -2011,7 +2011,7 @@ virGetSCSIHostNameByParentaddr(unsigned int domain,
> *
> * Read the value of sysfs "fc_host" entry.
> *
>- * Returns result as a stringon success, caller must free @result after
>+ * Returns result as a string on success, caller must free @result after
> * Otherwise returns NULL.
Looks like this could be separate commit.
> */
> char *
>@@ -2029,7 +2029,7 @@ virReadFCHost(const char *sysfs_prefix,
> host, entry) < 0)
> goto cleanup;
>
>- if (virFileReadAll(sysfs_path, 1024, &buf) < 0)
>+ if (virFileReadAllQuiet(sysfs_path, 1024, &buf) < 0)
Looks like the calls to this set the error or don't error out, so this
makes sense, but the other stub adds error, the previous call does too,
etc. Looking at it more closely, everything that calls this function is
a bit of a mess regarding error reporting. Just to not make it even
worse than before, I would suggest adding virFileExists() before this
and make the reporting optional. Or simply make the previous function
call quiet as well.
> goto cleanup;
>
> if ((p = strchr(buf, '\n')))
>diff --git a/tests/fchostdata/fc_host/host7/node_name b/tests/fchostdata/fc_host/host7/node_name
>new file mode 100644
>index 0000000..73a91bd
>--- /dev/null
>+++ b/tests/fchostdata/fc_host/host7/node_name
>@@ -0,0 +1 @@
>+0x2002001b32a9da4e
>diff --git a/tests/fchostdata/fc_host/host7/port_name b/tests/fchostdata/fc_host/host7/port_name
>new file mode 100644
>index 0000000..f25abeb
>--- /dev/null
>+++ b/tests/fchostdata/fc_host/host7/port_name
>@@ -0,0 +1 @@
>+0x2102001b32a9da4e
>diff --git a/tests/fchostdata/fc_host/host7/port_state b/tests/fchostdata/fc_host/host7/port_state
>new file mode 100644
>index 0000000..b73dd46
>--- /dev/null
>+++ b/tests/fchostdata/fc_host/host7/port_state
>@@ -0,0 +1 @@
>+Online
>diff --git a/tests/fchosttest.c b/tests/fchosttest.c
>index a08a2e8..ba580aa 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 7
>
The test number should be the same as the host number to make it
consistent, but it looks like it's not even the case now. But that
doesn't mean we need to be butchering it even more.
> /* 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;
>@@ -106,10 +109,43 @@ test3(const void *data ATTRIBUTE_UNUSED)
> return ret;
> }
>
>-/* Test virGetFCHostNameByWWN */
>+/* Test virReadFCHost fabric name optional */
> static int
> test4(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 (virReadFCHost(TEST_FC_HOST_PREFIX, TEST_FC_HOST_NUM_NO_FAB,
>+ "fabric_name"))
>+ goto cleanup;
>+
>+ if (STRNEQ(expect_wwnn, wwnn) ||
>+ STRNEQ(expect_wwpn, wwpn))
>+ goto cleanup;
>+
>+ ret = 0;
>+ cleanup:
>+ VIR_FREE(wwnn);
>+ VIR_FREE(wwpn);
>+ return ret;
>+}
>+
>+/* Test virGetFCHostNameByWWN */
>+static int
>+test5(const void *data ATTRIBUTE_UNUSED)
>+{
> const char *expect_hostname = "host5";
> char *hostname = NULL;
> int ret = -1;
Why don't you add it as a test6?
>@@ -130,7 +166,7 @@ test4(const void *data ATTRIBUTE_UNUSED)
>
> /* Test virFindFCHostCapableVport (host4 is not Online) */
> static int
>-test5(const void *data ATTRIBUTE_UNUSED)
>+test6(const void *data ATTRIBUTE_UNUSED)
> {
> const char *expect_hostname = "host5";
> char *hostname = NULL;
>@@ -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);
>--
>2.5.5
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170112/302de2aa/attachment-0001.sig>
More information about the libvir-list
mailing list