Re: [dm-devel] [PATCH v2 1/2] libmultipath: support host adapter name lookup for s390x ccw bus

[Martin Wilck <martin.wilck@suse.com>]

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@linux.ibm.com>
> Signed-off-by: Steffen Maier <maier@linux.ibm.com>
> ---
> 
> 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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel