Re: [PATCH 1/1] search: Add search by GPT partition type

[Daniel Kiper]

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 <address@hidden>
>
> Add a new search.fs_type tool to search by GPT partition type UUID.
>
> Signed-off-by: Carlo Caione <address@hidden>
> Signed-off-by: Will Thompson <address@hidden>
> ---
>  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 @
> - address@hidden|@option{--label}|@option{--fs-uuid}] @
> + address@hidden|@option{--label}|@option{--fs-uuid}|@option{--fs-type}] @
>   address@hidden [var]] address@hidden name
>  Search devices by file (@option{-f}, @option{--file}), filesystem label
> -(@option{-l}, @option{--label}), or filesystem UUID (@option{-u},
> address@hidden).
> +(@option{-l}, @option{--label}), filesystem UUID (@option{-u},
> address@hidden), or filesystem type UUID (@option{-t},
> address@hidden).
>
>  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
> address@hidden --fs-uuid} respectively.
> +The @samp{search.file}, @samp{search.fs_label}, @samp{search.fs_uuid}, and
> address@hidden commands are aliases for @samp{search --file},
> address@hidden --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