variable hooks & global variables

variable hooks & global variables

[Robert Millan]

When you set a variable hook (grub_register_variable_hook), this hook isn't
preserved after someone (e.g. configfile command) opens a new context
(grub_env_context_open), unless the variable has been set as global
(grub_env_export).

Is this what we want?

The only current user of variable hooks is "root" variable, and that hook
contains a sanity check that seems to be more suitable for global scope.

The color-related variables for which I wanted to add hooks would also
like to keep their hooks across contexts.

One option is to export these variables, or to modify grub_env_context_open()
to preserve hooks as well as exported variables.  I'm more inclined for the
latter.

Comments?

-- 
Robert Millan

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

Re: variable hooks & global variables

[Robert Millan]

On Thu, Jan 03, 2008 at 04:03:11PM +0100, Robert Millan wrote:
> 
> When you set a variable hook (grub_register_variable_hook), this hook isn't
> preserved after someone (e.g. configfile command) opens a new context
> (grub_env_context_open), unless the variable has been set as global
> (grub_env_export).
> 
> Is this what we want?
> 
> The only current user of variable hooks is "root" variable, and that hook
> contains a sanity check that seems to be more suitable for global scope.
> 
> The color-related variables for which I wanted to add hooks would also
> like to keep their hooks across contexts.
> 
> One option is to export these variables, or to modify grub_env_context_open()
> to preserve hooks as well as exported variables.  I'm more inclined for the
> latter.
> 
> Comments?

Erm, ignore the part about global variables.  Exporting them doesn't help:

      for (var = context->prev->vars[i]; var; var = var->next)
        {
          if (var->type == GRUB_ENV_VAR_GLOBAL)
            if (grub_env_set (var->name, var->value) != GRUB_ERR_NONE)
              {
                grub_env_context_close ();
                return grub_errno;
              }
        }

So, we just preserve hooks ?

-- 
Robert Millan

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

Re: variable hooks & global variables

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]


I propose this patch to preserve hooks only when variables are already
marked as global.

Additionally, it exports "root" variable.

On Thu, Jan 03, 2008 at 04:05:58PM +0100, Robert Millan wrote:
> On Thu, Jan 03, 2008 at 04:03:11PM +0100, Robert Millan wrote:
> > 
> > When you set a variable hook (grub_register_variable_hook), this hook isn't
> > preserved after someone (e.g. configfile command) opens a new context
> > (grub_env_context_open), unless the variable has been set as global
> > (grub_env_export).
> > 
> > Is this what we want?
> > 
> > The only current user of variable hooks is "root" variable, and that hook
> > contains a sanity check that seems to be more suitable for global scope.
> > 
> > The color-related variables for which I wanted to add hooks would also
> > like to keep their hooks across contexts.
> > 
> > One option is to export these variables, or to modify grub_env_context_open()
> > to preserve hooks as well as exported variables.  I'm more inclined for the
> > latter.
> > 
> > Comments?
> 
> Erm, ignore the part about global variables.  Exporting them doesn't help:
> 
>       for (var = context->prev->vars[i]; var; var = var->next)
>         {
>           if (var->type == GRUB_ENV_VAR_GLOBAL)
>             if (grub_env_set (var->name, var->value) != GRUB_ERR_NONE)
>               {
>                 grub_env_context_close ();
>                 return grub_errno;
>               }
>         }
> 
> So, we just preserve hooks ?
> 
> -- 
> Robert Millan
> 
> <GPLv2> I know my rights; I want my phone call!
> <DRM> What use is a phone call, if you are unable to speak?
> (as seen on /.)

-- 
Robert Millan

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

