Re: [PATCH v2] ieee1275: support added for multiple nvme bootpaths

[Avnish Chouhan <avnish@linux.ibm.com>]

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 <avnish@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