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

[Nick Vinson]

On 04/04/2018 01:23 PM, Daniel Kiper wrote:
> 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.

The idea was for GRUB_DISABLE_LINUX_UUID to act as a controlling
variable with this setup.
I reviewed the description for the variable, and having it act that way
doesn't make sense.  I will rewrite this part and make sure the two
variables act as independently as possible.

The one exception, however, would be when GRUB_DISABLE_LINUX_UUID is
unset and GRUB_ENABLE_LINUX_PARTUUID is set.  In such a case, I'll have
the code favor the fs UUID over the partition UUID.  I think this would
cause the least amount of surprise since the initramfs images I've seen
prefer the fs UUID.

> 
> 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.

It should favor the Linux device name.  I'll make sure that behavior is
preserved.

> 
> 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.

I can return the name to GRUB_DISABLE_LINUX_PARTUUID.  When I originally
used that name, there was concerns about maintaining compatibility with
older kernel versions.  However, I think I can use the name
GRUB_DISABLE_LINUX_PARTUUID while maintaining that compatibility.

Thanks,
Nicholas Vinson

> 
>> +    || ( ! 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
>

signature.asc
Description: OpenPGP digital signature