Hi Daniel,
Thank you so much for a review!
On 2025-05-27 01:00, Daniel Kiper wrote:
On Mon, May 19, 2025 at 04:34:34PM +0530, Avnish Chouhan wrote:
This patch sets mupltiple NVMe boot-devices for more robust boot.
Scenario where NVMe multipaths are available, all the available
bootpaths (Max 5)
will be added as the boot-device.
Signed-off-by: Avnish Chouhan <avn...@linux.ibm.com>
---
grub-core/osdep/unix/platform.c | 114
++++++++++++++++++++++++++++++++++++++++++++++++++++-
grub-core/osdep/linux/ofpath.c | 6 +++---
include/grub/util/install.h | 3 +++
include/grub/util/ofpath.h | 4 ++++
4 files changed, 123 insertions(+), 4 deletion(-)
diff --git a/grub-core/osdep/unix/platform.c
b/grub-core/osdep/unix/platform.c
index 55b8f40..124e0ed 100644
--- a/grub-core/osdep/unix/platform.c
+++ b/grub-core/osdep/unix/platform.c
@@ -28,6 +28,8 @@
#include <dirent.h>
#include <string.h>
#include <errno.h>
+#include <grub/util/ofpath.h>
Please add empty line here.
Sure!
+#define BOOTDEV_BUFFER 1000
static char *
get_ofpathname (const char *dev)
@@ -176,6 +178,105 @@ grub_install_register_efi (grub_device_t
efidir_grub_dev,
return ret;
}
+
+char *
+add_multiple_nvme_bootdevices (const char *install_device)
+{
+ char *sysfs_path, *nvme_ns, *ptr, *non_splitter_path;
+ unsigned int nsid;
+ char *multipath_boot, *ofpath, *ext_dir;
+ struct dirent *ep, *splitter_ep;
+ DIR *dp, *splitter_dp;
+ char *cntl_id, *dirR1, *dirR2, *splitter_info_path;
+ int is_FC = 0, is_splitter = 0;
Please use bool type here.
Sure!
+ nvme_ns = grub_strstr (install_device, "nvme");
grub_strstr() may return NULL. If it is not possible here then it
should
be explained.
NULL check not required here as this function works only on
install_device as "/dev/nvme3n1p1". I'll add the comment in the next
version!
+ nsid = of_path_get_nvme_nsid (nvme_ns);
+ if (nsid == 0)
+ return NULL;
+
+ sysfs_path = nvme_get_syspath (nvme_ns);
+ ofpath = xasprintf ("%s",get_ofpathname (nvme_ns));
Missing space after ",".
Sure, I'll add a space after ','.
+ if (grub_strstr (ofpath, "fibre-channel"))
+ {
+ strcat (sysfs_path, "/device");
+ is_FC = 1;
+ }
+ else
+ {
+ strcat (sysfs_path, "/subsystem");
+ is_FC = 0;
+ }
+ if (is_FC == 0)
+ {
+ cntl_id = grub_strstr (nvme_ns, "e");
Again, missing NULL check and s/grub_strstr/grub_strchr/...
NULL check not required as explained above. I'll change grub_strstr to
grub_strchr.
+ dirR1 = xasprintf ("nvme%c",cntl_id[1]);
+
+ splitter_info_path = xasprintf ("%s%s%s", "/sys/block/",
nvme_ns, "/device");
... xasprintf ("/sys/block/%s/device", nvme_ns);
Sure!
+ splitter_dp = opendir (splitter_info_path);
+ if (!splitter_dp)
+ return NULL;
+
+ while ((splitter_ep = readdir (splitter_dp)) != NULL)
+ {
+ if (grub_strstr (splitter_ep->d_name, "nvme"))
+ {
+ if (grub_strstr (splitter_ep->d_name, dirR1))
+ continue;
+
+ ext_dir = grub_strstr (splitter_ep->d_name, "e");
s/grub_strstr/grub_strchr/
Sure!
Missing NULL check too...
Not required as the check above "if (grub_strstr (splitter_ep->d_name,
"nvme"))".
+ if (!(grub_strstr (ext_dir, "n")))
s/grub_strstr/grub_strchr/
Sure, I'll change this!
grub_strchr (ext_dir, 'n') == NULL
+ {
+ dirR2 = xasprintf("%s", splitter_ep->d_name);
Something is off with indention...
Sure, I'll check and correct this!
+ is_splitter = 1;
+ break;
+ }
+ }
+ }
+ closedir (splitter_dp);
+ }
+ sysfs_path = xrealpath (sysfs_path);
+ dp = opendir (sysfs_path);
+ if (!dp)
+ return NULL;
+
+ ptr = multipath_boot = xmalloc (BOOTDEV_BUFFER);
+ if (is_splitter == 0 && is_FC == 0)
+ {
+ non_splitter_path = xasprintf ("%s%s%x:1 ", get_ofpathname
(dirR1), "/namespace@", nsid);
... xasprintf ("%s/namespace@%x:1 ", get_ofpathname (dirR1), nsid);
+ strncpy (ptr, non_splitter_path, strlen (non_splitter_path));
+ ptr += strlen (non_splitter_path);
+ free (non_splitter_path);
+ }
+ else
+ {
+ while ((ep = readdir (dp)) != NULL)
+ {
+ char *path;
Please add empty line here...
Sure.
+ if (grub_strstr (ep->d_name, "nvme"))
grub_strstr (ep->d_name, "nvme") != NULL
+ {
+ if (is_FC == 0 && !grub_strstr (ep->d_name, dirR1) &&
!grub_strstr (ep->d_name, dirR2))
... grub_strstr (ep->d_name, dirR1) == NULL && grub_strstr
(ep->d_name, dirR2) == NULL ...
+ continue;
+ path = xasprintf ("%s%s%x ", get_ofpathname
(ep->d_name), "/namespace@", nsid);
... xasprintf ("%s/namespace@%x:1 ", get_ofpathname (ep->d_name),
nsid);
I'll change as suggested here and in other places too!
+ if ((strlen (multipath_boot) + strlen (path)) >
BOOTDEV_BUFFER)
+ {
+ grub_util_warn (_("Maximum five entries are allowed
in the bootlist"));
+ free (path);
+ break;
+ }
+ strncpy (ptr, path, strlen (path));
+ ptr += strlen (path);
+ free (path);
+ }
+ }
+ }
+ *--ptr = '\0';
+ closedir (dp);
+
+ return multipath_boot;
+}
+
void
grub_install_register_ieee1275 (int is_prep, const char
*install_device,
int partno, const char *relpath)
@@ -215,8 +316,19 @@ grub_install_register_ieee1275 (int is_prep,
const char *install_device,
}
*ptr = '\0';
}
+ else if (grub_strstr (install_device, "nvme"))
+ {
+ boot_device = add_multiple_nvme_bootdevices (install_device);
+ }
else
- boot_device = get_ofpathname (install_device);
+ {
+ boot_device = get_ofpathname (install_device);
+ if (grub_strstr (boot_device, "nvme-of"))
+ {
+ free (boot_device);
+ boot_device = add_multiple_nvme_bootdevices
(install_device);
+ }
+ }
if (grub_util_exec ((const char * []){ "nvsetenv", "boot-device",
boot_device, NULL }))
diff --git a/grub-core/osdep/linux/ofpath.c
b/grub-core/osdep/linux/ofpath.c
index 7158c8c..48f11c9 100644
--- a/grub-core/osdep/linux/ofpath.c
+++ b/grub-core/osdep/linux/ofpath.c
@@ -209,7 +209,7 @@ find_obppath (const char *sysfs_path_orig)
}
}
-static char *
+char *
xrealpath (const char *in)
{
char *out;
@@ -224,7 +224,7 @@ xrealpath (const char *in)
return out;
}
-static char *
+char *
You do not need this change.
We need this function as this is used by the function
"of_path_get_nvme_nsid" we are using.
block_device_get_sysfs_path_and_link(const char *devicenode)
{
char *rpath;
@@ -684,7 +684,7 @@ of_path_get_nvme_nsid (const char* devname)
return nsid;
}
-static char *
+char *
nvme_get_syspath (const char *nvmedev)
{
char *sysfs_path;
diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index 51f3b13..a67e225 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -235,6 +235,9 @@ grub_install_register_efi (grub_device_t
efidir_grub_dev,
const char *efifile_path,
const char *efi_distributor);
+char *
+add_multiple_nvme_bootdevices (const char *install_device);
+
void
grub_install_register_ieee1275 (int is_prep, const char
*install_device,
int partno, const char *relpath);
diff --git a/include/grub/util/ofpath.h b/include/grub/util/ofpath.h
index 5962322..78e78e7 100644
--- a/include/grub/util/ofpath.h
+++ b/include/grub/util/ofpath.h
@@ -30,5 +30,9 @@ int add_filename_to_pile (char *filename, struct
ofpath_files_list_root* root);
void find_file (char* filename, char* directory, struct
ofpath_files_list_root* root, int max_depth, int depth);
char* of_find_fc_host (char* host_wwpn);
void free_ofpath_files_list (struct ofpath_files_list_root* root);
+char* nvme_get_syspath (const char *nvmedev);
+char* block_device_get_sysfs_path_and_link (const char *devicenode);
Please drop this declaration.
Same as explained above!
Daniel
Regards,
Avnish Chouhan
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
