Re: [PATCH] grub: add grub variable update functionality

[Daniel Kiper]

On Fri, Jan 04, 2019 at 07:53:42AM -0500, Prarit Bhargava wrote:
> Please be aware I am NOT subscribed to grub-devel.
>
> P.
>
> ---8<---
>
> Customers and users of the kernel are commenting that there is no way to 
> update
> a grub variable without copy and pasting the existing data.
>
> For example,
>
> [10:57 AM address@hidden grub-2.02]# ./grub-editenv list
> saved_entry=0
> next_entry=
> kernelopts=root=/dev/mapper/rhel_intel--wildcatpass--07-root ro 
> crashkernel=auto resume=/dev/mapper/rhel_intel--wildcatpass--07-swap 
> rd.lvm.lv=rhel_intel-wildcatpass-07/root 
> rd.lvm.lv=rhel_intel-wildcatpass-07/swap console=ttyS0,115200n81  
> ignore_loglevel
> [10:57 AM address@hidden grub-2.02]# ./grub-editenv - set 
> kernelopts="root=/dev/mapper/rhel_intel--wildcatpass--07-root ro 
> crashkernel=auto resume=/dev/mapper/rhel_intel--wildcatpass--07-swap 
> rd.lvm.lv=rhel_intel-wildcatpass-07/root 
> rd.lvm.lv=rhel_intel-wildcatpass-07/swap console=ttyS0,115200n81  
> ignore_loglevel newarg"
> [10:57 AM address@hidden grub-2.02]# ./grub-editenv list
> saved_entry=0
> next_entry=
> kernelopts=root=/dev/mapper/rhel_intel--wildcatpass--07-root ro 
> crashkernel=auto resume=/dev/mapper/rhel_intel--wildcatpass--07-swap 
> rd.lvm.lv=rhel_intel-wildcatpass-07/root 
> rd.lvm.lv=rhel_intel-wildcatpass-07/swap console=ttyS0,115200n81  
> ignore_loglevel newarg
>
> which is cumbersome.
>
> Add functionality to add to an existing variable.  For example,
>
> [10:58 AM address@hidden grub-2.02]# ./grub-editenv list
> saved_entry=0
> next_entry=
> kernelopts=root=/dev/mapper/rhel_intel--wildcatpass--07-root ro 
> crashkernel=auto resume=/dev/mapper/rhel_intel--wildcatpass--07-swap 
> rd.lvm.lv=rhel_intel-wildcatpass-07/root 
> rd.lvm.lv=rhel_intel-wildcatpass-07/swap console=ttyS0,115200n81  
> ignore_loglevel
> [10:58 AM address@hidden grub-2.02]# ./grub-editenv - set kernelopts+="newarg"
> [10:59 AM address@hidden grub-2.02]# ./grub-editenv list
> saved_entry=0
> next_entry=
> kernelopts=root=/dev/mapper/rhel_intel--wildcatpass--07-root ro 
> crashkernel=auto resume=/dev/mapper/rhel_intel--wildcatpass--07-swap 
> rd.lvm.lv=rhel_intel-wildcatpass-07/root 
> rd.lvm.lv=rhel_intel-wildcatpass-07/swap console=ttyS0,115200n81  
> ignore_loglevel newarg
>
> Signed-off-by: Prarit Bhargava <address@hidden>
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> ---
>  grub-core/lib/envblk.c    | 61 +++++++++++++++++++++++++++++++++++++++
>  include/grub/lib/envblk.h |  1 +
>  util/grub-editenv.c       | 25 +++++++++++++++-
>  3 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
> index 230e0e9d9abe..8ddbe2e8267e 100644
> --- a/grub-core/lib/envblk.c
> +++ b/grub-core/lib/envblk.c
> @@ -295,3 +295,64 @@ grub_envblk_iterate (grub_envblk_t envblk,
>        p = find_next_line (p, pend);
>      }
>  }
> +
> +int
> +grub_envblk_add (grub_envblk_t envblk, const char *name, const char *add)

s/grub_envblk_add/grub_envblk_extend/?

