Re: [PATCH 2/4] Add envblk reading/writing functionality to GRUB

[Daniel Kiper]

On Tue, Feb 25, 2020 at 11:26:41AM -0800, Paul Dagnelie wrote:
> We leverage the grub envblk file and fs functions to read from and
> write to the envblk. We also tweak the editenv code by factoring out
> the logic that creates the buffer with the envblk contents so it can
> be reused.

The commit message suggests that we need at least two patches here...

> We also add the grubenv_src variable, which can be used to force grub
> to load the grubenv file from a file or from the envblk even if it
> would not automatically choose to do so.
>
> commit d9e66e27592ad83f4a90d0e231a9ffc679617cae
> Author:     Paul Dagnelie <address@hidden>
> AuthorDate: Mon Feb 24 13:40:55 2020 -0800
> Commit:     Paul Dagnelie <address@hidden>
> CommitDate: Tue Feb 25 10:08:07 2020 -0800
>
>     Use envblk file and fs functions to implement reading the grubenv
> file from special FS regions
> ---
>  grub-core/commands/loadenv.c | 122 
> +++++++++++++++++++++++++++++++++++++------
>  include/grub/lib/envblk.h    |   2 +
>  include/grub/util/install.h  |   3 ++
>  util/editenv.c               |  26 +++++----
>  4 files changed, 129 insertions(+), 24 deletions(-)
>
> diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
> index 3fd664aac..0f20543a3 100644
> --- a/grub-core/commands/loadenv.c
> +++ b/grub-core/commands/loadenv.c
> @@ -79,6 +79,94 @@ open_envblk_file (char *filename,
>    return file;
>  }
>
> +static grub_file_t
> +open_envblk_block (grub_fs_t fs, grub_device_t dev, enum grub_file_type type)
> +{
> +  if (!fs->fs_envblk_open)
> +    return NULL;
> +  return grub_file_envblk_open(fs, dev, type);

Nit but...
  if (fs->fs_envblk_open)
    return grub_file_envblk_open (fs, dev, type);
  return NULL;

> +}
> +
> +static grub_file_t
> +open_envblk (grub_extcmd_context_t ctxt, enum grub_file_type type)
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  grub_file_t file = NULL;
> +  grub_device_t device = NULL;
> +  const char *source;
> +  grub_fs_t fs = NULL;
> +  char *filename = state[0].set ? state[0].arg : NULL;
> +
> +  source = grub_env_get ("grubenv_src");
> +  if (! source || ! grub_strcmp (source, GRUB_ENVBLK_SRCBLK))
> +    {
> +      grub_dprintf ("loadenv", "checking for envblk\n");
> +      char *device_name;
> +      const char *prefix;

Please do not mix variable declarations with the code. The former should
precede the latter.

> +      prefix = grub_env_get ("prefix");
> +      if (! prefix)
> +        {
> +          grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s'
> isn't set"), "prefix");
> +          return NULL;
> +        }
> +
> +      device_name = grub_file_get_device_name (prefix);
> +      if (grub_errno)
> +    return NULL;
> +
> +      device = grub_device_open (device_name);
> +      if (! device)
> +    return NULL;
> +
> +      fs = grub_fs_probe (device);
> +      if (! fs) {
> +        grub_device_close (device);
> +    return NULL;
> +      }
> +
> +      /* We have to reopen the device here because it will be closed
> in grub_fs_probe. */

s/it will be/it was/?

> +      device = grub_device_open (device_name);
> +      grub_free (device_name);
> +      if (! device)
> +    return NULL;
> +
> +    }
> +  if (! source)
> +    {
> +      if (fs->fs_envblk_open)
> +    {
> +      source = GRUB_ENVBLK_SRCBLK;
> +      grub_dprintf ("loadenv", "envblk support detected\n");
> +    }
> +      else
> +    {
> +      source = GRUB_ENVBLK_SRCFILE;
> +      grub_dprintf ("loadenv", "envblk support not detected\n");
> +    }
> +    }
> +
> +  if (! grub_strcmp (source, GRUB_ENVBLK_SRCFILE))
> +    {
> +      if (device)
> +        grub_device_close (device);
> +      file = open_envblk_file (filename, type);
> +
> +      if (! file)
> +    return NULL;
> +    }
> +  else if (! grub_strcmp (source, GRUB_ENVBLK_SRCBLK))
> +    {
> +      file = open_envblk_block (fs, device, type);
> +      if (! file)
> +    {
> +      grub_device_close (device);
> +      return NULL;
> +    }
> +    }
> +  return file;
> +}
> +
>  static grub_envblk_t
>  read_envblk_file (grub_file_t file)
>  {
> @@ -165,11 +253,10 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt,
> int argc, char **args)
>    whitelist.len = argc;
>    whitelist.list = args;
>
> -  /* state[0] is the -f flag; state[1] is the --skip-sig flag */
> -  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
> -               GRUB_FILE_TYPE_LOADENV
> -               | (state[1].set
> -                  ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE));
> +  file = open_envblk (ctxt, GRUB_FILE_TYPE_LOADENV
> +                            | (state[1].set
> +                   ? GRUB_FILE_TYPE_SKIP_SIGNATURE :
> +                   GRUB_FILE_TYPE_NONE));
>    if (! file)
>      return grub_errno;
>
> @@ -204,10 +291,9 @@ grub_cmd_list_env (grub_extcmd_context_t ctxt,
>    grub_file_t file;
>    grub_envblk_t envblk;
>
> -  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
> -               GRUB_FILE_TYPE_LOADENV
> -               | (state[1].set
> -                  ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE));
> +  file = open_envblk (ctxt, GRUB_FILE_TYPE_LOADENV
> +                | (state[1].set
> +                   ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE));
>    if (! file)
>      return grub_errno;
>
> @@ -379,7 +465,6 @@ save_env_read_hook (grub_disk_addr_t sector,
> unsigned offset, unsigned length,
>  static grub_err_t
>  grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
>  {
> -  struct grub_arg_list *state = ctxt->state;
>    grub_file_t file;
>    grub_envblk_t envblk;
>    struct grub_cmd_save_env_ctx ctx = {
> @@ -390,9 +475,8 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int
> argc, char **args)
>    if (! argc)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "no variable is specified");
>
> -  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
> -               GRUB_FILE_TYPE_SAVEENV
> -               | GRUB_FILE_TYPE_SKIP_SIGNATURE);
> +  file = open_envblk (ctxt, GRUB_FILE_TYPE_SAVEENV
> +                | GRUB_FILE_TYPE_SKIP_SIGNATURE);
>    if (! file)
>      return grub_errno;
>
> @@ -409,7 +493,7 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int
> argc, char **args)
>    if (! envblk)
>      goto fail;
>
> -  if (check_blocklists (envblk, ctx.head, file))
> +  if (! grub_file_envblk (file) && check_blocklists (envblk, ctx.head, file))
>      goto fail;
>
>    while (argc)
> @@ -432,7 +516,15 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt,
> int argc, char **args)
>        args++;
>      }
>
> -  write_blocklists (envblk, ctx.head, file);
> +  if (grub_file_envblk (file))
> +    {
> +      grub_file_seek (file, 0);
> +      file->fs->fs_envblk_write (file, grub_envblk_buffer (envblk),
> grub_envblk_size (envblk));
> +    }
> +  else
> +    {
> +      write_blocklists (envblk, ctx.head, file);
> +    }
>
>   fail:
>    if (envblk)
> diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h
> index c3e655921..7bef0c601 100644
> --- a/include/grub/lib/envblk.h
> +++ b/include/grub/lib/envblk.h
> @@ -21,6 +21,8 @@
>
>  #define GRUB_ENVBLK_SIGNATURE    "# GRUB Environment Block\n"
>  #define GRUB_ENVBLK_DEFCFG    "grubenv"
> +#define GRUB_ENVBLK_SRCBLK    "block"

