Hi Will,
Sorry for huge delay but I was swamped by other stuff.
Anyway, patch LGTM. Just a few nit picks below.
On Wed, Jun 20, 2018 at 04:38:08PM +0100, Will Thompson wrote:
> From: Carlo Caione <ca...@endlessm.com>
>
> Add a new search.fs_type tool to search by GPT partition type UUID.
>
> Signed-off-by: Carlo Caione <ca...@endlessm.com>
> Signed-off-by: Will Thompson <w...@endlessm.com>
> ---
> docs/grub.texi | 14 ++++---
> grub-core/Makefile.core.def | 5 +++
> grub-core/commands/search.c | 69 +++++++++++++++++++++++++++++++-
> grub-core/commands/search_type.c | 5 +++
> grub-core/commands/search_wrap.c | 12 ++++--
> grub-core/partmap/gpt.c | 9 +++++
> include/grub/gpt_partition.h | 9 -----
> include/grub/partition.h | 12 ++++++
> include/grub/search.h | 2 +
> 9 files changed, 118 insertions(+), 19 deletions(-)
> create mode 100644 grub-core/commands/search_type.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 2adfa97be..17b3ff55c 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4849,11 +4849,12 @@ unbootable. @xref{Using digital signatures}, for more
> information.
> @subsection search
>
> @deffn Command search @
> - [@option{--file}|@option{--label}|@option{--fs-uuid}] @
> + [@option{--file}|@option{--label}|@option{--fs-uuid}|@option{--fs-type}] @
> [@option{--set} [var]] [@option{--no-floppy}] name
> Search devices by file (@option{-f}, @option{--file}), filesystem label
> -(@option{-l}, @option{--label}), or filesystem UUID (@option{-u},
> -@option{--fs-uuid}).
> +(@option{-l}, @option{--label}), filesystem UUID (@option{-u},
> +@option{--fs-uuid}), or filesystem type UUID (@option{-t},
> +@option{--fs-type}).
>
> If the @option{--set} option is used, the first device found is set as the
> value of environment variable @var{var}. The default variable is
> @@ -4862,9 +4863,10 @@ value of environment variable @var{var}. The default
> variable is
> The @option{--no-floppy} option prevents searching floppy devices, which can
> be slow.
>
> -The @samp{search.file}, @samp{search.fs_label}, and @samp{search.fs_uuid}
> -commands are aliases for @samp{search --file}, @samp{search --label}, and
> -@samp{search --fs-uuid} respectively.
> +The @samp{search.file}, @samp{search.fs_label}, @samp{search.fs_uuid}, and
> +@samp{search.fs_type} commands are aliases for @samp{search --file},
> +@samp{search --label}, @samp{search --fs-type} and @samp{search --fs-uuid}
> +respectively.
Could not you use the same enumeration order like above?
> @end deffn
>
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index fc4767f19..b6c760bc1 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1017,6 +1017,11 @@ module = {
> common = commands/search_uuid.c;
> };
>
> +module = {
> + name = search_fs_type;
> + common = commands/search_type.c;
> +};
> +
> module = {
> name = search_label;
> common = commands/search_label.c;
> diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
> index 7dd32e445..22820ec47 100644
> --- a/grub-core/commands/search.c
> +++ b/grub-core/commands/search.c
> @@ -52,8 +52,48 @@ struct search_ctx
> unsigned nhints;
> int count;
> int is_cache;
> +#ifdef DO_SEARCH_FS_TYPE
> + char *part;
> +#endif
> };
>
> +#ifdef DO_SEARCH_FS_TYPE
> +static int
> +type_part_hook (grub_disk_t disk, const grub_partition_t partition, void
> *data)
> +{
> + struct search_ctx *ctx = data;
> + char *partition_name, *guid, *dev_name;
> + int found = 0;
> +
> + partition_name = grub_partition_get_name (partition);
> + if (!partition_name)
> + return 0;
> +
> + dev_name = grub_xasprintf ("%s,%s", disk->name, partition_name);
> + grub_free (partition_name);
> +
> + guid = grub_xasprintf ("%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> + grub_le_to_cpu32(partition->gpttype.data1),
> + grub_le_to_cpu16(partition->gpttype.data2),
> + grub_le_to_cpu16(partition->gpttype.data3),
> + (partition->gpttype.data4[0]),
> (partition->gpttype.data4[1]),
> + (partition->gpttype.data4[2]),
> (partition->gpttype.data4[3]),
> + (partition->gpttype.data4[4]),
> (partition->gpttype.data4[5]),
> + (partition->gpttype.data4[6]),
> (partition->gpttype.data4[7]));
> +
> + if (grub_strcasecmp (guid, ctx->key) == 0)
> + {
> + ctx->part = dev_name;
> + found = 1;
> + }
> + else
> + grub_free (dev_name);
> +
> + grub_free (guid);
> + return found;
> +}
> +#endif
> +
> /* Helper for FUNC_NAME. */
> static int
> iterate_device (const char *name, void *data)
> @@ -66,12 +106,27 @@ 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_FS_TYPE)
> #define compare_fn grub_strcasecmp
> #else
> #define compare_fn grub_strcmp
> #endif
>
> +#ifdef DO_SEARCH_FS_TYPE
> + {
> + grub_device_t dev;
> +
> + dev = grub_device_open (name);
> + if (dev)
> + {
> + if (dev->disk)
> + {
> + found = grub_partition_iterate (dev->disk, type_part_hook,
> ctx);
> + }
We do not need curly brackets here.
> + grub_device_close (dev);
> + }
> + }
> +#else
> #ifdef DO_SEARCH_FILE
> {
> char *buf;
> @@ -124,6 +179,7 @@ iterate_device (const char *name, void *data)
> grub_device_close (dev);
> }
> }
> +#endif
> #endif
>
> if (!ctx->is_cache && found && ctx->count == 0)
> @@ -153,11 +209,18 @@ iterate_device (const char *name, void *data)
>
> if (found)
> {
> +#ifdef DO_SEARCH_FS_TYPE
> + name = ctx->part;
> +#endif
> ctx->count++;
> if (ctx->var)
> grub_env_set (ctx->var, name);
> else
> grub_printf (" %s", name);
> +
> +#ifdef DO_SEARCH_FS_TYPE
> + grub_free (ctx->part);
> +#endif
> }
>
> grub_errno = GRUB_ERR_NONE;
> @@ -315,6 +378,8 @@ static grub_command_t cmd;
> GRUB_MOD_INIT(search_fs_file)
> #elif defined (DO_SEARCH_FS_UUID)
> GRUB_MOD_INIT(search_fs_uuid)
> +#elif defined (DO_SEARCH_FS_TYPE)
> +GRUB_MOD_INIT(search_fs_type)
> #else
> GRUB_MOD_INIT(search_label)
> #endif
> @@ -329,6 +394,8 @@ GRUB_MOD_INIT(search_label)
> GRUB_MOD_FINI(search_fs_file)
> #elif defined (DO_SEARCH_FS_UUID)
> GRUB_MOD_FINI(search_fs_uuid)
> +#elif defined (DO_SEARCH_FS_TYPE)
> +GRUB_MOD_FINI(search_fs_type)
> #else
> GRUB_MOD_FINI(search_label)
> #endif
> diff --git a/grub-core/commands/search_type.c
> b/grub-core/commands/search_type.c
> new file mode 100644
> index 000000000..437a55b33
> --- /dev/null
> +++ b/grub-core/commands/search_type.c
> @@ -0,0 +1,5 @@
> +#define DO_SEARCH_FS_TYPE 1
> +#define FUNC_NAME grub_search_fs_type
> +#define COMMAND_NAME "search.fs_type"
> +#define HELP_MESSAGE N_("Search devices by filesystem type. 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 d7fd26b94..ea9c98e65 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},
> + {"fs-type", 't', 0, N_("Search devices by a filesystem
> type."),
> + 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_FS_TYPE,
> 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_FS_TYPE].set)
> + grub_search_fs_type (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);
> @@ -204,10 +210,10 @@ GRUB_MOD_INIT(search)
> cmd =
> grub_register_extcmd ("search", grub_cmd_search,
> GRUB_COMMAND_FLAG_EXTRACTOR |
> GRUB_COMMAND_ACCEPT_DASH,
> - N_("[-f|-l|-u|-s|-n] [--hint HINT [--hint HINT] ...]"
> + N_("[-f|-l|-u|-t|-s|-n] [--hint HINT [--hint HINT]
> ...]"
> " NAME"),
> - N_("Search devices by file, filesystem label"
> - " or filesystem UUID."
> + N_("Search devices by file, filesystem label,"
> + " filesystem UUID or filesystem type."
> " 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/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> index 103f6796f..3b575afad 100644
> --- a/grub-core/partmap/gpt.c
> +++ b/grub-core/partmap/gpt.c
> @@ -109,9 +109,18 @@ grub_gpt_partition_map_iterate (grub_disk_t disk,
> part.partmap = &grub_gpt_partition_map;
> part.parent = disk->partition;
>
> + grub_memcpy(&part.gpttype, &entry.type, sizeof
> (grub_gpt_part_guid_t));
> +
> grub_dprintf ("gpt", "GPT entry %d: start=%lld, length=%lld\n", i,
> (unsigned long long) part.start,
> (unsigned long long) part.len);
> + grub_dprintf ("gpt", " partition type
> GUID=%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x\n",
> + grub_le_to_cpu32(entry.type.data1),
> grub_le_to_cpu16(entry.type.data2),
> + grub_le_to_cpu16(entry.type.data3),
> + (entry.type.data4[0]), (entry.type.data4[1]),
> + (entry.type.data4[2]), (entry.type.data4[3]),
> + (entry.type.data4[4]), (entry.type.data4[5]),
> + (entry.type.data4[6]), (entry.type.data4[7]));
>
> if (hook (disk, &part, hook_data))
> return grub_errno;
> diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h
> index 7a93f4329..75504f3e0 100644
> --- a/include/grub/gpt_partition.h
> +++ b/include/grub/gpt_partition.h
> @@ -22,15 +22,6 @@
> #include <grub/types.h>
> #include <grub/partition.h>
>
> -struct grub_gpt_part_guid
> -{
> - grub_uint32_t data1;
> - grub_uint16_t data2;
> - grub_uint16_t data3;
> - grub_uint8_t data4[8];
> -} GRUB_PACKED;
> -typedef struct grub_gpt_part_guid grub_gpt_part_guid_t;
> -
> #define GRUB_GPT_PARTITION_TYPE_EMPTY \
> { 0x0, 0x0, 0x0, \
> { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } \
> diff --git a/include/grub/partition.h b/include/grub/partition.h
> index 7adb7ec6e..a5783fdc6 100644
> --- a/include/grub/partition.h
> +++ b/include/grub/partition.h
> @@ -60,6 +60,15 @@ struct grub_partition_map
> };
> typedef struct grub_partition_map *grub_partition_map_t;
>
> +struct grub_gpt_part_guid
> +{
> + grub_uint32_t data1;
> + grub_uint16_t data2;
> + grub_uint16_t data3;
> + grub_uint8_t data4[8];
> +} GRUB_PACKED;
> +typedef struct grub_gpt_part_guid grub_gpt_part_guid_t;
> +
Do we really need this code shuffling?
> /* Partition description. */
> struct grub_partition
> {
> @@ -87,6 +96,9 @@ struct grub_partition
> /* The type of partition whne it's on MSDOS.
> Used for embedding detection. */
> grub_uint8_t msdostype;
> +
> + /* The type of partition when it's on GPT. */
> + grub_gpt_part_guid_t gpttype;
OK, this is used here but... I would like to avoid code shuffling if
possible. And it is more natural to have grub_gpt_part_guid in
include/grub/gpt_partition.h
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