> +{
> +  char *current, *new, *ostart, *pstart, *pend;
> +  int newsize, ret = 1;
> +
> +  /* get a copy of the existing entry */
> +  pstart = envblk->buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1;
> +  pstart = grub_strstr (pstart, name);

grub_strcmp() instead of grub_strstr()?

Even if not what will happen if e.g. env file contains this line among others:

xyx=kernelopts

> +  if (!pstart)
> +  {
> +     ret = -1; /* existing entry not found */
> +     goto out;

s/out/err/

> +  }
> +  pend = grub_strchr (pstart, '\n');

If grub_strcmp() then you do not need this...

> +  if (!pend || pend > (envblk->buf + envblk->size))
> +  {
> +     ret = -1; /* existing entry not found */
> +     goto out;
> +  }
> +
> +  current = grub_zalloc (pend - pstart + 1);
> +  if (!current)
> +  {
> +     ret = -2; /* out of memory */
> +     goto out;
> +  }
> +  grub_strncpy (current, pstart, (int)(pend - pstart));

Do we need (int) cast here?

> +  ostart = grub_strchr (current, '=');
> +  ostart++;
> +
> +  /* create a buffer for the updated entry. */

Please start all comments with capital letter.

> +  newsize = grub_strlen (ostart) + grub_strlen (add) + 2;
> +  new = grub_zalloc (newsize);

You do not need newsize here. Please drop it.

> +  if (!new)
> +    {
> +      return -2; /* out of memory */

May I ask you to use proper GRUB errors, e.g. GRUB_ERR_OUT_OF_MEMORY here.
Please take a look at include/grub/err.h for more details.

> +      goto out;
> +    }
> +
> +  grub_strcpy (new, ostart);
> +  grub_memcpy (new + grub_strlen (new), " ", 1);
> +  grub_strcpy (new + grub_strlen (new), add);

grub_snprintf() is your friend.

> +  /* erase the current entry */
> +  grub_envblk_delete (envblk, name);
> +  /* add the updated entry */
> +  if (!grub_envblk_set (envblk, name, new))
> +    {
> +      /* restore the original entry.  This should always work */
> +      grub_envblk_set (envblk, name, ostart);
> +      ret = 0;

s/0/GRUB_ERR_NONE/

> +    }
> +
> +out:

One space before label please.

> +  grub_free(new);
> +  grub_free(current);
> +  return ret;

Please add empty line before return.

> +}
> diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h
> index c3e655921709..2a0f09e3435b 100644
> --- a/include/grub/lib/envblk.h
> +++ b/include/grub/lib/envblk.h
> @@ -37,6 +37,7 @@ void grub_envblk_delete (grub_envblk_t envblk, const char 
> *name);
>  void grub_envblk_iterate (grub_envblk_t envblk,
>                            void *hook_data,
>                            int hook (const char *name, const char *value, 
> void *hook_data));
> +int grub_envblk_add(grub_envblk_t envblk, const char *name, const char *add);
>  void grub_envblk_close (grub_envblk_t envblk);
>
>  static inline char *
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index 118e89fe57fe..2df81a20b6bc 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -210,15 +210,38 @@ set_variables (const char *name, int argc, char *argv[])
>    while (argc)
>      {
>        char *p;
> +      int add = 0;
>
>        p = strchr (argv[0], '=');
>        if (! p)
>          grub_util_error (_("invalid parameter %s"), argv[0]);
>
> +      if ( *(p - 1) == '+')

This is not safe. What will happen if "p" points to the first character in 
argv[0]?

> +        {
> +          add = 1;
> +          *(p - 1) = 0;

I am not sure why you need to zero this. Anyway, I think that you do not
need "add" variable and code can be optimized further. IMO you can use
one "if" and "continue". This should suffice to differentiate between
set and add/extend cases.

> +        }
>        *(p++) = 0;
>
> -      if (! grub_envblk_set (envblk, argv[0], p))
> +      if (!add && ! grub_envblk_set (envblk, argv[0], p))
>          grub_util_error ("%s", _("environment block too small"));
> +      else if (add) {
> +        int ret;
> +        ret = grub_envblk_add (envblk, argv[0], p);
> +        switch (ret) {
> +        case (0):
> +          grub_util_error ("%s", _("environment block too small"));
> +          break;
> +        case (-1):
> +          grub_util_error("%s", _("existing entry not found"));
> +          break;
> +        case (-2):
> +          grub_util_error("%s", _("out of memory error"));

Please put "default:" here and drop everything below.

> +          break;
> +        default:
> +          break;
> +        }
> +      }

Daniel