Re: [GRUB PARTUUID PATCH V8 4/4] Update grub script template files

[Daniel Kiper]

On Wed, Mar 28, 2018 at 08:32:54AM -0700, Nicholas Vinson wrote:
> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
> partuuid target.  Update grub.texi documenation.
>
> Signed-off-by: Nicholas Vinson <address@hidden>
> ---
>  docs/grub.texi          |  9 +++++++++
>  util/grub-mkconfig.in   |  3 +++
>  util/grub.d/10_linux.in | 12 +++++++++---
>  3 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 65b4bbeda..019ce5d03 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1424,6 +1424,15 @@ the Linux kernel, using a @samp{root=UUID=...} kernel 
> parameter.  This is
>  usually more reliable, but in some cases it may not be appropriate.  To
>  disable the use of UUIDs, set this option to @samp{true}.
>
> address@hidden GRUB_ENABLE_LINUX_PARTUUID
> +Setting this option to @samp{true} changes the behavior of
> address@hidden so that it identifies the device containing the root
> +filesystem by the partition UUID via the @samp{root=PARTUUID=...} kernel
> +parameter.  This is desirable for Linux systems where identifying the root
> +filesystem by its filesystem UUID or device name is inappropriate.  However,
> +this option is only supported in Linux kernel versions 2.6.37 (3.10 for 
> systems
> +using the MSDOS partition scheme) or newer.
> +
>  @item GRUB_DISABLE_RECOVERY
>  If this option is set to @samp{true}, disable the generation of recovery
>  mode menu entries.
> diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
> index 35ef583b0..9a1f92bdf 100644
> --- a/util/grub-mkconfig.in
> +++ b/util/grub-mkconfig.in
> @@ -134,6 +134,7 @@ fi
>  # Device containing our userland.  Typically used for root= parameter.
>  GRUB_DEVICE="`${grub_probe} --target=device /`"
>  GRUB_DEVICE_UUID="`${grub_probe} --device ${GRUB_DEVICE} --target=fs_uuid 2> 
> /dev/null`" || true
> +GRUB_DEVICE_PARTUUID="`${grub_probe} --device ${GRUB_DEVICE} 
> --target=partuuid 2> /dev/null`" || true
>
>  # Device containing our /boot partition.  Usually the same as GRUB_DEVICE.
>  GRUB_DEVICE_BOOT="`${grub_probe} --target=device /boot`"
> @@ -188,6 +189,7 @@ if [ "x${GRUB_ACTUAL_DEFAULT}" = "xsaved" ] ; then 
> GRUB_ACTUAL_DEFAULT="`"${grub
>  # override them.
>  export GRUB_DEVICE \
>    GRUB_DEVICE_UUID \
> +  GRUB_DEVICE_PARTUUID \
>    GRUB_DEVICE_BOOT \
>    GRUB_DEVICE_BOOT_UUID \
>    GRUB_FS \
> @@ -223,6 +225,7 @@ export GRUB_DEFAULT \
>    GRUB_TERMINAL_OUTPUT \
>    GRUB_SERIAL_COMMAND \
>    GRUB_DISABLE_LINUX_UUID \
> +  GRUB_ENABLE_LINUX_PARTUUID \
>    GRUB_DISABLE_RECOVERY \
>    GRUB_VIDEO_BACKEND \
>    GRUB_GFXMODE \
> diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> index faedf74e1..53de33bea 100644
> --- a/util/grub.d/10_linux.in
> +++ b/util/grub.d/10_linux.in
> @@ -45,12 +45,18 @@ esac
>
>  # btrfs may reside on multiple devices. We cannot pass them as value of 
> root= parameter
>  # and mounting btrfs requires user space scanning, so force UUID in this 
> case.
> -if [ "x${GRUB_DEVICE_UUID}" = "x" ] || [ "x${GRUB_DISABLE_LINUX_UUID}" = 
> "xtrue" ] \
> -    || ! test -e "/dev/disk/by-uuid/${GRUB_DEVICE_UUID}" \
> +if [ "x${GRUB_DEVICE_UUID}" = "x" ] && [ "x${GRUB_DEVICE_PARTUUID}" = "x" ] \
> +    || [ "x${GRUB_DISABLE_LINUX_UUID}" = "xtrue" ] \

Hmmm... Have you tested this patch with GRUB_DISABLE_LINUX_UUID=true and
GRUB_ENABLE_LINUX_PARTUUID=true? I do not think so. LINUX_ROOT_DEVICE will
be set to ${GRUB_DEVICE}. I have a feeling that this is not what you expect.

Additionally, what will happen if GRUB_DISABLE_LINUX_UUID=true and
GRUB_ENABLE_LINUX_PARTUUID=false? Well, this should work right now
but please test it after fixing above mentioned issue.

Should not we do s/GRUB_ENABLE_LINUX_PARTUUID/GRUB_DISABLE_LINUX_PARTUUID/?
Then the variable will have similar meaning to GRUB_DISABLE_LINUX_UUID.
Hence, all will be less confusing.

> +    || ( ! test -e "/dev/disk/by-uuid/${GRUB_DEVICE_UUID}" \
> +        && ! test -e "/dev/disk/by-partuuid/${GRUB_DEVICE_PARTUUID}" ) \
>      || ( test -e "${GRUB_DEVICE}" && uses_abstraction "${GRUB_DEVICE}" lvm 
> ); then
>    LINUX_ROOT_DEVICE=${GRUB_DEVICE}
> -else
> +elif [ "x${GRUB_ENABLE_LINUX_PARTUUID}" != "xtrue" ] \
> +    || [ "x${GRUB_DEVICE_PARTUUID}" = "x" ] \
> +    || ! test -e "/dev/disk/by-partuuid/${GRUB_DEVICE_PARTUUID}"; then
>    LINUX_ROOT_DEVICE=UUID=${GRUB_DEVICE_UUID}
> +else
> +  LINUX_ROOT_DEVICE=PARTUUID=${GRUB_DEVICE_PARTUUID}
>  fi

Daniel