On 1/15/21 10:28 AM, Oleksandr Shchirskyi wrote:
> From: Blazej Kucman <blazej.kucman@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Blazej Kucman <blazej.kucman@xxxxxxxxx>
> ---
> 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
