Re: [PATCH v2] Add GRUB_DISABLE_UUID

[Nicholas Vinson <nvinson234@gmail.com>]

On 10/2/19 08:09, Daniel Kiper wrote:
> Adding Nicholas...
> 
> On Mon, Sep 30, 2019 at 04:59:12PM +0200, Javier Martinez Canillas wrote:
>> From: Peter Jones <pjones@redhat.com>
>>
>> The grub-mkconfig and 10_linux scripts by default attempt to use a UUID to
>> set the root kernel command line parameter and the $root GRUB environment
>> variable.
>>
>> The former can be disabled by setting the GRUB_DISABLE_LINUX_UUID variable
>> to "true", but there is currently no way to disable the latter.
>>
>> The generated grub config uses the search command with the --fs-uuid option
>> to find the device that has to be set as $root, i.e:
>>
>>   search --no-floppy --fs-uuid --set=root ...
>>
>> This is usually more reliable but in some cases it may not be appropriate,
>> so this patch introduces a new GRUB_DISABLE_UUID variable that can be used
>> to disable searching for the $root device by filesystem UUID.
>>
>> When disabled, the $root device will be set to the value specified in the
>> device.map as found by the grub-probe --target=compatibility_hint option.
>>
>> When setting GRUB_DISABLE_UUID=true, the GRUB_DISABLE_LINUX_UUID and
>> GRUB_DISABLE_LINUX_PARTUUID variables will also be set to "true" unless
>> these have been explicitly set to "false".
>>
>> That way, the GRUB_DISABLE_UUID variable can be used to force using the
>> device names for both GRUB and Linux.
>>
>> Signed-off-by: Peter Jones <pjones@redhat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> In general "Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>" but I would
> like to hear Nicholas opinions here too >
> Daniel
> 
>> ---
>>
>> Changes since v1 (suggested by Daniel Kiper):
>>
>> - Explain better in the commit message why the GRUB_DISABLE_UUID variable
>>    is needed and the difference with the existing GRUB_DISABLE_LINUX_UUID/
>>    GRUB_DISABLE_LINUX_PARTUUID variables.
>> - Remove logic that disabled setting the root cmdline param to either the
>>    filesystem UUID or partition UUID and instead use the existing variables
>>    to disable this.
>>
>>   docs/grub.texi            |  9 +++++++++
>>   util/grub-mkconfig.in     | 10 ++++++++++
>>   util/grub-mkconfig_lib.in |  4 ++--
>>   3 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/grub.texi b/docs/grub.texi
>> index 5ac61c09d1b..2fe0e1b83bb 100644
>> --- a/docs/grub.texi
>> +++ b/docs/grub.texi
>> @@ -1441,6 +1441,15 @@ enable the use of partition UUIDs, set this option to @samp{false}.
>>   If this option is set to @samp{true}, disable the generation of recovery
>>   mode menu entries.
>>
>> +@item GRUB_DISABLE_UUID
>> +Normally, @command{grub-mkconfig} will generate menu entries that use
>> +universally-unique identifiers (UUIDs) to identify various filesystems to
>> +search for files.  This is usually more reliable, but in some cases it may
>> +not be appropriate.  To disable this use of UUIDs, set this option to
>> +@samp{true}. Setting this option to @samp{true}, will also set the options
>> +@samp{GRUB_DISABLE_LINUX_UUID} and @samp{GRUB_DISABLE_LINUX_PARTUUID} to
>> +@samp{true}, unless they have been explicilty set to @samp{false}.
>> +
>>   @item GRUB_VIDEO_BACKEND
>>   If graphical video support is required, either because the @samp{gfxterm}
>>   graphical terminal is in use or because @samp{GRUB_GFXPAYLOAD_LINUX} is set,
>> diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
>> index a6ce375ed18..2537dbca2f9 100644
>> --- a/util/grub-mkconfig.in
>> +++ b/util/grub-mkconfig.in
>> @@ -158,6 +158,15 @@ if test -f ${sysconfdir}/default/grub ; then
>>     . ${sysconfdir}/default/grub
>>   fi
>>
>> +if [ "x${GRUB_DISABLE_UUID}" = "xtrue" ]; then
>> +  if [ "x${GRUB_DISABLE_LINUX_UUID}" != "xfalse" ]; then
>> +    GRUB_DISABLE_LINUX_UUID="true"
>> +  fi
>> +  if [ "x${GRUB_DISABLE_LINUX_PARTUUID}" != "xfalse" ]; then
>> +    GRUB_DISABLE_LINUX_PARTUUID="true"
>> +  fi
>> +fi

I don't think this logic is exactly consistent with 
51be3372ec8ba07ef68a409956ea0eefa89fe7c5.  That commit assumes any value 
other than 'true' is false.  It also sets the default values for 
GRUB_DISABLE_LINUX_PARTUUID in 10_linux.in and 20_linux_xen.in.

Both minor issues, but I do wonder if they should be addressed.

Thanks,
Nicholas Vinson

>> +
>>   # XXX: should this be deprecated at some point?
>>   if [ "x${GRUB_TERMINAL}" != "x" ] ; then
>>     GRUB_TERMINAL_INPUT="${GRUB_TERMINAL}"
>> @@ -227,6 +236,7 @@ export GRUB_DEFAULT \
>>     GRUB_DISABLE_LINUX_UUID \
>>     GRUB_DISABLE_LINUX_PARTUUID \
>>     GRUB_DISABLE_RECOVERY \
>> +  GRUB_DISABLE_UUID \
>>     GRUB_VIDEO_BACKEND \
>>     GRUB_GFXMODE \
>>     GRUB_BACKGROUND \
>> diff --git a/util/grub-mkconfig_lib.in b/util/grub-mkconfig_lib.in
>> index 0f801cab3e4..3e12b2c0e6a 100644
>> --- a/util/grub-mkconfig_lib.in
>> +++ b/util/grub-mkconfig_lib.in
>> @@ -156,7 +156,7 @@ prepare_grub_to_access_device ()
>>     if [ "x$fs_hint" != x ]; then
>>       echo "set root='$fs_hint'"
>>     fi
>> -  if fs_uuid="`"${grub_probe}" --device $@ --target=fs_uuid 2> /dev/null`" ; then
>> +  if [ "x${GRUB_DISABLE_UUID}" != "xtrue" ] && fs_uuid="`"${grub_probe}" --device $@ --target=fs_uuid 2> /dev/null`" ; then
>>       hints="`"${grub_probe}" --device $@ --target=hints_string 2> /dev/null`" || hints=
>>       echo "if [ x\$feature_platform_search_hint = xy ]; then"
>>       echo "  search --no-floppy --fs-uuid --set=root ${hints} ${fs_uuid}"
>> @@ -173,7 +173,7 @@ grub_get_device_id ()
>>     IFS='
>>   '
>>     device="$1"
>> -  if fs_uuid="`"${grub_probe}" --device ${device} --target=fs_uuid 2> /dev/null`" ; then
>> +  if [ "x${GRUB_DISABLE_UUID}" != "xtrue" ] && fs_uuid="`"${grub_probe}" --device ${device} --target=fs_uuid 2> /dev/null`" ; then
>>       echo "$fs_uuid";
>>     else
>>       echo $device |sed 's, ,_,g'
>> --
>> 2.21.0