[PATCH] Allow user-defined functions to override builtins.

[PATCH] Allow user-defined functions to override builtins.

[Glenn Washburn]

Currently, builtin commands take precedence over user-defined
functions.  This patch reverses that precedence, so that users can
"override" builtin commands.  Builtin commands may be accessed by
issuing the command prefixed by an '@' character.

My motivation for this change is to hook insmod in loaded configfiles
which set $prefix to a different location than desired.  If there are
any changes needed to help get this functionality included, please let
me know.

Glenn

---
 grub-core/script/execute.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
index afd5513..0769151 100644
--- a/grub-core/script/execute.c
+++ b/grub-core/script/execute.c
@@ -941,14 +941,15 @@ grub_script_execute_cmdline (struct
grub_script_cmd *cmd) args = argv.args + 2;
       cmdname = argv.args[1];
     }
-  grubcmd = grub_command_find (cmdname);
-  if (! grubcmd)
+  /* Allow user functions to override built in commands. */
+  func = grub_script_function_find (cmdname);
+  if (! func)
     {
       grub_errno = GRUB_ERR_NONE;
 
-      /* It's not a GRUB command, try all functions.  */
-      func = grub_script_function_find (cmdname);
-      if (! func)
+      /* It's not a function, check if GRUB command.  */
+      grubcmd = grub_command_find ((cmdname[0] ==
'@')?(cmdname+1):cmdname);
+      if (! grubcmd)
 	{
 	  /* As a last resort, try if it is an assignment.  */
 	  char *assign = grub_strdup (cmdname);
@@ -977,7 +978,9 @@ grub_script_execute_cmdline (struct grub_script_cmd
*cmd) }
 
   /* Execute the GRUB command or function.  */
-  if (grubcmd)
+  if (func)
+    ret = grub_script_function_call (func, argc, args);
+  else
     {
       if (grub_extractor_level && !(grubcmd->flags
 				    & GRUB_COMMAND_FLAG_EXTRACTOR))
@@ -990,8 +993,6 @@ grub_script_execute_cmdline (struct grub_script_cmd
*cmd) else
 	ret = (grubcmd->func) (grubcmd, argc, args);
     }
-  else
-    ret = grub_script_function_call (func, argc, args);
 
   if (invert)
     {
-- 
1.8.3.2

Re: [PATCH] Allow user-defined functions to override builtins.

[Michel Hermier]

Le 19/05/2014 08:37, Glenn Washburn a écrit :
> Currently, builtin commands take precedence over user-defined
> functions.  This patch reverses that precedence, so that users can
> "override" builtin commands.  Builtin commands may be accessed by
> issuing the command prefixed by an '@' character.
If you want to go this way, I would have preferred a 'builtin' command 
like other shell do, instead of reinventing the wheel and invent a new 
syntax.
But this only my opinion as a user, wait for developers opinion.
>
> My motivation for this change is to hook insmod in loaded configfiles
> which set $prefix to a different location than desired.  If there are
> any changes needed to help get this functionality included, please let
> me know.
>
> Glenn
>
> ---
>   grub-core/script/execute.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
> index afd5513..0769151 100644
> --- a/grub-core/script/execute.c
> +++ b/grub-core/script/execute.c
> @@ -941,14 +941,15 @@ grub_script_execute_cmdline (struct
> grub_script_cmd *cmd) args = argv.args + 2;
>         cmdname = argv.args[1];
>       }
> -  grubcmd = grub_command_find (cmdname);
> -  if (! grubcmd)
> +  /* Allow user functions to override built in commands. */
> +  func = grub_script_function_find (cmdname);
> +  if (! func)
>       {
>         grub_errno = GRUB_ERR_NONE;
>   
> -      /* It's not a GRUB command, try all functions.  */
> -      func = grub_script_function_find (cmdname);
> -      if (! func)
> +      /* It's not a function, check if GRUB command.  */
> +      grubcmd = grub_command_find ((cmdname[0] ==
> '@')?(cmdname+1):cmdname);
> +      if (! grubcmd)
>   	{
>   	  /* As a last resort, try if it is an assignment.  */
>   	  char *assign = grub_strdup (cmdname);
> @@ -977,7 +978,9 @@ grub_script_execute_cmdline (struct grub_script_cmd
> *cmd) }
>   
>     /* Execute the GRUB command or function.  */
> -  if (grubcmd)
> +  if (func)
> +    ret = grub_script_function_call (func, argc, args);
> +  else
>       {
>         if (grub_extractor_level && !(grubcmd->flags
>   				    & GRUB_COMMAND_FLAG_EXTRACTOR))
> @@ -990,8 +993,6 @@ grub_script_execute_cmdline (struct grub_script_cmd
> *cmd) else
>   	ret = (grubcmd->func) (grubcmd, argc, args);
>       }
> -  else
> -    ret = grub_script_function_call (func, argc, args);
>   
>     if (invert)
>       {

Re: [PATCH] Allow user-defined functions to override builtins.

[Ben Hildred]

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

On Mon, May 19, 2014 at 3:38 AM, Michel Hermier <michel.hermier@gmail.com>wrote:

> Le 19/05/2014 08:37, Glenn Washburn a écrit :
>
>  Currently, builtin commands take precedence over user-defined
>> functions.  This patch reverses that precedence, so that users can
>> "override" builtin commands.  Builtin commands may be accessed by
>> issuing the command prefixed by an '@' character.
>>
> If you want to go this way, I would have preferred a 'builtin' command
> like other shell do, instead of reinventing the wheel and invent a new
> syntax.
> But this only my opinion as a user, wait for developers opinion.
>
> Seconded, particularly  as the @ syntax is used elsewhere as echo
suppression. there is no need to confuse people.

>
>> My motivation for this change is to hook insmod in loaded configfiles
>> which set $prefix to a different location than desired.  If there are
>> any changes needed to help get this functionality included, please let
>> me know.
>>
>> Glenn
>>
>> ---
>>   grub-core/script/execute.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
>> index afd5513..0769151 100644
>> --- a/grub-core/script/execute.c
>> +++ b/grub-core/script/execute.c
>> @@ -941,14 +941,15 @@ grub_script_execute_cmdline (struct
>> grub_script_cmd *cmd) args = argv.args + 2;
>>         cmdname = argv.args[1];
>>       }
>> -  grubcmd = grub_command_find (cmdname);
>> -  if (! grubcmd)
>> +  /* Allow user functions to override built in commands. */
>> +  func = grub_script_function_find (cmdname);
>> +  if (! func)
>>       {
>>         grub_errno = GRUB_ERR_NONE;
>>   -      /* It's not a GRUB command, try all functions.  */
>> -      func = grub_script_function_find (cmdname);
>> -      if (! func)
>> +      /* It's not a function, check if GRUB command.  */
>> +      grubcmd = grub_command_find ((cmdname[0] ==
>> '@')?(cmdname+1):cmdname);
>> +      if (! grubcmd)
>>         {
>>           /* As a last resort, try if it is an assignment.  */
>>           char *assign = grub_strdup (cmdname);
>> @@ -977,7 +978,9 @@ grub_script_execute_cmdline (struct grub_script_cmd
>> *cmd) }
>>       /* Execute the GRUB command or function.  */
>> -  if (grubcmd)
>> +  if (func)
>> +    ret = grub_script_function_call (func, argc, args);
>> +  else
>>       {
>>         if (grub_extractor_level && !(grubcmd->flags
>>                                     & GRUB_COMMAND_FLAG_EXTRACTOR))
>> @@ -990,8 +993,6 @@ grub_script_execute_cmdline (struct grub_script_cmd
>> *cmd) else
>>         ret = (grubcmd->func) (grubcmd, argc, args);
>>       }
>> -  else
>> -    ret = grub_script_function_call (func, argc, args);
>>       if (invert)
>>       {
>>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
--
Ben Hildred
Automation Support Services
303 815 6721

[-- Attachment #2: Type: text/html, Size: 4364 bytes --]

Re: [PATCH] Allow user-defined functions to override builtins.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 19.05.2014 08:37, Glenn Washburn wrote:
> Currently, builtin commands take precedence over user-defined
> functions.  This patch reverses that precedence, so that users can
> "override" builtin commands.  Builtin commands may be accessed by
> issuing the command prefixed by an '@' character.
> 
Overriding builtins sounds like a bad idea. For once it creates a
language which is heavily dependent on context. This gets hairy and
messy very fast.
> My motivation for this change is to hook insmod in loaded configfiles
> which set $prefix to a different location than desired.  If there are
> any changes needed to help get this functionality included, please let
> me know.
> 
Could you detail your usercase more? $prefix is the location where
modules are, why not just set it to right location?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 274 bytes --]

Re: [PATCH] Allow user-defined functions to override builtins.

[Glenn Washburn]

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

On Thu, 22 May 2014 09:31:24 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> wrote:

> On 19.05.2014 08:37, Glenn Washburn wrote:
> > Currently, builtin commands take precedence over user-defined
> > functions.  This patch reverses that precedence, so that users can
> > "override" builtin commands.  Builtin commands may be accessed by
> > issuing the command prefixed by an '@' character.
> > 
> Overriding builtins sounds like a bad idea. For once it creates a
> language which is heavily dependent on context. This gets hairy and
> messy very fast.

I think that the power it provides compensates for that.  The user
should definitely know what they are doing, but I'd expect that of
anyone writing grub functions.  Most modern programming languages,
including bash, which grub script appears to be modeled after, supports
this.  Ultimately it allows for more flexibility and power for the grub
user (at the cost of allowing them to shoot themselves in the foot if
they so desire).

> > My motivation for this change is to hook insmod in loaded
> > configfiles which set $prefix to a different location than
> > desired.  If there are any changes needed to help get this
> > functionality included, please let me know.
> > 
> Could you detail your usercase more? $prefix is the location where
> modules are, why not just set it to right location?

The problem is that I want to load arbitrary grub config files from
various installs and have them work.  Ideally setting the correct root
and prefix, then loading the config file should work.  However, in some
(many?) cases this causes grub to crash because the config file will
invariably load modules which are built for the grub that installed the
config file, not the grub that is loading the config file.

But the prefix variable is used for other things in the loaded config
file.  So if I change the prefix to be that of the running grub to load
the correct modules, other parts of the config will break (for
instance, loading fonts or locales).

So my solution is to hook the insmod command, so that modules are
loaded from the path specified by the prefix_modules variable.  I had
been thinking of suggesting on the list to have separate variables for
the different uses of $prefix, but I think this is a better approach.

Another use case I can image is it could be used to disable certain
commands on the command line (but still allow them to be available to
scripts.  And I could probably come up with more cases.

This is definitely a useful feature for me.
Glenn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] Allow user-defined functions to override builtins.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.05.2014 09:47, Glenn Washburn wrote:
> But the prefix variable is used for other things in the loaded config
> file.  So if I change the prefix to be that of the running grub to load
> the correct modules, other parts of the config will break (for
> instance, loading fonts or locales).
Localesneed to come from folder matching running GRUB, otherwise you'll
get string mismatches. Fonts typically come from themes or is unifont.

Additionally your approach doesn't solve the problem in the first place.
insmod is just one of possible ways to load modules. For various
autoloads there is no command to hook.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 274 bytes --]

Re: [PATCH] Allow user-defined functions to override builtins.

[Michel Hermier]

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

Le 22 mai 2014 09:32, "Vladimir 'φ-coder/phcoder' Serbinenko" <
phcoder@gmail.com> a écrit :
>
> On 19.05.2014 08:37, Glenn Washburn wrote:
> > Currently, builtin commands take precedence over user-defined
> > functions.  This patch reverses that precedence, so that users can
> > "override" builtin commands.  Builtin commands may be accessed by
> > issuing the command prefixed by an '@' character.
> >
> Overriding builtins sounds like a bad idea. For once it creates a
> language which is heavily dependent on context. This gets hairy and
> messy very fast.
This probably true, but it add 2 great values for me.
One can add tracing/debugging for cheap even on generated/3 party scripts,
as afaik grub don't offer a tracing mode, this can be even more hairy.
One can emulate some extra funtionnality on older version to some point,
without the need to make custom wrappers, and requiring to hack 3rd party
scripts.
So it's true one can mess, and make the things dependant of context, and
maybe not all builtin should be overwritable, but when one feel the need to
override a builtin, he usually have good reason to do it, even if it can be
done in other ways.
> > My motivation for this change is to hook insmod in loaded configfiles
> > which set $prefix to a different location than desired.  If there are
> > any changes needed to help get this functionality included, please let
> > me know.
> >
> Could you detail your usercase more? $prefix is the location where
> modules are, why not just set it to right location?
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2152 bytes --]

Re: [PATCH] Allow user-defined functions to override builtins.

[Glenn Washburn]

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

On Sun, 25 May 2014 05:03:31 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> wrote:

> On 24.05.2014 09:47, Glenn Washburn wrote:
> > But the prefix variable is used for other things in the loaded
> > config file.  So if I change the prefix to be that of the running
> > grub to load the correct modules, other parts of the config will
> > break (for instance, loading fonts or locales).
> Localesneed to come from folder matching running GRUB, otherwise
> you'll get string mismatches. Fonts typically come from themes or is
> unifont.
> 
> Additionally your approach doesn't solve the problem in the first
> place. insmod is just one of possible ways to load modules. For
> various autoloads there is no command to hook.

I'm not sure of a good solution to the locale problem, but I don't
think I've run in to it (I use english, which I don't think needs
them).  So ignore my comment about locales.

As far as module auto-loading, please correct me if I'm wrong, but I
think auto-loading mostly happens when resolving dependencies when
loading another module.  I have seen, for instance, the "help loadfont"
command auto-load the font modules, but in what other situations does
modules auto-loading occur?

In my case, I think things generally work because auto-loading is
happening as a reaction to an explicit load from insmod.  In this
context things will work as desired because the changed prefix set in
the insmod hook will be used.

Michel also had a good use for it, which is to provide cheap builtin
trace/logging.  I expect as grub gets more powerful more uses not
thought of will crop up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] Allow user-defined functions to override builtins.