[-- Attachment #2: context.diff --]
[-- Type: text/x-diff, Size: 2061 bytes --]


	* kern/env.c (grub_env_context_open): Propagate hooks for global
	variables to new context.

	* kern/main.c (grub_set_root_dev): Export `root' variable.

diff -x '*~' -x '*.mk' -Nurp grub2/kern/env.c grub2.color/kern/env.c
--- grub2/kern/env.c	2007-12-30 09:52:04.000000000 +0100
+++ grub2.color/kern/env.c	2008-01-03 16:13:20.000000000 +0100
@@ -1,7 +1,7 @@
 /* env.c - Environment variables */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2003,2005,2006,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2003,2005,2006,2007,2008  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -96,11 +96,14 @@ grub_env_context_open (void)
       for (var = context->prev->vars[i]; var; var = var->next)
 	{
 	  if (var->type == GRUB_ENV_VAR_GLOBAL)
-	    if (grub_env_set (var->name, var->value) != GRUB_ERR_NONE)
-	      {
-		grub_env_context_close ();
-		return grub_errno;
-	      }
+	    {
+	      if (grub_env_set (var->name, var->value) != GRUB_ERR_NONE)
+		{
+		  grub_env_context_close ();
+		  return grub_errno;
+		}
+	      grub_register_variable_hook (var->name, var->read_hook, var->write_hook);
+	    }
 	}
     }
   
diff -x '*~' -x '*.mk' -Nurp grub2/kern/main.c grub2.color/kern/main.c
--- grub2/kern/main.c	2007-07-22 01:32:26.000000000 +0200
+++ grub2.color/kern/main.c	2008-01-03 16:29:19.000000000 +0100
@@ -1,7 +1,7 @@
 /* main.c - the kernel main routine */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2006,2008  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -78,6 +78,7 @@ grub_set_root_dev (void)
   const char *prefix;
 
   grub_register_variable_hook ("root", 0, grub_env_write_root);
+  grub_env_export ("root");
   
   prefix = grub_env_get ("prefix");
   

Re: variable hooks & global variables

[Yoshinori K. Okuji]

On Thursday 03 January 2008 16:05, Robert Millan wrote:
> On Thu, Jan 03, 2008 at 04:03:11PM +0100, Robert Millan wrote:
> > When you set a variable hook (grub_register_variable_hook), this hook
> > isn't preserved after someone (e.g. configfile command) opens a new
> > context (grub_env_context_open), unless the variable has been set as
> > global (grub_env_export).
> >
> > Is this what we want?
> >
> > The only current user of variable hooks is "root" variable, and that hook
> > contains a sanity check that seems to be more suitable for global scope.
> >
> > The color-related variables for which I wanted to add hooks would also
> > like to keep their hooks across contexts.
> >
> > One option is to export these variables, or to modify
> > grub_env_context_open() to preserve hooks as well as exported variables. 
> > I'm more inclined for the latter.
> >
> > Comments?
>
> Erm, ignore the part about global variables.  Exporting them doesn't help:
>
>       for (var = context->prev->vars[i]; var; var = var->next)
>         {
>           if (var->type == GRUB_ENV_VAR_GLOBAL)
>             if (grub_env_set (var->name, var->value) != GRUB_ERR_NONE)
>               {
>                 grub_env_context_close ();
>                 return grub_errno;
>               }
>         }
>
> So, we just preserve hooks ?

Global variables should preserve hooks. This is a bug. Local ones should not. 
If you want to have a variable to be inherited, you must make it global.

Okuji

Re: variable hooks & global variables

[Robert Millan]

On Sat, Jan 05, 2008 at 02:22:17AM +0100, Yoshinori K. Okuji wrote:
> On Thursday 03 January 2008 16:05, Robert Millan wrote:
> > On Thu, Jan 03, 2008 at 04:03:11PM +0100, Robert Millan wrote:
> > > When you set a variable hook (grub_register_variable_hook), this hook
> > > isn't preserved after someone (e.g. configfile command) opens a new
> > > context (grub_env_context_open), unless the variable has been set as
> > > global (grub_env_export).
> > >
> > > Is this what we want?
> > >
> > > The only current user of variable hooks is "root" variable, and that hook
> > > contains a sanity check that seems to be more suitable for global scope.
> > >
> > > The color-related variables for which I wanted to add hooks would also
> > > like to keep their hooks across contexts.
> > >
> > > One option is to export these variables, or to modify
> > > grub_env_context_open() to preserve hooks as well as exported variables. 
> > > I'm more inclined for the latter.
> > >
> > > Comments?
> >
> > Erm, ignore the part about global variables.  Exporting them doesn't help:
> >
> >       for (var = context->prev->vars[i]; var; var = var->next)
> >         {
> >           if (var->type == GRUB_ENV_VAR_GLOBAL)
> >             if (grub_env_set (var->name, var->value) != GRUB_ERR_NONE)
> >               {
> >                 grub_env_context_close ();
> >                 return grub_errno;
> >               }
> >         }
> >
> > So, we just preserve hooks ?
> 
> Global variables should preserve hooks. This is a bug.

Fixed then.

> Local ones should not. 
> If you want to have a variable to be inherited, you must make it global.

Ok.

-- 
Robert Millan

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