On Wed, 9 Feb 2022 17:19:30 -0500 Nicholas Vinson <nvinson...@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 <vkuzmic...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
