Re: [PATCH v2 2/3] grub: Make grub_envblk_iterate() return an int

[Daniel Kiper]

On Wed, Jan 16, 2019 at 01:34:42PM -0500, Prarit Bhargava wrote:
> grub_envblk_iterate() is a void.  Future functions will require the
> ability to interpret return codes from the iteration, so
> grub_envblk_iterate() should be an int.
>
> The value of 0 returned from the hook functions is overloaded and cannot
> be parsed by callers to grub_envblk_iterate() for error handling.  To fix
> this add GRUB_ERR_ITERATE_STOP and GRUB_ERR_ITERATE_CONTINUE to the error
> returns so the iteration can be stopped and continued respectively.
>
> Make grub_envblk_iterate() return an int.
>
> Signed-off-by: Prarit Bhargava 

I do not like this patch. First of all GRUB errors are defined as an enum
not as an int. The second: do not add error codes if they are not really
needed. And yours are not needed at all.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 2/3] grub: Make grub_envblk_iterate() return an int

[Prarit Bhargava]

grub_envblk_iterate() is a void.  Future functions will require the
ability to interpret return codes from the iteration, so
grub_envblk_iterate() should be an int.

The value of 0 returned from the hook functions is overloaded and cannot
be parsed by callers to grub_envblk_iterate() for error handling.  To fix
this add GRUB_ERR_ITERATE_STOP and GRUB_ERR_ITERATE_CONTINUE to the error
returns so the iteration can be stopped and continued respectively.

Make grub_envblk_iterate() return an int.

Signed-off-by: Prarit Bhargava 
Cc: mleit...@redhat.com
Cc: pjo...@redhat.com
Cc: javi...@redhat.com
Cc: ar...@redhat.com
Cc: Daniel Kiper 
---
 grub-core/commands/loadenv.c |  6 +++---
 grub-core/lib/envblk.c   | 17 -
 include/grub/err.h   |  4 +++-
 include/grub/lib/envblk.h|  6 +++---
 util/grub-editenv.c  |  2 +-
 5 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index 3fd664aac331..1bbae5f7f332 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -144,14 +144,14 @@ set_var (const char *name, const char *value, void 
*whitelist)
   if (! whitelist)
 {
   grub_env_set (name, value);
-  return 0;
+  return GRUB_ERR_ITERATE_CONTINUE;
 }
 
   if (test_whitelist_membership (name,
 (const grub_env_whitelist_t *) whitelist))
 grub_env_set (name, value);
 
-  return 0;
+  return GRUB_ERR_ITERATE_CONTINUE;
 }
 
 static grub_err_t
@@ -192,7 +192,7 @@ print_var (const char *name, const char *value,
void *hook_data __attribute__ ((unused)))
 {
   grub_printf ("%s=%s\n", name, value);
-  return 0;
+  return GRUB_ERR_ITERATE_CONTINUE;
 }
 
 static grub_err_t
diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
index 230e0e9d9abe..3e43843c4535 100644
--- a/grub-core/lib/envblk.c
+++ b/grub-core/lib/envblk.c
@@ -223,7 +223,7 @@ grub_envblk_delete (grub_envblk_t envblk, const char *name)
 }
 }
 
-void
+int
 grub_envblk_iterate (grub_envblk_t envblk,
  void *hook_data,
  int hook (const char *name, const char *value, void 
*hook_data))
@@ -247,8 +247,7 @@ grub_envblk_iterate (grub_envblk_t envblk,
   while (p < pend && *p != '=')
 p++;
   if (p == pend)
-/* Broken.  */
-return;
+return GRUB_ERR_OUT_OF_RANGE;
   name_end = p;
 
   p++;
@@ -264,13 +263,11 @@ grub_envblk_iterate (grub_envblk_t envblk,
 }
 
   if (p >= pend)
-/* Broken.  */
-return;
+return GRUB_ERR_OUT_OF_RANGE;
 
   name = grub_malloc (p - name_start + 1);
   if (! name)
-/* out of memory.  */
-return;
+return GRUB_ERR_OUT_OF_MEMORY;
 
   value = name + (value_start - name_start);
 
@@ -288,10 +285,12 @@ grub_envblk_iterate (grub_envblk_t envblk,
 
   ret = hook (name, value, hook_data);
   grub_free (name);
-  if (ret)
-return;
+  if (ret != GRUB_ERR_ITERATE_CONTINUE)
+return ret;
 }
 
   p = find_next_line (p, pend);
 }
+
+return GRUB_ERR_NONE;
 }
diff --git a/include/grub/err.h b/include/grub/err.h
index 1590c688e1d3..75ebb54201e8 100644
--- a/include/grub/err.h
+++ b/include/grub/err.h
@@ -71,7 +71,9 @@ typedef enum
 GRUB_ERR_NET_PACKET_TOO_BIG,
 GRUB_ERR_NET_NO_DOMAIN,
 GRUB_ERR_EOF,
-GRUB_ERR_BAD_SIGNATURE
+GRUB_ERR_BAD_SIGNATURE,
+GRUB_ERR_ITERATE_STOP,
+GRUB_ERR_ITERATE_CONTINUE
   }
 grub_err_t;
 
diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h
index c3e655921709..7637791f4228 100644
--- a/include/grub/lib/envblk.h
+++ b/include/grub/lib/envblk.h
@@ -34,9 +34,9 @@ typedef struct grub_envblk *grub_envblk_t;
 grub_envblk_t grub_envblk_open (char *buf, grub_size_t size);
 int grub_envblk_set (grub_envblk_t envblk, const char *name, const char 
*value);
 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_iterate (grub_envblk_t envblk,
+ void *hook_data,
+ int hook (const char *name, const char *value, void 
*hook_data));
 void grub_envblk_close (grub_envblk_t envblk);
 
 static inline char *
diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index 0b6c69b9688c..5f87b09d924a 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -169,7 +169,7 @@ print_var (const char *varname, const char *value,
void *hook_data __attribute__ ((unused)))
 {
   printf ("%s=%s\n", varname, value);
-  return 0;
+  return GRUB_ERR_ITERATE_CONTINUE;
 }
 
 static void
-- 
2.17.2


___