Re: [PATCH] imsm: nvme multipath support

[Jes Sorensen <jes@trained-monkey.org>]

On 1/15/21 10:28 AM, Oleksandr Shchirskyi wrote:
> From: Blazej Kucman <blazej.kucman@intel.com>
> 
> Add support for nvme devices which are represented
> via nvme-subsystem.
> Print warning when multi-path disk is added to RAID
> 
> Signed-off-by: Oleksandr Shchirskyi <oleksandr.shchirskyi@intel.com>
> Signed-off-by: Blazej Kucman <blazej.kucman@intel.com>
> ---
>  platform-intel.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++-
>  platform-intel.h |  2 ++
>  super-intel.c    | 38 ++++++++++++++--------
>  3 files changed, 108 insertions(+), 15 deletions(-)

Hi

A couple of nits on this one.

> diff --git a/platform-intel.c b/platform-intel.c
> index f1f6d4cd..2ee5a8da 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -668,12 +668,71 @@ const struct imsm_orom *find_imsm_capability(struct sys_dev *hba)
>  	return NULL;
>  }
>  
> +/* Check whether the nvme device is represented by nvme subsytem,
> + * if yes virtual path should be changed to hardware device path,
> + * to allow IMSM capabilities detection.
> + * Returns:
> + *	hardware path to device - if the device is represented via
> + *		nvme virtual subsytem
> + *	NULL - if the device is not represented via nvme virtual subsytem
> + */
> +char *get_nvme_multipath_dev_hw_path(const char *dev_path)
> +{
> +	char buf[PATH_MAX];
> +	DIR *dir;
> +	struct dirent *ent;
> +	char *nvme_subsystem_path = "/devices/virtual/nvme-subsystem";
> +	char *ptr;
> +	char *rp = NULL;
> +
> +	if (!strstr(dev_path, nvme_subsystem_path))
> +		return NULL;
> +
> +	memcpy(buf, dev_path, sizeof(buf) - 1);
> +	buf[PATH_MAX-1] = '\0';

You are indiscriminately copying PATH_MAX here, even if dev_path is only
a few characters long.

> +	dir = opendir(buf);
> +	if (!dir)
> +		return NULL;

Second, the only thing you do with the copy is to pass it to opendir?
Why not just check the bounds and pass dev_path straight to opendir()?

> +	for (ent = readdir(dir); ent; ent = readdir(dir)) {
> +		if (!(strncmp(ent->d_name, "nvme", 4) == 0))
> +			continue;
> +
> +		// Needed is realpath to controller not to namespace.

Please use proper comments.

> +		ptr = ent->d_name + 4;
> +		if (!strstr(ptr, "n")) {
> +			char buf1[PATH_MAX + NAME_MAX + 1];

That's a lot of stack usage in this function, why not just alloca() what
you need?

> +
> +/* Verify if multipath is supported by NVMe controller
> + * Returns:
> + *	0 - not supported
> + *	1 - supported
> + */
> +int is_multipath_nvme(int disk_fd)
> +{
> +	char *ns_name;
> +	char path_buf[PATH_MAX];
> +	char ns_path[PATH_MAX];
> +	char *nvme_subsystem_path = "/devices/virtual/nvme-subsystem";
> +
> +	ns_name = fd2kname(disk_fd);
> +	sprintf(path_buf, "/sys/block/%s", ns_name);
> +	if (!realpath(path_buf, ns_path))
> +		return 0;
> +	if (!strstr(ns_path, nvme_subsystem_path))
> +		return 0;

Given both path_buf and ns_path are throwaway buffers, why not just use
one and save the stack space?

Thanks,
Jes