Discussion:
[PATCH] Split of normal mode (version 2)
Bean
2009-03-30 17:41:14 UTC
Permalink
Hi,

This new patch make some changes based on the discussion of previous patch.

1, Move script engine to script/sh (sh.mod)
2, Move generic menu code to menu (menu.mod)
3, Move text menu viewer to menu/text (textmenu.mod)
4, Move misc function to lib (misc.mod)
5, Move setjmp to lib (setjmp.mod)

Now normal.mod only contains the reader code. To configure script
engine and viewer, you should add these lines at the beginning of
grub.cfg:

insmod sh
handler parser sh
insmod textmenu
--
Bean
Yoshinori K. Okuji
2009-04-03 18:40:33 UTC
Permalink
Post by Bean
Hi,
This new patch make some changes based on the discussion of previous patch.
1, Move script engine to script/sh (sh.mod)
2, Move generic menu code to menu (menu.mod)
3, Move text menu viewer to menu/text (textmenu.mod)
4, Move misc function to lib (misc.mod)
5, Move setjmp to lib (setjmp.mod)
I don't agree on the last two. Also, I don't like that you have just removed
the rescue command.
Post by Bean
Now normal.mod only contains the reader code. To configure script
engine and viewer, you should add these lines at the beginning of
insmod sh
handler parser sh
insmod textmenu
I prefer a more sophisticated approach (note: I hate manual loading).

For example, we can allow a config file to have a shebang, like "#!sh". If not
specified, GRUB can assume that "sh" is used, and load it automatically. This
kind of technique could even allow for using different languages in one
setup.

For textmenu, I think it makes sense to have a command "textmenu". Just
like "boot", GRUB can execute "textmenu" implicitly if a config file defines
any menu entry but does not execute any menu command. This way, textmenu is
automatically loaded.

Regards,
Okuji
Bean
2009-04-03 19:49:36 UTC
Permalink
Post by Yoshinori K. Okuji
Post by Bean
Hi,
This new patch make some changes based on the discussion of previous patch.
1, Move script engine to script/sh (sh.mod)
2, Move generic menu code to menu (menu.mod)
3, Move text menu viewer to menu/text (textmenu.mod)
4, Move misc function to lib (misc.mod)
5, Move setjmp to lib (setjmp.mod)
I don't agree on the last two. Also, I don't like that you have just removed
the rescue command.
Hi,

The reason for a separate misc.mod is that it contains pure function
and has no side effect, so other modules can use them freely. In the
other hand, normal.mod does a lot of tasks in the init function, it's
not a good place for common function. Besides, there are already
helper function in the lib directory, like crc and hexdump, it's not
piratical to put each one in a new module, we might just merge them
into one helper module.

The problem with setjmp is that it's platform dependent, so if we keep
it in normal.mod, we must define it in very platform rmk file. But now
setjmp is not used in normal.mod anymore, we could move it out, and
thus make normal.mod platform independent.
Post by Yoshinori K. Okuji
Post by Bean
Now normal.mod only contains the reader code. To configure script
engine and viewer, you should add these lines at the beginning of
insmod sh
handler parser sh
insmod textmenu
I prefer a more sophisticated approach (note: I hate manual loading).
For example, we can allow a config file to have a shebang, like "#!sh". If not
specified, GRUB can assume that "sh" is used, and load it automatically. This
kind of technique could even allow for using different languages in one
setup.
For textmenu, I think it makes sense to have a command "textmenu". Just
like "boot", GRUB can execute "textmenu" implicitly if a config file defines
any menu entry but does not execute any menu command. This way, textmenu is
automatically loaded.
I'm thinking about using environment variable to set handler, for
example, we could set it like this:

set parser=sh
set menu=textmenu
set terminal_output=gfxterm

We could use variable hooks to load the modules on demand.

