var=foo should be valid?

var=foo should be valid?

[Carles Pina i Estany]

Hello,

Right now in Grub2 a user could do:
name=john
set name=john

both are valid.

(first one has a bug but very easy to fix, even thought I would maybe
refactor this code to implement in other way)

Question that arised in IRC: should be support the first one or not? Two
interfaces for the same thing?

-- 
Carles Pina i Estany
	http://pinux.info

Re: var=foo should be valid?

[Carles Pina i Estany]

Hi,

On Nov/29/2009, Carles Pina i Estany wrote:

> Right now in Grub2 a user could do:
> name=john
> set name=john
> 
> both are valid.
> 
> (first one has a bug but very easy to fix, even thought I would maybe
> refactor this code to implement in other way)
> 
> Question that arised in IRC: should be support the first one or not? Two
> interfaces for the same thing?

my opinion: doesn't matter (a lot)

And because "doesn't matter": we can only accept "set name=john".

So we avoid bugs like it happened now, more simple and users wondering
"if there are two ways means that set is doing something that it's not
happening in name=foo, maybe setting a reference and not copying the
string*? maybe exporting**?".

*: happens in VBA
**: someone could think that if we accept without and with set it's
exporting like export name=foo in Bash

-- 
Carles Pina i Estany
	http://pinux.info

Re: var=foo should be valid?

[Carles Pina i Estany]

Hi,

On Nov/29/2009, Carles Pina i Estany wrote:

> And because "doesn't matter": we can only accept "set name=john".

s/we can/we should/ (more clear)

-- 
Carles Pina i Estany
	http://pinux.info

Re: var=foo should be valid?

[Colin Watson]

On Sun, Nov 29, 2009 at 06:45:03PM +0000, Carles Pina i Estany wrote:
> On Nov/29/2009, Carles Pina i Estany wrote:
> > Right now in Grub2 a user could do:
> > name=john
> > set name=john
> > 
> > both are valid.
> > 
> > (first one has a bug but very easy to fix, even thought I would maybe
> > refactor this code to implement in other way)
> > 
> > Question that arised in IRC: should be support the first one or not? Two
> > interfaces for the same thing?
> 
> my opinion: doesn't matter (a lot)
> 
> And because "doesn't matter": we can only accept "set name=john".

