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

[Glenn Washburn]

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

> +}
> +#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.

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