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@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);