I would appreciate it if we left it as it is now. It is not
intrinsically bad to have both, and GRUB 2 is already deployed in
production so it's quite likely that people will in fact be using both
syntaxes. (Indeed, the configuration file I'm delivering by default in
Ubuntu 9.10 uses both syntaxes! I should probably normalise that but I
simply hadn't thought about it as being any kind of problem.) We should
not break production installations when there is no reason to do so.

(I don't know what the bug you're referring to is; both 'foo=bar' and
'set foo=bar' seem to work OK here. I may be missing some subtle
problem, but if I'm missing it then users will too.)

> **: someone could think that if we accept without and with set it's
> exporting like export name=foo in Bash

If they're familiar with bash then they'll assume that both forms are
the same.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: var=foo should be valid?

[Carles Pina i Estany]

Hello,

On Nov/30/2009, Colin Watson wrote:
> On Sun, Nov 29, 2009 at 06:45:03PM +0000, Carles Pina i Estany wrote:
> > On Nov/29/2009, Carles Pina i Estany wrote:

> (I don't know what the bug you're referring to is; both 'foo=bar' and
> 'set foo=bar' seem to work OK here. I may be missing some subtle
> problem, but if I'm missing it then users will too.)

the problem is easy to fix and I already have the fix ready (either fix
the current the bug or delete the var=foo feature) (well, as I said,
maybe we can re-implement the way that it's implemented now, but we can
discuss it later)

The bug is that if a module hooks into a variable (e.g. gettext does,
grub_register_varaible_hook) and then the user changes the value of the
variable using lang=newlang, the function that gets called is executed
but grub_errno is 16 (I think that was 16). Because I use grub_file_open
and grub_file_open is not working if grub_errno != 0 (no error) when
it's called then the gettext module cannot translate the strings using
the new language.

(I'm talking about the gettext module but potentially can affect other
modules of course)

-- 
Carles Pina i Estany
	http://pinux.info

Re: var=foo should be valid?

[Robert Millan]

On Mon, Nov 30, 2009 at 09:39:50AM +0000, Colin Watson wrote:
> 
> I would appreciate it if we left it as it is now. It is not
> intrinsically bad to have both, and GRUB 2 is already deployed in
> production so it's quite likely that people will in fact be using both
> syntaxes. (Indeed, the configuration file I'm delivering by default in
> Ubuntu 9.10 uses both syntaxes! I should probably normalise that but I
> simply hadn't thought about it as being any kind of problem.) We should
> not break production installations when there is no reason to do so.

Don't worry, we can't promise we'll stick to an interface forever, but
we're careful not to break existing setups.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: var=foo should be valid?

[Robert Millan]

On Sun, Nov 29, 2009 at 06:39:36PM +0000, Carles Pina i Estany wrote:
> (first one has a bug but very easy to fix, even thought I would maybe
> refactor this code to implement in other way)

Can you give more details about this bug and the refactor that would be
needed?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: var=foo should be valid?

[Carles Pina i Estany]

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


Hi,

On Dec/04/2009, Robert Millan wrote:
> On Sun, Nov 29, 2009 at 06:39:36PM +0000, Carles Pina i Estany wrote:
> > (first one has a bug but very easy to fix, even thought I would maybe
> > refactor this code to implement in other way)
> 
> Can you give more details about this bug and the refactor that would be
> needed?

Find attached the simple patch (without refactoring) (it resets
grub_errno _before_ grub_env_set, because grub_env_set calls the hooked
functions).

The refactoring would be in file script/execute.c, function
grub_script_execute_cmdline. See the attached file set_refactor.patch 
for the idea.  It's more work/things in Grub but I think that more 
robust since now will be the same code using set or without using set. 
Of course, maybe this extra-complexity is not needed.

I have not looked into the Grub parser: maybe the extra-best place to
implement it it's there, but I'm not familiar with Lex/Yacc/Bison/... . 

Cheers,

-- 
Carles Pina i Estany
	http://pinux.info

[-- Attachment #2: set_simple.patch --]
[-- Type: text/x-diff, Size: 492 bytes --]

=== modified file 'script/execute.c'
--- script/execute.c	2009-11-23 15:37:33 +0000
+++ script/execute.c	2009-12-04 21:43:28 +0000
@@ -116,10 +116,11 @@ grub_script_execute_cmdline (struct grub
 	      /* Create two strings and set the variable.  */
 	      *eq = '\0';
 	      eq++;
-	      grub_env_set (assign, eq);
 
 	      /* This was set because the command was not found.  */
 	      grub_errno = GRUB_ERR_NONE;
+
+	      grub_env_set (assign, eq);
 	    }
 	  grub_free (assign);
 


[-- Attachment #3: set_refactor.patch --]
[-- Type: text/x-diff, Size: 824 bytes --]

=== modified file 'script/execute.c'
--- script/execute.c	2009-11-23 15:37:33 +0000
+++ script/execute.c	2009-12-04 21:51:41 +0000
@@ -113,13 +113,15 @@ grub_script_execute_cmdline (struct grub
 
 	  if (eq)
 	    {
-	      /* Create two strings and set the variable.  */
-	      *eq = '\0';
-	      eq++;
-	      grub_env_set (assign, eq);
-
-	      /* This was set because the command was not found.  */
-	      grub_errno = GRUB_ERR_NONE;
+	      // TODO
+	      // New Command Line Function = set
+	      // New arguments = Old Command Line Function concatenate Old arguments
+	      // Call grub_script_execute_cmdline (newCmd)
+	      //
+	      // So if before user typed a=b we call recursively:
+	      // set a=b
+	      // 
+	      // Alternatively: Don't need to check for "="
 	    }
 	  grub_free (assign);
 

Re: var=foo should be valid?

[Carles Pina i Estany]

Hi,

On Dec/04/2009, Carles Pina i Estany wrote:
> 
> Hi,
> 
> On Dec/04/2009, Robert Millan wrote:
> > On Sun, Nov 29, 2009 at 06:39:36PM +0000, Carles Pina i Estany wrote:
> > > (first one has a bug but very easy to fix, even thought I would maybe
> > > refactor this code to implement in other way)
> > 
> > Can you give more details about this bug and the refactor that would be
> > needed?
> 
> Find attached the simple patch (without refactoring) (it resets
> grub_errno _before_ grub_env_set, because grub_env_set calls the hooked
> functions).

We should fix it. We have three options (maintaining the current
behaviour for the user):


a) simple_patch (just swapping that two lines)

b) refactoring patch (calls the same function with set as command and
the passed command as arguments, etc. should be possible but I haven't
implemented)

c) play with the grammatic of the language instead of fallback in the
execution of the each line. Not sure how feasible it is since I haven't
look into it.

a) would be the patch that I attached, b) I would do it in a couple of
days, hopefully. c) I would not do it for quite long time and I think
that some other people could fix it.

If we go for c) maybe we could commit a) as a temporary fix waiting for
some other changes in teh grammar. Even, for c) would be nice to have
the unit tests to see if we break other things.

Thanks,

-- 
Carles Pina i Estany
	http://pinux.info

Re: var=foo should be valid?

[Carles Pina i Estany]

Hi,

On Dec/12/2009, Carles Pina i Estany wrote:

> We should fix it. We have three options (maintaining the current
> behaviour for the user):
> 
> 
> a) simple_patch (just swapping that two lines)

After talking with Robert last night and for the time being I've
committed the simple one (r1933) to fix the current bug.

-- 
Carles Pina i Estany
	http://pinux.info