Re: [PATCH v3] commands/search: Add support to search by PARTUUID

[Nicholas Vinson <nvinson234@gmail.com>]

On 2/1/22 21:54, Glenn Washburn wrote:
> Hi Vitaly,
> 
> Now that GRUB is out of a feature freeze, there's a chance this can
> make it in.
> 
> On Thu, 15 Apr 2021 16:59:07 +0300
> Vitaly Kuzmichev via Grub-devel <grub-devel@gnu.org> wrote:
> 
>> Improve 'search' grub-shell command with functionality to search for
>> a partition by PARTUUID string. This is useful on systems where FSUUID
>> is not guaranteed to be constant, e.g. it is not preserved between
>> system updates, and modifying grub.cfg is undesired.
>>
>> V2:
>> This patch is based on Michael Grzeschik version from 27 May 2019 [1]
>> with the following changes:
>>
>> 1. Addressed review feedback from Daniel Kiper [2] and Nick Vinson [3].
>>
>> 2. Added support for GPT PARTUUID.
>>
>> 3. Moved MBR disk signature reading from partmap/msdos.c to
>> commands/search.c to read it only when it is needed.
>>
>> [1] https://lists.gnu.org/archive/html/grub-devel/2019-05/msg00124.html
>> [2] https://lists.gnu.org/archive/html/grub-devel/2019-05/msg00131.html
>> [3] https://lists.gnu.org/archive/html/grub-devel/2019-05/msg00134.html
>>
>> V3:
>> Minor coding style and spelling fixes.
>>
>> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@dev.rtsoft.ru>
>> ---
>>   grub-core/Makefile.core.def          |  5 ++
>>   grub-core/commands/search.c          | 80 +++++++++++++++++++++++++++-
>>   grub-core/commands/search_partuuid.c |  5 ++
>>   grub-core/commands/search_wrap.c     | 10 +++-
>>   include/grub/search.h                |  2 +
>>   5 files changed, 98 insertions(+), 4 deletions(-)
>>   create mode 100644 grub-core/commands/search_partuuid.c
> 
> There should be a documentation change as well to document the new
> parameter.
> 
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 8022e1c0a..4568943e3 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1063,6 +1063,11 @@ module = {
>>     common = commands/search_file.c;
>>   };
>>   
>> +module = {
>> +  name = search_partuuid;
>> +  common = commands/search_partuuid.c;
>> +};
>> +
>>   module = {
>>     name = search_fs_uuid;
>>     common = commands/search_uuid.c;
>> diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
>> index ed090b3af..5014982fd 100644
>> --- a/grub-core/commands/search.c
>> +++ b/grub-core/commands/search.c
>> @@ -54,6 +54,28 @@ struct search_ctx
>>     int is_cache;
>>   };
>>   
>> +#ifdef DO_SEARCH_PARTUUID
>> +#include <grub/gpt_partition.h>
>> +#include <grub/i386/pc/boot.h>
> 
> These are better just put at the top with the rest of the includes. I
> don't think they need to be ifdef'd, I wouldn't complain if they were
> either.
> 
>> +
>> +/* Helper for iterate_device. */
>> +static char *
>> +gpt_guid_to_str (grub_gpt_part_guid_t *gpt_guid)
>> +{
>> +  /* Convert mixed-endian UUID from bytes to string */
>> +  return
>> +    grub_xasprintf (
>> +      "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
>> +	grub_le_to_cpu32(gpt_guid->data1),
>> +	grub_le_to_cpu16(gpt_guid->data2),
>> +	grub_le_to_cpu16(gpt_guid->data3),
>> +	gpt_guid->data4[0], gpt_guid->data4[1],
>> +	gpt_guid->data4[2], gpt_guid->data4[3],
>> +	gpt_guid->data4[4], gpt_guid->data4[5],
>> +	gpt_guid->data4[6], gpt_guid->data4[7]);
> 
> I think its better to do the compare with dashes stripped out of both.
> Or even better to use grub_uuidcascmp (which comes from my patch
> series[1] that I hope to get accepted at some point).
> 

If you want to compare without dashes, I think it'd make more sense to 
convert the UUID into an array of 2 64-bit numbers.  The resultant 
comparisons would be much more efficient than a string compare. 
Additionally, the conversion might be slightly less ugly than converting 
to a string.

grub_uint64_t uuid[2] = {
     grub_le_to_cpu32(gpt_guid->data1) << 32 |
     grub_le_to_cpu16(gpt_guid->data2) << 16 |
     grub_le_to_cpu16(gpt_guid->data2),
     gpt_guid->data4[0] << 56 | gpt_guid->data4[1] << 48 |
     gpt_guid->data4[2] << 40 | gpt_guid->data4[3] << 32 |
     gpt_guid->data4[4] << 24 | gpt_guid->data4[5] << 16 |
     gpt_guid->data4[6] <<  8 | gpt_guid->data4[7]

};

>> +}
>> +#endif
>> +
>>   /* Helper for FUNC_NAME.  */
>>   static int
>>   iterate_device (const char *name, void *data)
>> @@ -66,13 +88,63 @@ iterate_device (const char *name, void *data)
>>         name[0] == 'f' && name[1] == 'd' && name[2] >= '0' && name[2] <= '9')
>>       return 1;
>>   
>> -#ifdef DO_SEARCH_FS_UUID
>> +#if defined (DO_SEARCH_FS_UUID) || defined (DO_SEARCH_PARTUUID)
>>   #define compare_fn grub_strcasecmp
>>   #else
>>   #define compare_fn grub_strcmp
>>   #endif
>>   
>> -#ifdef DO_SEARCH_FILE
>> +#ifdef DO_SEARCH_PARTUUID
>> +    {
>> +      struct grub_gpt_partentry gptdata;
>> +      grub_uint32_t disk_sig;
>> +      grub_disk_t ptdisk;
>> +      grub_disk_t disk;
>> +      char *part_uuid;
>> +
>> +      ptdisk = grub_disk_open (name);
>> +      if (ptdisk && ptdisk->partition && ptdisk->partition->partmap)
>> +	{
>> +	  if (grub_strcmp (ptdisk->partition->partmap->name, "gpt") == 0)
>> +	    {
>> +	      disk = grub_disk_open (ptdisk->name);
>> +	      if (disk && grub_disk_read (disk, ptdisk->partition->offset,
>> +					  ptdisk->partition->index,
>> +					  sizeof (gptdata), &gptdata) == 0)
>> +		{
>> +		  part_uuid = gpt_guid_to_str (&gptdata.guid);
>> +
>> +		  if (part_uuid && compare_fn (part_uuid, ctx->key) == 0)
>> +		    found = 1;
>> +		  if (part_uuid)
>> +		    grub_free (part_uuid);
>> +		}
>> +	      if (disk)
>> +		grub_disk_close (disk);
>> +	    }
>> +	  else if (grub_strcmp (ptdisk->partition->partmap->name, "msdos") == 0)
>> +	    {
>> +	      disk = grub_disk_open (ptdisk->name);
>> +	      if (disk && grub_disk_read (disk, 0,
>> +					  GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
>> +					  sizeof(disk_sig), &disk_sig) == 0)
>> +		{
>> +		  part_uuid = grub_xasprintf ("%08x-%02x",
>> +					      grub_le_to_cpu32(disk_sig),
>> +					      ptdisk->partition->index + 1);
> 
> Maybe strip off the dashes for the compare here too.
> 

Same suggestion here, but the top 88 bits would always be 0.

Regards,
Nicholas Vinson

>> +		  if (part_uuid && compare_fn (part_uuid, ctx->key) == 0)
>> +		    found = 1;
>> +		  if (part_uuid)
>> +		    grub_free (part_uuid);
>> +		}
>> +	      if (disk)
>> +		grub_disk_close (disk);
>> +	    }
>> +	}
>> +      if (ptdisk)
>> +	grub_disk_close (ptdisk);
>> +    }
>> +#elif defined (DO_SEARCH_FILE)
>>       {
>>         char *buf;
>>         grub_file_t file;
>> @@ -313,6 +385,8 @@ static grub_command_t cmd;
>>   
>>   #ifdef DO_SEARCH_FILE
>>   GRUB_MOD_INIT(search_fs_file)
>> +#elif defined (DO_SEARCH_PARTUUID)
>> +GRUB_MOD_INIT(search_partuuid)
>>   #elif defined (DO_SEARCH_FS_UUID)
>>   GRUB_MOD_INIT(search_fs_uuid)
>>   #else
>> @@ -327,6 +401,8 @@ GRUB_MOD_INIT(search_label)
>>   
>>   #ifdef DO_SEARCH_FILE
>>   GRUB_MOD_FINI(search_fs_file)
>> +#elif defined (DO_SEARCH_PARTUUID)
>> +GRUB_MOD_FINI(search_partuuid)
>>   #elif defined (DO_SEARCH_FS_UUID)
>>   GRUB_MOD_FINI(search_fs_uuid)
>>   #else
>> diff --git a/grub-core/commands/search_partuuid.c b/grub-core/commands/search_partuuid.c
>> new file mode 100644
>> index 000000000..e4aa20b5f
>> --- /dev/null
>> +++ b/grub-core/commands/search_partuuid.c
>> @@ -0,0 +1,5 @@
>> +#define DO_SEARCH_PARTUUID 1
>> +#define FUNC_NAME grub_search_partuuid
>> +#define COMMAND_NAME "search.partuuid"
>> +#define HELP_MESSAGE N_("Search devices by PARTUUID. If VARIABLE is specified, the first device found is set to a variable.")
>> +#include "search.c"
>> diff --git a/grub-core/commands/search_wrap.c b/grub-core/commands/search_wrap.c
>> index 47fc8eb99..f516b584e 100644
>> --- a/grub-core/commands/search_wrap.c
>> +++ b/grub-core/commands/search_wrap.c
>> @@ -36,6 +36,8 @@ static const struct grub_arg_option options[] =
>>        0, 0},
>>       {"fs-uuid",		'u', 0, N_("Search devices by a filesystem UUID."),
>>        0, 0},
>> +    {"partuuid",	'p', 0, N_("Search devices by a PARTUUID."),
> 
> s/PARTUUID/partition UUID/
> 
>> +     0, 0},
>>       {"set",		's', GRUB_ARG_OPTION_OPTIONAL,
>>        N_("Set a variable to the first device found."), N_("VARNAME"),
>>        ARG_TYPE_STRING},
>> @@ -71,6 +73,7 @@ enum options
>>       SEARCH_FILE,
>>       SEARCH_LABEL,
>>       SEARCH_FS_UUID,
>> +    SEARCH_PARTUUID,
>>       SEARCH_SET,
>>       SEARCH_NO_FLOPPY,
>>       SEARCH_HINT,
>> @@ -186,6 +189,9 @@ grub_cmd_search (grub_extcmd_context_t ctxt, int argc, char **args)
>>     else if (state[SEARCH_FS_UUID].set)
>>       grub_search_fs_uuid (id, var, state[SEARCH_NO_FLOPPY].set,
>>   			 hints, nhints);
>> +  else if (state[SEARCH_PARTUUID].set)
>> +    grub_search_partuuid (id, var, state[SEARCH_NO_FLOPPY].set,
>> +			  hints, nhints);
>>     else if (state[SEARCH_FILE].set)
>>       grub_search_fs_file (id, var, state[SEARCH_NO_FLOPPY].set,
>>   			 hints, nhints);
>> @@ -206,8 +212,8 @@ GRUB_MOD_INIT(search)
>>   			  GRUB_COMMAND_FLAG_EXTRACTOR | GRUB_COMMAND_ACCEPT_DASH,
>>   			  N_("[-f|-l|-u|-s|-n] [--hint HINT [--hint HINT] ...]"
>>   			     " NAME"),
>> -			  N_("Search devices by file, filesystem label"
>> -			     " or filesystem UUID."
>> +			  N_("Search devices by file, filesystem label,"
>> +			     " PARTUUID or filesystem UUID."
> 
> s/PARTUUID/partition UUID/
> 
>>   			     " If --set is specified, the first device found is"
>>   			     " set to a variable. If no variable name is"
>>   			     " specified, `root' is used."),
>> diff --git a/include/grub/search.h b/include/grub/search.h
>> index d80347df3..7755645f6 100644
>> --- a/include/grub/search.h
>> +++ b/include/grub/search.h
>> @@ -25,5 +25,7 @@ void grub_search_fs_uuid (const char *key, const char *var, int no_floppy,
>>   			  char **hints, unsigned nhints);
>>   void grub_search_label (const char *key, const char *var, int no_floppy,
>>   			char **hints, unsigned nhints);
>> +void grub_search_partuuid (const char *key, const char *var, int no_floppy,
>> +			   char **hints, unsigned nhints);
>>   
>>   #endif
> 
> Glenn
> 
> [1] https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00290.html
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel