All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] commands/search: Add support to search by PARTUUID
@ 2021-03-04 19:01 Vitaly Kuzmichev
  2021-04-15 13:59 ` [PATCH v3] " Vitaly Kuzmichev
  0 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuzmichev @ 2021-03-04 19:01 UTC (permalink / raw)
  To: grub-devel

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

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     |  8 ++-
 include/grub/search.h                |  2 +
 5 files changed, 97 insertions(+), 3 deletions(-)
 create mode 100644 grub-core/commands/search_partuuid.c

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>
+
+/* 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]);
+}
+#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);
+		  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..90e7cc9a0 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."),
+     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);
@@ -207,7 +213,7 @@ GRUB_MOD_INIT(search)
 			  N_("[-f|-l|-u|-s|-n] [--hint HINT [--hint HINT] ...]"
 			     " NAME"),
 			  N_("Search devices by file, filesystem label"
-			     " or filesystem UUID."
+			     " PARTUUID or filesystem 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..7ca0b6b03 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
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3] commands/search: Add support to search by PARTUUID
  2021-03-04 19:01 [PATCH v2] commands/search: Add support to search by PARTUUID Vitaly Kuzmichev
@ 2021-04-15 13:59 ` Vitaly Kuzmichev
  2021-04-15 14:51   ` Mihai Moldovan
                     ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Vitaly Kuzmichev @ 2021-04-15 13:59 UTC (permalink / raw)
  To: grub-devel; +Cc: daniel.kiper

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

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>
+
+/* 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]);
+}
+#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);
+		  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."),
+     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."
 			     " 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
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] commands/search: Add support to search by PARTUUID
  2021-04-15 13:59 ` [PATCH v3] " Vitaly Kuzmichev
@ 2021-04-15 14:51   ` Mihai Moldovan
  2022-02-02  2:54   ` Glenn Washburn
  2022-02-09 10:08   ` [PATCH v4] " Vitaly Kuzmichev
  2 siblings, 0 replies; 10+ messages in thread
From: Mihai Moldovan @ 2021-04-15 14:51 UTC (permalink / raw)
  To: grub-devel


[-- Attachment #1.1: Type: text/plain, Size: 741 bytes --]

* On 4/15/21 3:59 PM, Vitaly Kuzmichev via Grub-devel 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.

This is also incredibly useful in case you want to select a specific device in a
RAID1 setup (if supported, for instance Linux swraid with 0.9 or 1.0 metadata).
While it would be safe to read from devices, there currently is no way to point
grub directly to a specific device other than hardcoding it (and that's
dangerous, since the system firmware can remap devices as it sees fit).



Mihai


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] commands/search: Add support to search by PARTUUID
  2021-04-15 13:59 ` [PATCH v3] " Vitaly Kuzmichev
  2021-04-15 14:51   ` Mihai Moldovan
@ 2022-02-02  2:54   ` Glenn Washburn
  2022-02-09 22:19     ` Nicholas Vinson
  2022-02-09 10:08   ` [PATCH v4] " Vitaly Kuzmichev
  2 siblings, 1 reply; 10+ messages in thread
From: Glenn Washburn @ 2022-02-02  2:54 UTC (permalink / raw)
  To: Vitaly Kuzmichev via Grub-devel; +Cc: Vitaly Kuzmichev, daniel.kiper

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4] commands/search: Add support to search by PARTUUID
  2021-04-15 13:59 ` [PATCH v3] " Vitaly Kuzmichev
  2021-04-15 14:51   ` Mihai Moldovan
  2022-02-02  2:54   ` Glenn Washburn
@ 2022-02-09 10:08   ` Vitaly Kuzmichev
  2022-02-09 15:48     ` Glenn Washburn
  2 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuzmichev @ 2022-02-09 10:08 UTC (permalink / raw)
  To: grub-devel; +Cc: daniel.kiper

Improve 'search' grub-shell command with functionality to search for
a partition by PARTUUID string. This is useful for embedded 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, V4:
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

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..9b690ba85 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>
+
+/* Helper for iterate_device. */
+static char *
+convert_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]);
+}
+#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 = convert_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);
+		  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."),
+     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."
 			     " 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
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] commands/search: Add support to search by PARTUUID
  2022-02-09 10:08   ` [PATCH v4] " Vitaly Kuzmichev
