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

[Prarit Bhargava]

On 1/14/19 10:49 AM, Daniel Kiper wrote:
> 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/?

Hi Daniel, thanks for the valuable feedback.

After reading your comments I saw that using grub_envblk_iterate() to modify the
entry was easier than attempting to parse everything myself.  I've added that to
my v2.  Unfortunately this means I'm skipping over some of your comments which
are no longer applicable to v2.

> 
>> +{
>> +  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/

Applied to v2.

> 
>> +  }
>> +  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.

Applied to v2.

> 
>> +  newsize = grub_strlen (ostart) + grub_strlen (add) + 2;
>> +  new = grub_zalloc (newsize);
> 
> You do not need newsize here. Please drop it.

Applied to v2.

> 
>> +  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.
> 

Applied to v2.

>> +      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.

:) Applied to v2.  and "Duh." :)

> 
>> +  /* 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/
> 

Applied to v2.

>> +    }
>> +
>> +out:
> 
> One space before label please.

Applied to v2.

> 
>> +  grub_free(new);
>> +  grub_free(current);
>> +  return ret;
> 
> Please add empty line before return.

Applied to v2.

> 
>> +}
>> 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.

The commandline is

./grub-editenv - set prarit="hello"

which results in

prarit=hello

In this case argv[0] is 'prarit=hello' and we really want to pass only 'prarit'
as the argument.  The simple way this is done in the existing code is to write a
'0' to the equal sign.

Similarly,

./grub-editenv - set prarit+="hello"

I need to write a 0 to the plus sign and the equal sign, ie

          *(p - 1) = 0;
          *(p++) = 0;

While correct it is confusing to look at :).  I'll think of another option for 
v2.

P.

> 
>> +        }
>>        *(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
>