[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] commands/search: Add support to search by PARTUUID
|
From: |
Glenn Washburn |
|
Subject: |
Re: [PATCH v3] commands/search: Add support to search by PARTUUID |
|
Date: |
Wed, 9 Feb 2022 18:55:59 -0600 |
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