@ 2022-02-09 15:48     ` Glenn Washburn
  2022-02-09 16:41       ` Vitaly Kuzmichev
  0 siblings, 1 reply; 10+ messages in thread
From: Glenn Washburn @ 2022-02-09 15:48 UTC (permalink / raw)
  To: Vitaly Kuzmichev via Grub-devel; +Cc: Vitaly Kuzmichev, daniel.kiper

On Wed,  9 Feb 2022 13:08:51 +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 for embedded 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, V4:
> Coding style and spelling fixes.

As far as I can tell, this doesn't address any of my concerns. Is this
correct? You might want to use the --interdiff or --range-diff options
to git format-patch so its easier to see what actually changed between
patch versions.

Glenn

> 
> 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
> 
> 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..9b690ba85 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>
> +
> +/* Helper for iterate_device. */
> +static char *
> +convert_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]);
> +}
> +#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 = convert_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);
> +		  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."),
> +     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."
>  			     " 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] commands/search: Add support to search by PARTUUID
  2022-02-09 15:48     ` Glenn Washburn
@ 2022-02-09 16:41       ` Vitaly Kuzmichev
  2022-02-09 20:23         ` Glenn Washburn
  0 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuzmichev @ 2022-02-09 16:41 UTC (permalink / raw)
  To: development, Vitaly Kuzmichev via Grub-devel; +Cc: daniel.kiper

Hello Glenn,

On 2/9/22 6:48 PM, Glenn Washburn wrote:
> On Wed,  9 Feb 2022 13:08:51 +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 for embedded 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, V4:
>> Coding style and spelling fixes.
> As far as I can tell, this doesn't address any of my concerns. Is this
> correct? You might want to use the --interdiff or --range-diff options
> to git format-patch so its easier to see what actually changed between
> patch versions.
>
> Glenn


Aah, sorry, I totally missed your feedback. Thank you.

I have checked your points, I agree with everything, though I would 
prefer to treat uuids with dashes placed in wrong positions (e.g. 
1122334455-66-77-8899-aabbccdd-eeff00 instead of correct one 
11223344-5566-7788-99aa-bbccddeeff00) as invalid uuids. Anyway I agree 
to let user to split "that weird long string" with dashes wherever he wants.

I will fix and send v5 patch tomorrow.


PS. Yes, --interdiff and --range-diff look useful, but the complete 
version of the patch should be included to the mail anyway, right?


With Best Regards,

Vitaly K.


>
>> 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
>>
>> 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..9b690ba85 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>
>> +
>> +/* Helper for iterate_device. */
>> +static char *
>> +convert_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]);
>> +}
>> +#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 = convert_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);
>> +		  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."),
>> +     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."
>>   			     " 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] commands/search: Add support to search by PARTUUID
  2022-02-09 16:41       ` Vitaly Kuzmichev
