Re: [PATCH] Save/Load environment variable support

[Robert Millan]

On Fri, Jun 13, 2008 at 03:07:54PM +0800, Bean wrote:
>  # Misc.
> -pkglib_MODULES += gzio.mod elf.mod
> +pkglib_MODULES += gzio.mod elf.mod findroot.mod
>  
> [...]
> +# For findroot.mod.
> +findroot_mod_SOURCES = kern/findroot.c
> +findroot_mod_CFLAGS = $(COMMON_CFLAGS)
> +findroot_mod_LDFLAGS = $(COMMON_LDFLAGS)

Some findroot bits were mixed up in the patch.

> +/* Names of important environment variables.  */
> +#define GRUB_ENVBLK_RDIR     "rdir"
> +#define GRUB_ENVBLK_UUID     "uuid"
> +#define GRUB_ENVBLK_LABEL    "label"
> +#define GRUB_ENVBLK_IDFILE   "idfile"

I'd prefer if the envblk part was agnostic about which variables have a
meaning.

> +++ b/util/envblk.c
> @@ -0,0 +1,171 @@
> +/* envblk.c - Common function for environment block.  */
> [...]
> +
> +#ifdef GRUB_UTIL

Files that are meant to be used both by grub and util are usually put in
the non-util dir (I'm not sure which non-util dir would fit, though, as it
depends on what you want to do next).

> +#include <string.h>
> +
> +#define grub_strlen  strlen
> +#define grub_strcpy  strcpy
> +#define grub_strchr  strchr
> +#define grub_memcmp  memcmp
> +#define grub_memcpy  memcpy

Uhm can we avoid this?  The rest of non-util code just calls the grub_*
functions (linking with kern/misc.c in this case).

> +grub_envblk_t
> +grub_envblk_find (char *buf)
> +{
> +  grub_uint32_t *pd;
> +  int len;
> +
> +  pd = (grub_uint32_t *) buf;
> +
> +  for (len = GRUB_ENVBLK_MAXLEN - 6; len > 0; len -= 4, pd++)
> +    if (*pd == GRUB_ENVBLK_SIGNATURE)

I wonder if this would be dangerous when run on files where env block
presence is optional, like core.img (though we can always think about this
later when that option is added).

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)