On Mon, 2022-02-14 at 19:55 +0100, Steffen Maier wrote:
> There are also (FCP) HBAs that appear on a bus different from PCI.
>
> Complements v0.6.0 commit
> 01ab2a468ea2 ("libmultipath: Add additional path wildcards").
>
> With that we can easily get the full FCP addressing triplet
> (HBA, WWPN, LUN) from multipath tools without additional tools
> and correlation:
>
> $ multipathd -k'show paths format "%w|%i|%a|%r"'
> uuid |hcil |host adapter|target
> WWPN
> 36005076400820293e8000000000000a0|1:0:3:160 |0.0.5080
> |0x500507680b25c449
> 36005076400820293e8000000000000a0|1:0:4:160 |0.0.5080
> |0x500507680b25c448
> 36005076400820293e8000000000000a0|58:0:3:160 |0.0.50c0
> |0x500507680b26c449
> 36005076400820293e8000000000000a0|58:0:4:160 |0.0.50c0
> |0x500507680b26c448
>
> ^^^^^^^^
> instead of [undef]
>
> As a side effect this patch theoretically also enables group by
> host adapter for s390x based on v0.6.0 commit a28e61e5cc9a
> ("Crafted ordering of child paths for round robin path selector").
>
> Reviewed-by: Benjamin Block <bblock@xxxxxxxxxxxxx>
> Signed-off-by: Steffen Maier <maier@xxxxxxxxxxxxx>
> ---
>
> Notes:
> Changes since v1:
> - Make sysfs_get_host_pci_name() static and generalize for
> adapters
> on different bus types, in order to reduce code duplication
> (Ben).
> The ancestor walk is always the same based on kernel driver
> core
> with the only difference that PCI matches against driver name
> whereas CCW matches against subsystem name.
> Unfortunately, the diffstat increased because I had to move the
> new static sysfs_get_host_bus_id() in front of its only user
> sysfs_get_host_adapter_name() [or else a strange upfront
> prototype
> would have been necessary].
>
> libmultipath/discovery.c | 69 ++++++++++++++++++++++----------------
> --
> libmultipath/discovery.h | 1 -
> 2 files changed, 38 insertions(+), 32 deletions(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 7d939ae08004..5aba7e8d495f 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
>
> [...]
> -
> -int sysfs_get_host_pci_name(const struct path *pp, char *pci_name)
> +static int sysfs_get_host_bus_id(const struct path *pp, char
> *bus_id)
> {
> struct udev_device *hostdev, *parent;
> char host_name[HOST_NAME_LEN];
> - const char *driver_name, *value;
> + const char *driver_name, *subsystem_name, *value;
>
> - if (!pp || !pci_name)
> + if (!pp || !bus_id)
> return 1;
>
> sprintf(host_name, "host%d", pp->sg_id.host_no);
Nit: While at it, you could have changed sprintf() to snprintf().
But this is no requirement, can be done separately / later.
> @@ -525,10 +499,17 @@ int sysfs_get_host_pci_name(const struct path
> *pp, char *pci_name)
> }
> if (!strcmp(driver_name, "pcieport"))
> break;
The context doesn't show it here, but above these lines, we have
if (!driver_name) {
parent = udev_device_get_parent(parent);
continue;
}
Is it certain that this condition can't cause a valid ccw device (where
the driver attribute isn't required) to be skipped with the "continue"
statement? Even if the answer is "yes", I'd prefer self-explanatory
code here, because not all of us are s390 / ccw experts.
Also, the code readability could be improved by changing the while loop
to a for loop and getting rid of the multiple
udev_device_get_parent(parent) calls. Like above, not a requirement,
but the change would be welcome.
> + subsystem_name = udev_device_get_subsystem(parent);
> + if (!subsystem_name) {
> + parent = udev_device_get_parent(parent);
> + continue;
> + }
> + if (!strcmp(subsystem_name, "ccw"))
> + break;
> parent = udev_device_get_parent(parent);
> }
> if (parent) {
> - /* pci_device found
> + /* pci_device or ccw fcp device found
> */
> value = udev_device_get_sysname(parent);
>
> @@ -537,7 +518,7 @@ int sysfs_get_host_pci_name(const struct path
> *pp, char *pci_name)
> return 1;
> }
>
> - strncpy(pci_name, value, SLOT_NAME_SIZE);
> + strncpy(bus_id, value, SLOT_NAME_SIZE);
Again not mandatory, but we should replace strncpy() by strlcpy() when
we encounter it.
Regards,
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