For textmenu, we could add some test in normal.mod. After the main
config file is parsed, if it still doesn't contain a menu viewer, we
load textmenu automatically.
--
Bean
Yoshinori K. Okuji
2009-04-03 20:12:15 UTC
Permalink
Post by Bean
Post by Yoshinori K. Okuji
Post by Bean
Hi,
This new patch make some changes based on the discussion of previous patch.
1, Move script engine to script/sh (sh.mod)
2, Move generic menu code to menu (menu.mod)
3, Move text menu viewer to menu/text (textmenu.mod)
4, Move misc function to lib (misc.mod)
5, Move setjmp to lib (setjmp.mod)
I don't agree on the last two. Also, I don't like that you have just
removed the rescue command.
Hi,
The reason for a separate misc.mod is that it contains pure function
and has no side effect, so other modules can use them freely. In the
other hand, normal.mod does a lot of tasks in the init function, it's
not a good place for common function. Besides, there are already
helper function in the lib directory, like crc and hexdump, it's not
piratical to put each one in a new module, we might just merge them
into one helper module.
We can just put them in the normal.mod. What is wrong? Frankly, your argument
reminds me of the old discussion about monolithic vs. micro kernels...
Post by Bean
The problem with setjmp is that it's platform dependent, so if we keep
it in normal.mod, we must define it in very platform rmk file. But now
setjmp is not used in normal.mod anymore, we could move it out, and
thus make normal.mod platform independent.
setjmp is required for the switch between rescue mode and normal mode. Don't
remove it. "Hey, this is easier to maintain" is not a good reason to drop an
useful feature. If your intention is to reduce the maintenance cost, you can
simply define a variable for common files in common.rmk and use it in each
machine-specific rmk file.
Post by Bean
Post by Yoshinori K. Okuji
Post by Bean
Now normal.mod only contains the reader code. To configure script
engine and viewer, you should add these lines at the beginning of
insmod sh
handler parser sh
insmod textmenu
I prefer a more sophisticated approach (note: I hate manual loading).
For example, we can allow a config file to have a shebang, like "#!sh".
If not specified, GRUB can assume that "sh" is used, and load it
automatically. This kind of technique could even allow for using
different languages in one setup.
For textmenu, I think it makes sense to have a command "textmenu". Just
like "boot", GRUB can execute "textmenu" implicitly if a config file
defines any menu entry but does not execute any menu command. This way,
textmenu is automatically loaded.
I'm thinking about using environment variable to set handler, for
set parser=sh
set menu=textmenu
set terminal_output=gfxterm
We could use variable hooks to load the modules on demand.
For textmenu, we could add some test in normal.mod. After the main
config file is parsed, if it still doesn't contain a menu viewer, we
load textmenu automatically.
I agree that terminal_output (and so terminal_input as well) can be
implemented as environment variables, but it is very questionable whether a
parser or a menu interface can be considered as an environment variable. An
important question is how you would determine the name of a module from the
name of a parser or a menu interface.

Regards,
Okuji
phcoder
2009-04-03 22:19:46 UTC
Permalink
Post by Yoshinori K. Okuji
We can just put them in the normal.mod. What is wrong? Frankly, your argument
reminds me of the old discussion about monolithic vs. micro kernels...
Yes, it's quite similar and IMO it's better to have more small modules.
It also forces to code in an abstratc way which simplifies the maintaining
Post by Yoshinori K. Okuji
setjmp is required for the switch between rescue mode and normal mode.
It isn't. You can just call the corresponding function. What's wrong
with such approach?
--
Regards
Vladimir 'phcoder' Serbinenko
Colin D Bennett
2009-04-03 23:02:40 UTC
Permalink
On Sat, 04 Apr 2009 00:19:46 +0200
Post by phcoder
Post by Yoshinori K. Okuji
setjmp is required for the switch between rescue mode and normal mode.
It isn't. You can just call the corresponding function. What's wrong
with such approach?
So you could have something like

-------------
void grub_main ()
{
//...
// Try to start up in normal mode
normal ();
}

void normal ()
{
// do stuff
// User asked for rescue mode:
rescue ();
}

void rescue ()
{
// do stuff
// User asked for normal mode:
normal ();
}
-------------

What if you switch back and forth between normal and rescue mode
many times in a row? The stack will grow with each call and eventually
the stack will overflow and Bad Things will happen.

Granted, you'd have to switch many times to overflow the stack, but it
is a sub-optimal situation. You could have something like:

-------------
// In this context, grub_main acts as a dispatcher
// so that normal and rescue mode can be switched without
// unbounded growing of the stack.

enum mode { RESCUE, NORMAL };

void
grub_main ()
{
enum mode next_mode;
//...

// Try to start up in normal mode
next_mode = NORMAL;
while (1)
{
switch (next_mode)
{
case RESCUE:
next_mode = rescue ();
break;
case NORMAL:
next_mode = normal ();
break;
default:
// Panic!
}
}
}

enum mode
normal ()
{
// do stuff
// User asked for rescue mode:
return RESCUE;
}

enum mode
rescue ()
{
// do stuff
// User asked for normal mode:
return NORMAL;
}
-------------


Now you could also return function pointers instead of using
switch/case with enum constants, but it's the same concept. Then
setjmp is another similar way to do it without implementing a central
dispatcher like grub_main above.

At least that's how I think of it.

Regards,
Colin
Yoshinori K. Okuji
2009-04-05 14:23:54 UTC
Permalink
Post by Colin D Bennett
What if you switch back and forth between normal and rescue mode
many times in a row? The stack will grow with each call and eventually
the stack will overflow and Bad Things will happen.
Yes.
Post by Colin D Bennett
Now you could also return function pointers instead of using
switch/case with enum constants, but it's the same concept. Then
setjmp is another similar way to do it without implementing a central
dispatcher like grub_main above.
Exactly. For those who know the concept of continuation, setjmp/longjmp is
very handy. This makes some kind of programs to be written easily and simply.

Regards,
Okuji
Bean
2009-04-04 05:06:18 UTC
Permalink
Post by Yoshinori K. Okuji
Post by Bean
Post by Yoshinori K. Okuji
Post by Bean
Hi,
This new patch make some changes based on the discussion of previous patch.
1, Move script engine to script/sh (sh.mod)
2, Move generic menu code to menu (menu.mod)
3, Move text menu viewer to menu/text (textmenu.mod)
4, Move misc function to lib (misc.mod)
5, Move setjmp to lib (setjmp.mod)
I don't agree on the last two. Also, I don't like that you have just
removed the rescue command.
Hi,
The reason for a separate misc.mod is that it contains pure function
and has no side effect, so other modules can use them freely. In the
other hand, normal.mod does a lot of tasks in the init function, it's
not a good place for common function. Besides, there are already
helper function in the lib directory, like crc and hexdump, it's not
piratical to put each one in a new module, we might just merge them
into one helper module.
We can just put them in the normal.mod. What is wrong? Frankly, your argument
reminds me of the old discussion about monolithic vs. micro kernels...
One of the problem for normal.mod dependency is its side effect. For
example, currently ls.mod depend on normal.mod just for
grub_normal_print_device_info. If we want to embed ls.mod in core.img,
we must embedded normal.mod as well, along with a lot of
initialization actions that we may not want.
Post by Yoshinori K. Okuji
Post by Bean
The problem with setjmp is that it's platform dependent, so if we keep
it in normal.mod, we must define it in very platform rmk file. But now
setjmp is not used in normal.mod anymore, we could move it out, and
thus make normal.mod platform independent.
setjmp is required for the switch between rescue mode and normal mode. Don't
remove it. "Hey, this is easier to maintain" is not a good reason to drop an
useful feature. If your intention is to reduce the maintenance cost, you can
simply define a variable for common files in common.rmk and use it in each
machine-specific rmk file.
No, setjmp is not required for the switch between rescue mode and
normal mode anymore. In fact, to enter rescue mode, you use:

handler reader rescue

To switch back to normal mode:

handler reader normal

