grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Nicholas Vinson
Subject: Re: [PATCH v3] commands/search: Add support to search by PARTUUID
Date: Wed, 9 Feb 2022 17:19:30 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]