Re: [PATCH] grub2: add support to search for partuuid

2019-05-28 Thread Nick Vinson


On 5/27/19 1:07 AM, Michael Grzeschik wrote:
> With this feature the grub-shell is able to search for partuuid strings.
> This reduces the complexity to find the correct boot partition on
> systems with unspecified device connectivity.
> 
> Signed-off-by: Michael Grzeschik 
> ---
>  grub-core/Makefile.core.def  |  5 +
>  grub-core/commands/search.c  | 25 -
>  grub-core/commands/search_partuuid.c |  5 +
>  grub-core/commands/search_wrap.c |  8 +++-
>  grub-core/partmap/msdos.c|  9 +
>  include/grub/disk.h  |  2 ++
>  6 files changed, 52 insertions(+), 2 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 474a63e68..208feb934 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1070,6 +1070,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..3ac93943b 100644
> --- a/grub-core/commands/search.c
> +++ b/grub-core/commands/search.c
> @@ -72,7 +72,26 @@ iterate_device (const char *name, void *data)
>  #define compare_fn grub_strcmp
>  #endif
>  
> -#ifdef DO_SEARCH_FILE
> +#ifdef (DO_SEARCH_PARTUUID)

Typo here.  DO_SEARCH_PARTUUID should not be wrapped with parentheses.

> +{
> +  grub_disk_t disk;
> +  char *part_uuid;
> +  char *uuid;
> +
> +  disk = grub_disk_open (name);
> +  if (disk && disk->partition)
> + {
> +  part_uuid = grub_xasprintf ("%08x-%02x", disk->nt_disk_sig,
> +  disk->partition->index + 1);
> +  if (compare_fn (part_uuid, ctx->key) == 0)
> + found = 1;
> +  if (part_uuid)
> +grub_free (part_uuid);
> +}
> +  if (disk)
> +grub_disk_close (disk);
> +}

Did you intend to omit GPT support for this command?  Asking because it
would be a bit weird in my opinion to add support for PARTUUIDs but omit
the most commonly used partition scheme that actually supports them.

Thanks,
Nicholas Vinson

> +#elif defined (DO_SEARCH_FILE)
>  {
>char *buf;
>grub_file_t file;
> @@ -313,6 +332,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 +348,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 0..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 d7fd26b94..ba36dbac2 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] ...]"
>" 

Re: [PATCH] grub2: add support to search for partuuid

2019-05-28 Thread Daniel Kiper
CC-ing Vladimir.

On Mon, May 27, 2019 at 10:07:39AM +0200, Michael Grzeschik wrote:
> With this feature the grub-shell is able to search for partuuid strings.
> This reduces the complexity to find the correct boot partition on
> systems with unspecified device connectivity.

What is an usage case for this feature?

> Signed-off-by: Michael Grzeschik 
> ---
>  grub-core/Makefile.core.def  |  5 +
>  grub-core/commands/search.c  | 25 -
>  grub-core/commands/search_partuuid.c |  5 +
>  grub-core/commands/search_wrap.c |  8 +++-
>  grub-core/partmap/msdos.c|  9 +
>  include/grub/disk.h  |  2 ++
>  6 files changed, 52 insertions(+), 2 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 474a63e68..208feb934 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1070,6 +1070,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..3ac93943b 100644
> --- a/grub-core/commands/search.c
> +++ b/grub-core/commands/search.c
> @@ -72,7 +72,26 @@ iterate_device (const char *name, void *data)
>  #define compare_fn grub_strcmp
>  #endif

Should not you "#define compare_fn grub_strcasecmp" a few lines above?

Otherwise LGTM.

Daniel

> -#ifdef DO_SEARCH_FILE
> +#ifdef (DO_SEARCH_PARTUUID)
> +{
> +  grub_disk_t disk;
> +  char *part_uuid;
> +  char *uuid;
> +
> +  disk = grub_disk_open (name);
> +  if (disk && disk->partition)
> + {
> +  part_uuid = grub_xasprintf ("%08x-%02x", disk->nt_disk_sig,
> +  disk->partition->index + 1);
> +  if (compare_fn (part_uuid, ctx->key) == 0)
> + found = 1;
> +  if (part_uuid)
> +grub_free (part_uuid);
> +}
> +  if (disk)
> +grub_disk_close (disk);
> +}
> +#elif defined (DO_SEARCH_FILE)
>  {
>char *buf;
>grub_file_t file;
> @@ -313,6 +332,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 +348,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 0..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 d7fd26b94..ba36dbac2 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."
> +