It is not recursive, so no need for setjmp.
Post by Yoshinori K. Okuji
Post by Bean
Post by Yoshinori K. Okuji
Post by Bean
Now normal.mod only contains the reader code. To configure script
engine and viewer, you should add these lines at the beginning of
insmod sh
handler parser sh
insmod textmenu
I prefer a more sophisticated approach (note: I hate manual loading).
For example, we can allow a config file to have a shebang, like "#!sh".
If not specified, GRUB can assume that "sh" is used, and load it
automatically. This kind of technique could even allow for using
different languages in one setup.
For textmenu, I think it makes sense to have a command "textmenu". Just
like "boot", GRUB can execute "textmenu" implicitly if a config file
defines any menu entry but does not execute any menu command. This way,
textmenu is automatically loaded.
I'm thinking about using environment variable to set handler, for
set parser=sh
set menu=textmenu
set terminal_output=gfxterm
We could use variable hooks to load the modules on demand.
For textmenu, we could add some test in normal.mod. After the main
config file is parsed, if it still doesn't contain a menu viewer, we
load textmenu automatically.
I agree that terminal_output (and so terminal_input as well) can be
implemented as environment variables, but it is very questionable whether a
parser or a menu interface can be considered as an environment variable. An
important question is how you would determine the name of a module from the
name of a parser or a menu interface.
We can generate a handler.lst by scanning source for
grub_parser_register, etc, just like commands.lst:

parser.sh: sh
reader.normal: normal
menu.textmenu: textmenu
terminal_outout.gfxterm: gfxterm

We can then read it in normal.mod and set the hooks for all handler classes.
--
Bean
Yoshinori K. Okuji
2009-04-05 14:33:02 UTC
Permalink
Post by Bean
One of the problem for normal.mod dependency is its side effect. For
example, currently ls.mod depend on normal.mod just for
grub_normal_print_device_info. If we want to embed ls.mod in core.img,
we must embedded normal.mod as well, along with a lot of
initialization actions that we may not want.
Right, if we can completely merge the ls on rescue mode and that on normal
mode. For now, I am not sure if this is feasible, so I prefer to keep a poor
version of ls for rescue mode, which does not require functions in
normal.mod, until you prove that that is feasible.
Post by Bean
We can generate a handler.lst by scanning source for
parser.sh: sh
reader.normal: normal
menu.textmenu: textmenu
terminal_outout.gfxterm: gfxterm
We can then read it in normal.mod and set the hooks for all handler classes.
I like if you can merge all kinds of handlers this way, thus eliminating the
command.lst and the fs.lst.

But I still think that having commands for parsers and menus, since you can
do:

grub> sh /boot/grub/foo.sh
grub> textmenu
grub> gfxmenu

They won't be very useful for most people, but could be useful for
development.

Regards,
Okuji
Bean
2009-04-05 15:02:59 UTC
Permalink
Post by Yoshinori K. Okuji
Post by Bean
One of the problem for normal.mod dependency is its side effect. For
example, currently ls.mod depend on normal.mod just for
grub_normal_print_device_info. If we want to embed ls.mod in core.img,
we must embedded normal.mod as well, along with a lot of
initialization actions that we may not want.
Right, if we can completely merge the ls on rescue mode and that on normal
mode. For now, I am not sure if this is feasible, so I prefer to keep a poor
version of ls for rescue mode, which does not require functions in
normal.mod, until you prove that that is feasible.
Hi,

Currently, this is implemented using priority list. The commands has a
field that indicate its priority. For normal command, the value is 0,
for extended command, it's 1. So if two version of ls is loaded, the
extended ls would be used. All commands are in a unify set, so you can
run normal commads/extended commands in either rescue or normal mode.
Post by Yoshinori K. Okuji
Post by Bean
We can generate a handler.lst by scanning source for
parser.sh: sh
reader.normal: normal
menu.textmenu: textmenu
terminal_outout.gfxterm: gfxterm
We can then read it in normal.mod and set the hooks for all handler classes.
I like if you can merge all kinds of handlers this way, thus eliminating the
command.lst and the fs.lst.
But I still think that having commands for parsers and menus, since you can
grub> sh /boot/grub/foo.sh
grub> textmenu
grub> gfxmenu
They won't be very useful for most people, but could be useful for
development.
Yeah, that's possible. We can register a generic command to do the task.
--
Bean
Yoshinori K. Okuji
2009-04-05 15:43:02 UTC
Permalink
Post by Bean
Post by Yoshinori K. Okuji
Post by Bean
One of the problem for normal.mod dependency is its side effect. For
example, currently ls.mod depend on normal.mod just for
grub_normal_print_device_info. If we want to embed ls.mod in core.img,
we must embedded normal.mod as well, along with a lot of
initialization actions that we may not want.
Right, if we can completely merge the ls on rescue mode and that on
normal mode. For now, I am not sure if this is feasible, so I prefer to
keep a poor version of ls for rescue mode, which does not require
functions in normal.mod, until you prove that that is feasible.
Hi,
Currently, this is implemented using priority list. The commands has a
field that indicate its priority. For normal command, the value is 0,
for extended command, it's 1. So if two version of ls is loaded, the
extended ls would be used. All commands are in a unify set, so you can
run normal commads/extended commands in either rescue or normal mode.
Great. As long as we can use a smaller (so more stupid) version of ls, I have
no objection.

