All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] grub: add grub variable update functionality
@ 2019-01-04 12:53 Prarit Bhargava
  2019-01-11 11:40 ` Prarit Bhargava
  2019-01-14 15:49 ` Daniel Kiper
  0 siblings, 2 replies; 4+ messages in thread
From: Prarit Bhargava @ 2019-01-04 12:53 UTC (permalink / raw)
  To: grub-devel; +Cc: Prarit Bhargava, mleitner, pjones, javierm, aruiz

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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 grub-2.02]# ./grub-editenv - set kernelopts+="newarg"
[10:59 AM root@intel-wildcatpass-07 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 <prarit@redhat.com>
Cc: mleitner@redhat.com
Cc: pjones@redhat.com
Cc: javierm@redhat.com
Cc: aruiz@redhat.com
---
 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)
+{
+  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);
+  if (!pstart)
+  {
+     ret = -1; /* existing entry not found */
+     goto out;
+  }
+  pend = grub_strchr (pstart, '\n');
+  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));
+
+  ostart = grub_strchr (current, '=');
+  ostart++;
+
+  /* create a buffer for the updated entry. */
+  newsize = grub_strlen (ostart) + grub_strlen (add) + 2;
+  new = grub_zalloc (newsize);
+  if (!new)
+    {
+      return -2; /* out of memory */
+      goto out;
+    }
+
+  grub_strcpy (new, ostart);
+  grub_memcpy (new + grub_strlen (new), " ", 1);
+  grub_strcpy (new + grub_strlen (new), add);
+
+  /* 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;
+    }
+
+out:
+  grub_free(new);
+  grub_free(current);
+  return ret;
+}
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) == '+')
+        {
+          add = 1;
+          *(p - 1) = 0;
+        }
       *(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"));
+          break;
+        default:
+          break;
+        }
+      }
 
       argc--;
       argv++;
-- 
2.17.2



^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] grub: add grub variable update functionality
  2019-01-04 12:53 [PATCH] grub: add grub variable update functionality Prarit Bhargava
@ 2019-01-11 11:40 ` Prarit Bhargava
  2019-01-14 15:49 ` Daniel Kiper
  1 sibling, 0 replies; 4+ messages in thread
From: Prarit Bhargava @ 2019-01-11 11:40 UTC (permalink / raw)
  To: grub-devel; +Cc: mleitner, pjones, javierm, aruiz


Just re-pinging on this ...

P.

On 1/4/19 7:53 AM, 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 grub-2.02]# ./grub-editenv - set kernelopts+="newarg"
> [10:59 AM root@intel-wildcatpass-07 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 <prarit@redhat.com>
> Cc: mleitner@redhat.com
> Cc: pjones@redhat.com
> Cc: javierm@redhat.com
> Cc: aruiz@redhat.com
> ---
>  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)
> +{
> +  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);
> +  if (!pstart)
> +  {
> +     ret = -1; /* existing entry not found */
> +     goto out;
> +  }
> +  pend = grub_strchr (pstart, '\n');
> +  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));
> +
> +  ostart = grub_strchr (current, '=');
> +  ostart++;
> +
> +  /* create a buffer for the updated entry. */
> +  newsize = grub_strlen (ostart) + grub_strlen (add) + 2;
> +  new = grub_zalloc (newsize);
> +  if (!new)
> +    {
> +      return -2; /* out of memory */
> +      goto out;
> +    }
> +
> +  grub_strcpy (new, ostart);
> +  grub_memcpy (new + grub_strlen (new), " ", 1);
> +  grub_strcpy (new + grub_strlen (new), add);
> +
> +  /* 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;
> +    }
> +
> +out:
> +  grub_free(new);
> +  grub_free(current);
> +  return ret;
> +}
> 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) == '+')
> +        {
> +          add = 1;
> +          *(p - 1) = 0;
> +        }
>        *(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"));
> +          break;
> +        default:
> +          break;
> +        }
> +      }
>  
>        argc--;
>        argv++;
> 



^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] grub: add grub variable update functionality
  2019-01-04 12:53 [PATCH] grub: add grub variable update functionality Prarit Bhargava
  2019-01-11 11:40 ` Prarit Bhargava
@ 2019-01-14 15:49 ` Daniel Kiper
  2019-01-15 20:39   ` Prarit Bhargava
  1 sibling, 1 reply; 4+ messages in thread
From: Daniel Kiper @ 2019-01-14 15:49 UTC (permalink / raw)
  To: Prarit Bhargava; +Cc: grub-devel, aruiz, mleitner, javierm

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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 grub-2.02]# ./grub-editenv - set kernelopts+="newarg"
> [10:59 AM root@intel-wildcatpass-07 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 <prarit@redhat.com>
> Cc: mleitner@redhat.com
> Cc: pjones@redhat.com
> Cc: javierm@redhat.com
> Cc: aruiz@redhat.com
> ---
>  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


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] grub: add grub variable update functionality
  2019-01-14 15:49 ` Daniel Kiper
@ 2019-01-15 20:39   ` Prarit Bhargava
  0 siblings, 0 replies; 4+ messages in thread
From: Prarit Bhargava @ 2019-01-15 20:39 UTC (permalink / raw)
  To: Daniel Kiper; +Cc: grub-devel, aruiz, mleitner, javierm



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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 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 root@intel-wildcatpass-07 grub-2.02]# ./grub-editenv - set kernelopts+="newarg"
>> [10:59 AM root@intel-wildcatpass-07 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 <prarit@redhat.com>
>> Cc: mleitner@redhat.com
>> Cc: pjones@redhat.com
>> Cc: javierm@redhat.com
>> Cc: aruiz@redhat.com
>> ---
>>  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
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-15 20:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-04 12:53 [PATCH] grub: add grub variable update functionality Prarit Bhargava
2019-01-11 11:40 ` Prarit Bhargava
2019-01-14 15:49 ` Daniel Kiper
2019-01-15 20:39   ` Prarit Bhargava

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.