@ 2022-02-09 20:23         ` Glenn Washburn
  0 siblings, 0 replies; 10+ messages in thread
From: Glenn Washburn @ 2022-02-09 20:23 UTC (permalink / raw)
  To: Vitaly Kuzmichev; +Cc: Vitaly Kuzmichev via Grub-devel, daniel.kiper

On Wed, 9 Feb 2022 19:41:28 +0300
Vitaly Kuzmichev <vkuzmichev@dev.rtsoft.ru> wrote:

> Hello Glenn,
> 
> On 2/9/22 6:48 PM, Glenn Washburn wrote:
> > On Wed,  9 Feb 2022 13:08:51 +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 for embedded 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, V4:
> >> Coding style and spelling fixes.
> > As far as I can tell, this doesn't address any of my concerns. Is this
> > correct? You might want to use the --interdiff or --range-diff options
> > to git format-patch so its easier to see what actually changed between
> > patch versions.
> >
> > Glenn
> 
> 
> Aah, sorry, I totally missed your feedback. Thank you.
> 
> I have checked your points, I agree with everything, though I would 
> prefer to treat uuids with dashes placed in wrong positions (e.g. 
> 1122334455-66-77-8899-aabbccdd-eeff00 instead of correct one 
> 11223344-5566-7788-99aa-bbccddeeff00) as invalid uuids.

I'm curious why you would want to treat them as invalid uuids? I don't
really see use value for it. Usually the uuids are copy-pasted from
some other program. I prefer not to care how that other program formats
them.

> Anyway I agree 
> to let user to split "that weird long string" with dashes wherever he wants.
> 
> I will fix and send v5 patch tomorrow.

As was mentioned on another thread and not mentioned in my feedback, it
would be more ideal to put the code for extracting UUIDs from GPT and
MBR partition which is here and in probe into a common place. Perhaps
in grub-core/kern/partition.c in a function call
grub_partition_get_uuid (). If the partition does not support uuid, then
return NULL.

Also, the list convention is to have new patch versions on their own
thread. So no need to thread it on to this thread.

> 
> PS. Yes, --interdiff and --range-diff look useful, but the complete 
> version of the patch should be included to the mail anyway, right?

Yes, the complete patch should be sent as well. Those options can show
changes between patch versions in such as way that it is ignored when
importing the patch. Its just helpful for reviewers.

Glenn

> 
> 
> With Best Regards,
> 
> Vitaly K.
> 
> 
> >
> >> 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
> >>
> >> 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..9b690ba85 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>
> >> +
> >> +/* Helper for iterate_device. */
> >> +static char *
> >> +convert_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]);
> >> +}
> >> +#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 = convert_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);
> >> +		  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."),
> >> +     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."
> >>   			     " 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] commands/search: Add support to search by PARTUUID
  2022-02-02  2:54   ` Glenn Washburn
@ 2022-02-09 22:19     ` Nicholas Vinson
  2022-02-10  0:55       ` Glenn Washburn
  0 siblings, 1 reply; 10+ messages in thread
From: Nicholas Vinson @ 2022-02-09 22:19 UTC (permalink / raw)
  To: grub-devel



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] commands/search: Add support to search by PARTUUID
  2022-02-09 22:19     ` Nicholas Vinson
@ 2022-02-10  0:55       ` Glenn Washburn
  0 siblings, 0 replies; 10+ messages in thread
From: Glenn Washburn @ 2022-02-10  0:55 UTC (permalink / raw)
  To: Nicholas Vinson; +Cc: The development of GNU GRUB

On Wed, 9 Feb 2022 17:19:30 -0500
Nicholas Vinson <nvinson234@gmail.com> wrote:

> 
> 
> 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]
> 
> };

This does make sense, since the GUIDs for GPT and MBR are already in
binary. Perhaps create a function like:
  int grub_uuid_to_bin (const char *s, grub_uint64_t *uuid_upper, grub_uint64_t *uuid_lower)

This would convert up to 32 characters ignoring dashes. The other benefit (as I see it), is that this needn't be held up waiting for a library function that compares uuids to be added.

Glenn

> 
> >> +}
> >> +#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
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-02-10  0:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 19:01 [PATCH v2] commands/search: Add support to search by PARTUUID Vitaly Kuzmichev
2021-04-15 13:59 ` [PATCH v3] " Vitaly Kuzmichev
2021-04-15 14:51   ` Mihai Moldovan
2022-02-02  2:54   ` Glenn Washburn
2022-02-09 22:19     ` Nicholas Vinson
2022-02-10  0:55       ` Glenn Washburn
2022-02-09 10:08   ` [PATCH v4] " Vitaly Kuzmichev
2022-02-09 15:48     ` Glenn Washburn
2022-02-09 16:41       ` Vitaly Kuzmichev
2022-02-09 20:23         ` Glenn Washburn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.