Regards,
Okuji
Bean
2009-04-06 16:39:23 UTC
Permalink
Hi,

This is another update of the patch.

1, Now completion.c is in menu.mod, and menu_viewer.c is in misc.mod,
the reason for the switch is to allow configfile to depend on misc.mod
only.
2, Auto generate handler.lst file, which contain module information
for handlers. normal.mod uses it to register commands to set active
handler, for example:

parser.sh
menu_viewer.text
terminal_output.gfxterm

3, configfile now support an optional parameter to specify the script
engine, for example:

configfile /aa.cfg sh

When configfile returns, the script engine would be restored to the
previous value. This is useful for switching script engine. For
example, you can parse a file in another language, then switch back to
sh for the rest of grub.cfg.

4, normal.mod set the default parser and menu viewer before parsing grub.cfg:
parser.sh
menu_viewer.text
--
Bean
Yoshinori K. Okuji
2009-04-07 00:41:12 UTC
Permalink
Post by Bean
Hi,
This is another update of the patch.
Thank you. I will review your patch and send comments tonight.

Regards,
Okuji
Post by Bean
1, Now completion.c is in menu.mod, and menu_viewer.c is in misc.mod,
the reason for the switch is to allow configfile to depend on misc.mod
only.
2, Auto generate handler.lst file, which contain module information
for handlers. normal.mod uses it to register commands to set active
parser.sh
menu_viewer.text
terminal_output.gfxterm
3, configfile now support an optional parameter to specify the script
configfile /aa.cfg sh
When configfile returns, the script engine would be restored to the
previous value. This is useful for switching script engine. For
example, you can parse a file in another language, then switch back to
sh for the rest of grub.cfg.
4, normal.mod set the default parser and menu viewer before parsing
grub.cfg: parser.sh
menu_viewer.text
Yoshinori K. Okuji
2009-04-09 23:49:57 UTC
Permalink
Post by Bean
Hi,
This is another update of the patch.
1, Now completion.c is in menu.mod, and menu_viewer.c is in misc.mod,
the reason for the switch is to allow configfile to depend on misc.mod
only.
I think the name "misc.mod" is ugly. Can you think about a better name?
Post by Bean
2, Auto generate handler.lst file, which contain module information
for handlers. normal.mod uses it to register commands to set active
parser.sh
menu_viewer.text
terminal_output.gfxterm
Great.
Post by Bean
3, configfile now support an optional parameter to specify the script
configfile /aa.cfg sh
When configfile returns, the script engine would be restored to the
previous value. This is useful for switching script engine. For
example, you can parse a file in another language, then switch back to
sh for the rest of grub.cfg.
I object to the syntax, but not to the idea. "configfile" is GRUB-specific, so
it might be acceptable. But IIRC the underlying function is shared
with "source", right? In Bourne Shell, "source FILE ARG" means that the file
FILE is executed with a positional argument ARG. So it is not intuitive to
specify a parser this way.

I proposed using a shebang some days ago. Was it so bad?
Post by Bean
4, normal.mod set the default parser and menu viewer before parsing
grub.cfg: parser.sh
menu_viewer.text
Regards,
Okuji
Bean
2009-04-10 05:17:54 UTC
Permalink
Post by Yoshinori K. Okuji
Post by Bean
Hi,
This is another update of the patch.
1, Now completion.c is in menu.mod, and menu_viewer.c is in misc.mod,
the reason for the switch is to allow configfile to depend on misc.mod
only.
I think the name "misc.mod" is ugly. Can you think about a better name?
Hi,