[Andrey Borzenkov]

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

В Tue, 27 May 2014 01:27:47 -0500
Glenn Washburn <development@efficientek.com> пишет:

> On Sun, 25 May 2014 05:03:31 +0200
> Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> wrote:
> 
> > On 24.05.2014 09:47, Glenn Washburn wrote:
> > > But the prefix variable is used for other things in the loaded
> > > config file.  So if I change the prefix to be that of the running
> > > grub to load the correct modules, other parts of the config will
> > > break (for instance, loading fonts or locales).
> > Localesneed to come from folder matching running GRUB, otherwise
> > you'll get string mismatches. Fonts typically come from themes or is
> > unifont.
> > 
> > Additionally your approach doesn't solve the problem in the first
> > place. insmod is just one of possible ways to load modules. For
> > various autoloads there is no command to hook.
> 
> I'm not sure of a good solution to the locale problem, but I don't
> think I've run in to it (I use english, which I don't think needs
> them).  So ignore my comment about locales.
> 
> As far as module auto-loading, please correct me if I'm wrong, but I
> think auto-loading mostly happens when resolving dependencies when
> loading another module.  I have seen, for instance, the "help loadfont"
> command auto-load the font modules, but in what other situations does
> modules auto-loading occur?
> 

Every time you use any command that is defined in a module. Or use
hashing/encryption algorithm. Or ... actually most of grub is
auto-loaded on demand.

> In my case, I think things generally work because auto-loading is
> happening as a reaction to an explicit load from insmod.  In this
> context things will work as desired because the changed prefix set in
> the insmod hook will be used.
> 
> Michel also had a good use for it, which is to provide cheap builtin
> trace/logging.  I expect as grub gets more powerful more uses not
> thought of will crop up.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]