Re: [PATCH] Save/Load environment variable support

[Bean]

On Sun, Jun 15, 2008 at 3:30 AM, Robert Millan <address@hidden> wrote:
> 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.

Oh, I forget to remove them.

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

I fount out that if I use grub_*, I end up adding quite a few of extra
modules that serve no purpose other than string manipulation. Perhaps
we should separate those routines from kern/misc.c ?

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

Yes, perhaps we should store a pointer to environment block so that we
don't need to scan for it.

-- 
Bean