Perhaps lib.mod, or helper.mod ?
Post by Yoshinori K. Okuji
Post by Bean
2, Auto generate handler.lst file, which contain module information
for handlers. normal.mod uses it to register commands to set active
parser.sh
menu_viewer.text
terminal_output.gfxterm
Great.
Post by Bean
3, configfile now support an optional parameter to specify the script
configfile /aa.cfg sh
When configfile returns, the script engine would be restored to the
previous value. This is useful for switching script engine. For
example, you can parse a file in another language, then switch back to
sh for the rest of grub.cfg.
I object to the syntax, but not to the idea. "configfile" is GRUB-specific, so
it might be acceptable. But IIRC the underlying function is shared
with "source", right? In Bourne Shell, "source FILE ARG" means that the file
FILE is executed with a positional argument ARG. So it is not intuitive to
specify a parser this way.
Yeah, I don't like the syntax either, but we need to find a way to
distinguish between the "configfile" and "source". "source" operates
on the current environment, while "configfile" create a new
environment.
Post by Yoshinori K. Okuji
I proposed using a shebang some days ago. Was it so bad?
I like that idea too, but it needs to change grub_file_getline a
little bit as currently it treats lines starting with '#' as comment,
so we don't have a chance to examine the contents behind it.

Perhaps we can combine this with configfile/source. For example, when
it reads a file starts with #!, it automatically switch parser, and
restore back to the original parser on exit. This way, we don't need
to hack the configfile/source command at all.
--
Bean
Bean
2009-04-10 20:17:43 UTC
Permalink
Hi,

Another update for the patch:

sync with svn r2074
misc bug fixes
change build script for i386-efi, i386-coreboot, i386-ieee1275 and
x86_64-efi as well as i386-pc, grub-emu now builds properly for
i386-pc.
support the use of #! in the script file. It would switch to the
selected parser, read the file, then restore to the original parser
before returning.
configfile/source now back to previous syntax, as there is no need to
change parser in here anymore.
--
Bean
Yoshinori K. Okuji
2009-04-11 09:50:14 UTC
Permalink
Post by Bean
Hi,
sync with svn r2074
misc bug fixes
change build script for i386-efi, i386-coreboot, i386-ieee1275 and
x86_64-efi as well as i386-pc, grub-emu now builds properly for
i386-pc.
support the use of #! in the script file. It would switch to the
selected parser, read the file, then restore to the original parser
before returning.
configfile/source now back to previous syntax, as there is no need to
change parser in here anymore.
I am afraid that this patch is getting too big to review. Honestly, I would
like you to check in some parts quickly, and the rest still requires more
discussion. Anyway, I think it is a good practice to check in one thing at
one time, so splitting the patch is a good thing. For example:

- handler unification -> one patch
- config embedding -> one patch
- parser separation -> one patch
- viewer separation -> one patch
- misc module -> one patch
- etc.

Due to the volume, I am not certain if I have really read everything. :(

Regards,
Okuji
Bean
2009-04-11 14:03:15 UTC
Permalink
Post by Yoshinori K. Okuji
Post by Bean
Hi,
sync with svn r2074
misc bug fixes
change build script for i386-efi, i386-coreboot, i386-ieee1275 and
x86_64-efi as well as i386-pc, grub-emu now builds properly for
i386-pc.
support the use of #! in the script file. It would switch to the
selected parser, read the file, then restore to the original parser
before returning.
configfile/source now back to previous syntax, as there is no need to
change parser in here anymore.
I am afraid that this patch is getting too big to review. Honestly, I would
like you to check in some parts quickly, and the rest still requires more
discussion. Anyway, I think it is a good practice to check in one thing at
- handler unification -> one patch
- config embedding -> one patch
- parser separation -> one patch
- viewer separation -> one patch
- misc module -> one patch
- etc.
Due to the volume, I am not certain if I have really read everything. :(
Hi,

All right.
--
Bean
Continue reading on narkive:
Loading...