On Fri, Nov 01, 2024 at 10:03:16AM +0800, Michael Chang wrote: > On Wed, Oct 30, 2024 at 05:12:48PM GMT, Daniel Kiper wrote: > > Adding Leo... > > > > On Tue, Oct 29, 2024 at 03:57:18PM +0800, Michael Chang via Grub-devel > > wrote: > > > The "cmdpath" environment variable is set at startup to the location > > > from which the grub image is loaded. It includes a device part and, > > > optionally, an absolute directory name if the grub image is booted as a > > > file in a local file-system directory, or in a remote server directory, > > > like TFTP. > > > > > > This entire process relies on firmware to provide the correct device > > > path of the booted image. > > > > > > We encountered an issue when the image is booted from the root > > > directory, where the absolute directory name "/" is discarded. This > > > makes it unclear whether the root path was missing in the firmware > > > provided device path or if it is simply the root directory. This > > > ambiguity can cause confusion in custom scripts, potentially causing > > > them to interpret firmware data incorrectly and trigger unintended > > > fallback measures. > > > > > > This patch fixes the problem by properly assigning the "fwpath" returned > > > by "grub_machine_get_bootlocation()" to "cmdpath". The fix is based on > > > the fact that fwpath is NULL if the firmware didn’t provide a path part > > > or an NUL character, "", if it represents the root directory. With this, > > > it becomes possible to clearly distinguish: > > > > > > - cmdpath=(hd0,1) - Either the image is booted from the first (raw) > > > partition, or the firmware failed to provide the path part. > > > - cmdpath=(hd0,1)/ - The image is booted from the root directory in the > > > first partition. > > > > > > As a side note, the fix is similar to [1], but without the renaming > > > part. > > > > > > [1] https://mail.gnu.org/archive/html/grub-devel/2024-10/msg00155.html > > > > > > Signed-off-by: Michael Chang <mch...@suse.com> > > > --- > > > grub-core/kern/main.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > index d29494d54..6645173c1 100644 > > > --- a/grub-core/kern/main.c > > > +++ b/grub-core/kern/main.c > > > @@ -136,7 +136,11 @@ grub_set_prefix_and_root (void) > > > { > > > char *cmdpath; > > > > > > - cmdpath = grub_xasprintf ("(%s)%s", fwdevice, fwpath ? : ""); > > > + if (fwpath && *fwpath == '\0') > > > + cmdpath = grub_xasprintf ("(%s)/", fwdevice); > > > + else > > > + cmdpath = grub_xasprintf ("(%s)%s", fwdevice, fwpath ? : ""); > > > > Would not this below work? > > > > cmdpath = grub_xasprintf ("(%s)%s", fwdevice, (fwpath == NULL || *fwpath > > == '\0') ? "/" : fwpath); > > It looks to me that this expression sets the path part of cmdpath to "/" > when fwpath == NULL. This is not desired, as it could mislead people > into thinking grub is booted from a root directory, when in fact it > might be a raw partition or disk.
OK, then if/else requires a comment describing what is happening there. > > And if yes does this (completely) replace the patch mentioned by you above? > > The mentioned patch is actually more complicated. I think it could be > broken down into three parts: > > 1. Fixing the boot from the root directory issue, as this patch does. > 2. Renaming cmdpath to fw_path. > 3. Using the new fw_path to look up grub.cfg. > > My concerns with respect to these three parts are: > > 1. Although it fixes the root directory issue, it also intentionally > ignores the case where fwpath is set to NULL. IMHO this is problematic > because booting from a raw partition or disk is valid, many setups such > as MBR, GPT BIOS Boot, and PPC PReP, are still actively used today. OK... > 2. Renaming the well-known cmdpath may break custom (third-party) scripts. This is not acceptable. So, cmdpath variable name cannot be changed. And I suggest to not change its name in downstream too. > 3. There is already a mechanism for looking up grub.cfg using the prefix > variable. If prefix is not set (e.g., when grub-mkimage is used without > specifying -p ..), its value is derived automatically from > fwdevice/fwpath. This makes the change seem redundant, as it’s already > covered. Which change are you referring to? > So, unless these concerns can be clarified, I think this simpler patch > makes sense, as it addresses one problem at a time in an obvious way. Michael, Leo, please agree how you want to proceed further taking into account above. > > Last but not least, please next time CC original author of the patch you > > are referring to. > > Thanks for the reminder. I will. Thanks! Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