s/GRUB_ENVBLK_SRCBLK/GRUB_ENVBLK_SRC_BLK/g

> +#define GRUB_ENVBLK_SRCFILE    "file"

s/GRUB_ENVBLK_SRCFILE/GRUB_ENVBLK_SRC_FILE/g

>  #ifndef ASM_FILE
>
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 2631b1074..2ca349503 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -246,6 +246,9 @@ grub_install_get_blocklist (grub_device_t root_dev,
>                            void *data),
>                  void *hook_data);
>
> +void
> +grub_util_create_envblk_buffer (char *, size_t);
> +
>  void
>  grub_util_create_envblk_file (const char *name);
>
> diff --git a/util/editenv.c b/util/editenv.c
> index 81f68bd10..45aeba259 100644
> --- a/util/editenv.c
> +++ b/util/editenv.c
> @@ -32,13 +32,29 @@
>  #define DEFAULT_ENVBLK_SIZE    1024
>  #define GRUB_ENVBLK_MESSAGE    "# WARNING: Do not edit this file by
> tools other than "PACKAGE"-editenv!!!\n"
>
> +void
> +grub_util_create_envblk_buffer (char *buf, size_t size)
> +{
> +  if (size < DEFAULT_ENVBLK_SIZE)
> +    grub_util_error (_("Envblock buffer too small"));
> +  char *pbuf;
> +  pbuf = buf;
> +  memcpy (pbuf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
> +  pbuf += sizeof (GRUB_ENVBLK_SIGNATURE) - 1;
> +  memcpy (pbuf, GRUB_ENVBLK_MESSAGE, sizeof (GRUB_ENVBLK_MESSAGE) - 1);
> +  pbuf += sizeof (GRUB_ENVBLK_MESSAGE) - 1;
> +  memset (pbuf , '#',
> +          size - sizeof (GRUB_ENVBLK_SIGNATURE) - sizeof
> (GRUB_ENVBLK_MESSAGE) + 2);
> +}
> +
>  void
>  grub_util_create_envblk_file (const char *name)
>  {
>    FILE *fp;
> -  char *buf, *pbuf, *namenew;
> +  char *buf, *namenew;
>
>    buf = xmalloc (DEFAULT_ENVBLK_SIZE);
> +  grub_util_create_envblk_buffer(buf, DEFAULT_ENVBLK_SIZE);
>
>    namenew = xasprintf ("%s.new", name);
>    fp = grub_util_fopen (namenew, "wb");
> @@ -46,14 +62,6 @@ grub_util_create_envblk_file (const char *name)
>      grub_util_error (_("cannot open `%s': %s"), namenew,
>               strerror (errno));
>
> -  pbuf = buf;
> -  memcpy (pbuf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
> -  pbuf += sizeof (GRUB_ENVBLK_SIGNATURE) - 1;
> -  memcpy (pbuf, GRUB_ENVBLK_MESSAGE, sizeof (GRUB_ENVBLK_MESSAGE) - 1);
> -  pbuf += sizeof (GRUB_ENVBLK_MESSAGE) - 1;
> -  memset (pbuf , '#',
> -          DEFAULT_ENVBLK_SIZE - sizeof (GRUB_ENVBLK_SIGNATURE) -
> sizeof (GRUB_ENVBLK_MESSAGE) + 2);
> -
>    if (fwrite (buf, 1, DEFAULT_ENVBLK_SIZE, fp) != DEFAULT_ENVBLK_SIZE)
>      grub_util_error (_("cannot write to `%s': %s"), namenew,
>               strerror (errno));

Please provide util changes in separate patches.

Daniel