On Tue, June 3, 2025 at 5:04 AM, Avnish Chouhan <avn...@linux.ibm.com> wrote:
> > Message: 1
> > Date: Wed, 21 May 2025 12:51:25 +0000
> > From: Alec Brown <alec.r.br...@oracle.com>
> > To: grub-devel@gnu.org
> > Cc: christopher.obb...@linaro.org, daniel.ki...@oracle.com,
> > jan.setjeeil...@oracle.com, alec.r.br...@oracle.com,
> > mate.ku...@canonical.com, pjo...@redhat.com,
> > ross.philip...@oracle.com, 93...@debian.org, phco...@gmail.com
> > Subject: [PATCH v4 3/4] blsuki: Check for mounted /boot in emu
> > Message-ID: <20250521125126.3928350-4-alec.r.br...@oracle.com>
> >
> > From: Robbie Harwood <rharw...@redhat.com>
> >
> > Irritatingly, BLS defines paths relative to the mountpoint of the
> > filesystem which contains its snippets, not / or any other fixed
> > location. So grub2-emu needs to know whether /boot is a separate
> > filesystem from / and conditionally prepend a path.
> >
> > Signed-off-by: Robbie Harwood <rharw...@redhat.com>
> > Signed-off-by: Alec Brown <alec.r.br...@oracle.com>
> > ---
> > grub-core/Makefile.core.def | 4 ++
> > grub-core/commands/blsuki.c | 92 ++++++++++++++++++++++++++++++---
> > grub-core/osdep/linux/getroot.c | 8 +++
> > grub-core/osdep/unix/getroot.c | 10 ++++
> > include/grub/emu/misc.h | 2 +-
> > 5 files changed, 107 insertions(+), 9 deletions(-)
> >
> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 67628f65f..93b88795e 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -367,6 +367,10 @@ kernel = {
> > emu = kern/emu/cache_s.S;
> > emu = kern/emu/hostdisk.c;
> > emu = osdep/unix/hostdisk.c;
> > + emu = osdep/relpath.c;
> > + emu = osdep/getroot.c;
> > + emu = osdep/unix/getroot.c;
> > + emu = osdep/devmapper/getroot.c;
> > emu = osdep/exec.c;
> > extra_dist = osdep/unix/exec.c;
> > emu = osdep/devmapper/hostdisk.c;
> > diff --git a/grub-core/commands/blsuki.c b/grub-core/commands/blsuki.c
> > index 2ad960ae3..bf2edc5ac 100644
> > --- a/grub-core/commands/blsuki.c
> > +++ b/grub-core/commands/blsuki.c
> > @@ -32,6 +32,13 @@
> > #include <grub/vercmp.h>
> > #include <grub/lib/envblk.h>
> >
> > +#ifdef GRUB_MACHINE_EMU
> > +#include <grub/emu/misc.h>
> > +#define GRUB_BOOT_DEVICE "/boot"
> > +#else
> > +#define GRUB_BOOT_DEVICE ""
> > +#endif
> > +
> > GRUB_MOD_LICENSE ("GPLv3+");
> >
> > #define GRUB_BLS_CONFIG_PATH "/loader/entries/"
> > @@ -56,6 +63,40 @@ static grub_blsuki_entry_t *entries = NULL;
> >
> > #define FOR_BLSUKI_ENTRIES(var) FOR_LIST_ELEMENTS (var, entries)
> >
> > +#ifdef GRUB_MACHINE_EMU
> > +/*
> > + * Cache probing in blsuki_update_boot_device().
> > + */
>
> Hi Alec,
>
> /* Cache probing in blsuki_update_boot_device(). */
>
> > +static int separate_boot = -1;
> > +#endif
> > +
> > +/*
> > + * BLS appears to make paths relative to the filesystem that snippets
> > are
> > + * on, not /. Attempt to cope.
> > + */
> > +static char *blsuki_update_boot_device (char *tmp)
> > +{
> > +#ifdef GRUB_MACHINE_EMU
> > + char *ret;
>
> char *ret = NULL;
Hi Avnish,
I don't think it is necessary for this variable to be initialized to NULL since
it will be overwritten by grub_make_system_path_relative_to_its_root().
>
> > +
> > + if (separate_boot != -1)
> > + goto probed;
> > +
> > + separate_boot = 0;
> > +
> > + ret = grub_make_system_path_relative_to_its_root (GRUB_BOOT_DEVICE);
> > +
> > + if (ret != NULL && ret[0] == '\0')
> > + separate_boot = 1;
> > +
> > + probed:
> > + if (!separate_boot)
> > + return tmp;
> > +#endif
> > +
> > + return grub_stpcpy (tmp, GRUB_BOOT_DEVICE);
> > +}
> > +
> > /*
> > * This function will add a new keyval pair to a list of keyvals
> > stored in the
> > * entry parameter.
> > @@ -582,7 +623,7 @@ bls_get_linux (grub_blsuki_entry_t *entry)
> > if (options == NULL)
> > options = blsuki_expand_val (grub_env_get ("default_kernelopts"));
> >
> > - if (grub_add (grub_strlen ("linux "), grub_strlen (linux_path),
> > &size))
> > + if (grub_add (grub_strlen ("linux " GRUB_BOOT_DEVICE), grub_strlen
> > (linux_path), &size))
> > {
> > grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected while
> > calculating linux buffer size");
> > goto finish;
> > @@ -604,6 +645,7 @@ bls_get_linux (grub_blsuki_entry_t *entry)
> >
> > tmp = linux_cmd;
> > tmp = grub_stpcpy (tmp, "linux ");
> > + tmp = blsuki_update_boot_device (tmp);
> > tmp = grub_stpcpy (tmp, linux_path);
> > if (options != NULL)
> > {
> > @@ -679,7 +721,7 @@ bls_get_initrd (grub_blsuki_entry_t *entry)
> >
> > for (i = 0; early_initrd_list != NULL && early_initrd_list[i]
> > != NULL; i++)
> > {
> > - if (grub_add (size, 1, &size) ||
> > + if (grub_add (size, grub_strlen (" " GRUB_BOOT_DEVICE), &size) ||
> > grub_add (size, grub_strlen (prefix), &size) ||
> > grub_add (size, grub_strlen (early_initrd_list[i]), &size))
> > {
> > @@ -690,7 +732,7 @@ bls_get_initrd (grub_blsuki_entry_t *entry)
> >
> > for (i = 0; initrd_list != NULL && initrd_list[i] != NULL; i++)
> > {
> > - if (grub_add (size, 1, &size) ||
> > + if (grub_add (size, grub_strlen (" " GRUB_BOOT_DEVICE), &size) ||
> > grub_add (size, grub_strlen (initrd_list[i]), &size))
> > {
> > grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected
> > calculating initrd buffer size");
> > @@ -713,6 +755,7 @@ bls_get_initrd (grub_blsuki_entry_t *entry)
> > {
> > grub_dprintf ("blsuki", "adding early initrd %s\n",
> > early_initrd_list[i]);
> > tmp = grub_stpcpy (tmp, " ");
> > + tmp = blsuki_update_boot_device (tmp);
> > tmp = grub_stpcpy (tmp, prefix);
> > tmp = grub_stpcpy (tmp, early_initrd_list[i]);
> > }
> > @@ -721,6 +764,7 @@ bls_get_initrd (grub_blsuki_entry_t *entry)
> > {
> > grub_dprintf ("blsuki", "adding initrd %s\n", initrd_list[i]);
> > tmp = grub_stpcpy (tmp, " ");
> > + tmp = blsuki_update_boot_device (tmp);
> > tmp = grub_stpcpy (tmp, initrd_list[i]);
> > }
> > tmp = grub_stpcpy (tmp, "\n");
> > @@ -775,7 +819,7 @@ bls_get_devicetree (grub_blsuki_entry_t *entry)
> > }
> > }
> >
> > - if (grub_add (grub_strlen ("devicetree "), grub_strlen
> > (dt_path), &size) ||
> > + if (grub_add (grub_strlen ("devicetree " GRUB_BOOT_DEVICE),
> > grub_strlen (dt_path), &size) ||
> > grub_add (size, 1, &size))
> > {
> > grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> > device tree buffer size");
> > @@ -796,6 +840,7 @@ bls_get_devicetree (grub_blsuki_entry_t *entry)
> > tmp = dt_cmd;
> > tmp = grub_stpcpy (dt_cmd, "devicetree");
> > tmp = grub_stpcpy (tmp, " ");
> > + tmp = blsuki_update_boot_device (tmp);
> > if (add_dt_prefix == true)
> > tmp = grub_stpcpy (tmp, prefix);
> > tmp = grub_stpcpy (tmp, dt_path);
> > @@ -924,7 +969,11 @@ blsuki_set_find_entry_info (struct
> > find_entry_info *info, const char *dirname, c
> >
> > if (devid == NULL)
> > {
> > +#ifdef GRUB_MACHINE_EMU
> > + devid = "host";
> > +#else
> > devid = grub_env_get ("root");
> > +#endif
> > if (devid == NULL)
> > return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't
> > set"), "root");
> > }
> > @@ -966,10 +1015,13 @@ blsuki_set_find_entry_info (struct
> > find_entry_info *info, const char *dirname, c
> > * parameter. If the fallback option is enabled, the default location
> > will be
> > * checked for BLS config files if the first attempt fails.
> > */
> > -static void
> > +static grub_err_t
> > blsuki_find_entry (struct find_entry_info *info, bool enable_fallback)
> > {
> > struct read_entry_info read_entry_info;
> > + char *default_dir = NULL;
> > + char *tmp;
>
> char *tmp = NULL;
I don't think it is necessary for this NULL initialization either since tmp
will get set to the value given by blsuki_update_boot_device(). Times where I
think it would be necessary is if the variables initialization is dependent on
an if conditional. However, looking through this function in particular, I see
some variables such as dir_fs and dir_dev don't need to be set to NULL.
>
> > + grub_size_t default_size;
> > grub_fs_t dir_fs = NULL;
> > grub_device_t dir_dev = NULL;
> > bool fallback = false;
> > @@ -1000,7 +1052,15 @@ blsuki_find_entry (struct find_entry_info
> > *info, bool enable_fallback)
> > */
> > if (entries == NULL && fallback == false && enable_fallback ==
> > true)
> > {
> > - blsuki_set_find_entry_info (info, GRUB_BLS_CONFIG_PATH, NULL);
> > + default_size = grub_strlen (GRUB_BOOT_DEVICE) + grub_strlen
> > (GRUB_BLS_CONFIG_PATH);
> > + default_dir = grub_malloc (default_size);
> > + if (default_dir == NULL)
> > + return grub_errno;
> > +
> > + tmp = blsuki_update_boot_device (default_dir);
> > + tmp = grub_stpcpy (tmp, GRUB_BLS_CONFIG_PATH);
> > +
> > + blsuki_set_find_entry_info (info, default_dir, NULL);
>
> This function returns "grub_err_t", error check missed!
Good catch! I'll fix that.
>
> err = blsuki_set_find_entry_info (info, default_dir, NULL);
> if (err)
> return grub_errno;
>
> > grub_dprintf ("blsuki", "Entries weren't found in %s, fallback to
> > %s\n",
> > read_entry_info.dirname, info->dirname);
> > fallback = true;
> > @@ -1009,6 +1069,9 @@ blsuki_find_entry (struct find_entry_info *info,
> > bool enable_fallback)
> > fallback = false;
> > }
> > while (fallback == true);
> > +
> > + grub_free (default_dir);
> > + return GRUB_ERR_NONE;
> > }
> >
> > static grub_err_t
> > @@ -1018,6 +1081,9 @@ blsuki_load_entries (char *path, bool
> > enable_fallback)
> > static grub_err_t r;
> > const char *devid = NULL;
> > char *dir = NULL;
> > + char *default_dir = NULL;
> > + char *tmp;
>
> char *tmp = NULL;
>
> > + grub_size_t dir_size;
> > struct find_entry_info info = {
> > .dev = NULL,
> > .fs = NULL,
> > @@ -1059,15 +1125,25 @@ blsuki_load_entries (char *path, bool
> > enable_fallback)
> > }
> >
> > if (dir == NULL)
> > - dir = (char *) GRUB_BLS_CONFIG_PATH;
> > + {
> > + dir_size = grub_strlen (GRUB_BOOT_DEVICE) + grub_strlen
> > (GRUB_BLS_CONFIG_PATH);
> > + default_dir = grub_malloc (dir_size);
> > + if (default_dir == NULL)
> > + return grub_errno;
>
> Indentation seems off, please check!
Same thing with the other patches, the indentation looks strange because tabs
are being used in place of 8 spaces and the formatting of the patch looks
strange because of it.
>
> Thank you!
>
> Regards,
> Avnish Chouhan
>
Thanks for looking through these patches!
Alec Brown
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
