normal/cmdline bug & patch

normal/cmdline bug & patch

[Tomas Ebenlendr]

My grub sumetimes prevent to boot / unload normal mode. Here is patch
after 2 days work. (I made grub to log to LPT, this can bochs save to
file, it is not clean patch, so I don't post it here.)

Here is bugfix:

"hist_end - i" cannot be greater than hist_size, but it can be smaller
than zero

--- grub2_x/normal/cmdline.c	2004-06-05 00:20:18.000000000 +0200
+++ grub2_patched/normal/cmdline.c	2004-06-15 13:18:59.000000000 +0200
@@ -55,8 +55,8 @@
 	  for (i = 0; i < delsize; i++)
 	    {
 	      int pos = hist_end - i;
-	      if (pos > hist_size)
-		pos -= hist_size;
+	      if (pos < hist_size)
+		pos += hist_size;
 	      grub_free (old_hist_lines[pos]);
 	    }
 
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45508177747

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

patch of patch:

> --- grub2_x/normal/cmdline.c	2004-06-05 00:20:18.000000000 +0200
> +++ grub2_patched/normal/cmdline.c	2004-06-15 13:18:59.000000000 +0200
> @@ -55,8 +55,8 @@
>  	  for (i = 0; i < delsize; i++)
>  	    {
>  	      int pos = hist_end - i;
> -	      if (pos > hist_size)
> -		pos -= hist_size;
> +	      if (pos < hist_size)
here should be:  if (pos < 0)

> +		pos += hist_size;
>  	      grub_free (old_hist_lines[pos]);
>  	    }
>  
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45509698821

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

I missed there, that hist_end is _not_ the last entry, but it is by one
after it. So here is final version of patch:

--- grub2_x/normal/cmdline.c	2004-06-15 15:41:57.000000000 +0200
+++ grub2_patched/normal/cmdline.c	2004-06-15 15:41:22.000000000 +0200
@@ -52,11 +52,11 @@
 	  int delsize = hist_used - newsize;
 	  hist_used = newsize;
 
-	  for (i = 0; i < delsize; i++)
+	  for (i = 1; i <= delsize; i++)
 	    {
 	      int pos = hist_end - i;
-	      if (pos > hist_size)
-		pos -= hist_size;
+	      if (pos < 0)
+		pos += hist_size;
 	      grub_free (old_hist_lines[pos]);
 	    }
 
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45533805151

Re: normal/cmdline bug & patch

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

> I missed there, that hist_end is _not_ the last entry, but it is by one
> after it. So here is final version of patch:

Thanks a lot for your patch.

Hopefully I can have a look at all new patches this evening.  Same for
the filesystem work we talked about.

Can you please write a changelog entry for your patch next time?  You
can find some information on how to do that on:

http://www.gnu.org/prep/standards_40.html#SEC40

Thanks,
Marco

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

> Can you please write a changelog entry for your patch next time?  You
> can find some information on how to do that on:

Ok here is another version of the patch with two more bugs fixed:

bug1 ... hist_end does not point to last entry but to first empty.
bug2 ... pos can underflow, but not overflow
bug3 ... correction must act as 'modulo' so adding modulo value is right
    way
bug4 ... if hist_pos == hist_end, we need ot copy nothing, all lines were
    copied.

--- grub2_x/normal/cmdline.c	2004-06-15 15:41:57.000000000 +0200
+++ grub2_patched/normal/cmdline.c	2004-06-15 15:41:22.000000000 +0200
@@ -52,11 +52,11 @@
 	  int delsize = hist_used - newsize;
 	  hist_used = newsize;
 
-	  for (i = 0; i < delsize; i++)
+	  for (i = 1; i <= delsize; i++)
 	    {
 	      int pos = hist_end - i;
-	      if (pos > hist_size)
-		pos -= hist_size;
+	      if (pos < 0)
+		pos += hist_size;
 	      grub_free (old_hist_lines[pos]);
 	    }
 
 	  hist_end -= delsize;
 	  if (hist_end < 0)
-	    hist_end = hist_size - hist_end;
+	    hist_end += hist_size;
 	}
 
-      if (hist_pos < hist_end)
+      if (hist_pos <= hist_end)
 	grub_memmove (hist_lines, old_hist_lines + hist_pos,
 		      (hist_end - hist_pos) * sizeof (char *));
       else
--- grub2_x/ChangeLog	2004-06-05 00:20:17.000000000 +0200
+++ grub2_patched/ChangeLog	2004-06-15 18:14:34.000000000 +0200
@@ -1,3 +1,10 @@
+2004-06-15  Tomas Ebenlendr <ebik@ucw.cz>
+
+        Bugfix of cmdline history (normal mode).
+
+	* normal/commandline.c (grub_set_history): History reallocating
+	was completely bad (about 4 brainos), all fixed.
+
 2004-05-24  Marco Gerards  <metgerards@student.han.nl>
 
 	Add support for UFS version 1 and 2.  Add support for the minix

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45563461344

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

Oh sorry, another bug found. I will wait at least one hour before
submitting any patch, so I won't post too many messages.

Here is complete patch of normal/cmdline.c (grub_history_set)

> 
> Ok here is another version of the patch with two more bugs fixed:
> 
> bug1 ... hist_end does not point to last entry but to first empty.
> bug2 ... pos can underflow, but not overflow
> bug3 ... correction must act as 'modulo' so adding modulo value is right
>     way
> bug4 ... if hist_pos == hist_end, we need ot copy nothing, all lines were
>     copied.
> 
  - now it is fixed the right way. Before I fixed it, so another bug
    occur when buffer is full.
bug5 ... something was copied when hist_end < hist_pos, but it was
    definitely not what should be copied.

I think this function must have been written very late at nigt.

Now I will try to write 'chain' module (normal mode command for
'_chain'). 

---------

diff -ur grub2_x/ChangeLog grub2_work_xxx/ChangeLog
--- grub2_x/ChangeLog	2004-06-15 19:19:04.000000000 +0200
+++ grub2_work_xxx/ChangeLog	2004-06-15 19:24:42.000000000 +0200
@@ -1,3 +1,10 @@
+2004-06-15  Tomas Ebenlendr <ebik@ucw.cz>
+
+        Bugfix of cmdline history (normal mode).
+
+	* normal/commandline.c (grub_set_history): History reallocating
+	was completely bad (about 5 brainos), all fixed.
+
 2004-05-24  Marco Gerards  <metgerards@student.han.nl>
 
 	Add support for UFS version 1 and 2.  Add support for the minix
diff -ur grub2_x/normal/cmdline.c grub2_work_xxx/normal/cmdline.c
--- grub2_x/normal/cmdline.c	2004-06-15 19:18:52.000000000 +0200
+++ grub2_work_xxx/normal/cmdline.c	2004-06-15 19:14:15.000000000 +0200
@@ -52,33 +52,35 @@
 	  int delsize = hist_used - newsize;
 	  hist_used = newsize;
 
-	  for (i = 0; i < delsize; i++)
+	  for (i = 1; i <= delsize; i++)
 	    {
 	      int pos = hist_end - i;
-	      if (pos > hist_size)
-		pos -= hist_size;
+	      if (pos < 0)
+		pos += hist_size;
 	      grub_free (old_hist_lines[pos]);
 	    }
 
 	  hist_end -= delsize;
 	  if (hist_end < 0)
-	    hist_end = hist_size - hist_end;
+	    hist_end += hist_size;
 	}
 
      if (hist_pos < hist_end)
 	grub_memmove (hist_lines, old_hist_lines + hist_pos,
 		      (hist_end - hist_pos) * sizeof (char *));
-       else
+       else if (hist_used)
 	{
-	  /* Copy the first part.  */
-	  grub_memmove (hist_lines, old_hist_lines,
-			hist_pos * sizeof (char *));
+	  /* old_hist_lines: 0 <older part> hist_end <empty> hist_pos <newer part> */
+	  /* entry at hist_end is empty, at hist_pos contains first entry */
 
-
-	  /* Copy the last part.  */
-	  grub_memmove (hist_lines + hist_pos, old_hist_lines + hist_pos,
+	  /* Copy the older part.  */
+	  grub_memmove (hist_lines, old_hist_lines + hist_pos,
 			(hist_size - hist_pos) * sizeof (char *));
 
+	  /* Copy the newer part. */
+	  grub_memmove (hist_lines + hist_size - hist_pos, old_hist_lines,
+			hist_end * sizeof (char *));
+
 	}
     }
 
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45578118675

Re: normal/cmdline bug & patch

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

> +	* normal/commandline.c (grub_set_history): History reallocating
> +	was completely bad (about 4 brainos), all fixed.

Oh, shame on me... I should be terribly ashamed for that piece of code.

Are you still fixing bugs?  If you keep sending updates, I will wait
for a while. ;)

Thanks,
Marco

Re: normal/cmdline bug & patch

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

> Oh sorry, another bug found. I will wait at least one hour before
> submitting any patch, so I won't post too many messages.

[...]

The patch looks ok to me.  Unfortunately I can't apply it, patch
rejects the patch.

Perhaps this is a problem with my mailclient, or Thomas' mailclient
screwed things up... Can someone else test this?

Okuji, is it ok for you to apply this patch (and the other bugfixes
Tomas sent in)?

--
Marco

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

If you applied some of patches in this thread before this patch, please
undo them this is the only and final patch.

> ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:
> 
> > Oh sorry, another bug found. I will wait at least one hour before
> > submitting any patch, so I won't post too many messages.
> 
> [...]
> 
> The patch looks ok to me.  Unfortunately I can't apply it, patch
> rejects the patch.
> 
> Perhaps this is a problem with my mailclient, or Thomas' mailclient
> screwed things up... Can someone else test this?
> 
> Okuji, is it ok for you to apply this patch (and the other bugfixes
> Tomas sent in)?
> 
> --
> Marco
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45751122622

Re: normal/cmdline bug & patch

[Yoshinori K. Okuji]

On Tuesday 15 June 2004 22:36, Marco Gerards wrote:
> Okuji, is it ok for you to apply this patch (and the other bugfixes
> Tomas sent in)?

Bugfixes are very welcome. :)

As for the direct access patch, I'd like to think about it more.

Okuji

Re: normal/cmdline bug & patch

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Tuesday 15 June 2004 22:36, Marco Gerards wrote:
>> Okuji, is it ok for you to apply this patch (and the other bugfixes
>> Tomas sent in)?
>
> Bugfixes are very welcome. :)

Ok, I wanted to make sure if there were issues with copyright
assignments.  I will apply the patches ASAP.

> As for the direct access patch, I'd like to think about it more.

Direct access patch?

Thanks,
Marco

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

> ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:
> 
> 
> > I also sugested that if we want users not confused by 2 versions of
> > modules ve may add automatic loading of 'normal mode module variants' of
> > each 'rescue mode module variant'. (e.g. _chain.mod is loaded, we load
> > normal.mod and it automatically loads chain.mod, we then use insmod
> > _linux.mod and normal.mod will also load linux.mod).
> 
> Will this work in a hard-coded way or a flexible way, comparable to
> module loading (and dependencies) on linux?
> 
> I should catch up with my email and read the discussion...
> 
> --
> Marco

This was not discussed. I prefer here (in this case) hardcoding. At
normal mode entrance (rescue cmd normal) and normal mode insmod 
will be checked if module is named in special way.
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45785221868

Marco Gerards <metgerards@student.han.nl>

[Tomas Ebenlendr]

> > As for the direct access patch, I'd like to think about it more.
> 
> Direct access patch?

Subject is 'grub-setup on unmounted fs' or similar.
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45793987806

sorry

[Tomas Ebenlendr]

I'm very sorry. The marco's mail address should go to 'To:' field. Not
to 'Subject:'
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.45794455829

Re: normal/cmdline bug & patch

[Yoshinori K. Okuji]

On Wednesday 16 June 2004 13:50, Tomas Ebenlendr wrote:
> > > I also sugested that if we want users not confused by 2 versions
> > > of modules ve may add automatic loading of 'normal mode module
> > > variants' of each 'rescue mode module variant'. (e.g. _chain.mod
> > > is loaded, we load normal.mod and it automatically loads
> > > chain.mod, we then use insmod _linux.mod and normal.mod will also
> > > load linux.mod).
> >
> > Will this work in a hard-coded way or a flexible way, comparable to
> > module loading (and dependencies) on linux?
> >
> > I should catch up with my email and read the discussion...
> >
> > --
> > Marco
>
> This was not discussed. I prefer here (in this case) hardcoding. At
> normal mode entrance (rescue cmd normal) and normal mode insmod
> will be checked if module is named in special way.

My preferable way is to load modules implicitly when executing commands. 
For example:

grub> linux /boot/vmlinuz root=/dev/hda1 ro

[Check if linux.mod is loaded currently]
  [If not, load it]
    [If failed, abort this command]
[Load the kernel]

This is possible in the same way as autoload in Emacs. For example, we 
can have /boot/grub/autoload.lst which is a table of commands and 
modules, like this:

chainload chainload.mod
linux linux.mod
initrd linux.mod
multiboot mb.mod

Okuji

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

> [Check if linux.mod is loaded currently]
>   [If not, load it]
>     [If failed, abort this command]
> [Load the kernel]
> 
> This is possible in the same way as autoload in Emacs. For example, we 
> can have /boot/grub/autoload.lst which is a table of commands and 
> modules, like this:
> 
> chainload chainload.mod
> linux linux.mod
> initrd linux.mod
> multiboot mb.mod
> 
> Okuji

Yes that will be very nice. Basic users need not to know about modules.
:-)

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.4631986187

Re: normal/cmdline bug & patch

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> This is possible in the same way as autoload in Emacs. For example, we 
> can have /boot/grub/autoload.lst which is a table of commands and 
> modules, like this:
>
> chainload chainload.mod
> linux linux.mod
> initrd linux.mod
> multiboot mb.mod

How about other modules, like filesystems?

--
Marco

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

> "Yoshinori K. Okuji" <okuji@enbug.org> writes:
> 
> > This is possible in the same way as autoload in Emacs. For example, we 
> > can have /boot/grub/autoload.lst which is a table of commands and 
> > modules, like this:
> >
> > chainload chainload.mod
> > linux linux.mod
> > initrd linux.mod
> > multiboot mb.mod
> 
> How about other modules, like filesystems?

Here is one not so simple, but generic idea:

let's say that the table will be:
<module> <domain> <resource>

linux.mod cmd linux
ext2.mod fs /"file" like magic-recognition pattern (may have whitespaces)/

then we can have function (grub_ prefixes ommited):
  err_t loadresource(char * domain,load_determine_t hook, void * hook-arg);
and type
  typedef int load_determine_t(char * resource, void * arg);

in case "cmd" hook() will be simple: returns TRUE if
    strcmp(resource,(char*) arg) == 0
in case "fs" 'arg' will point to some fs structure, 'hook' will try to
    read sector as specified in recogition pattern (char * resource) and
    tries to match it.

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.46326654523

Re: normal/cmdline bug & patch

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

>> How about other modules, like filesystems?
>
> Here is one not so simple, but generic idea:
>
> let's say that the table will be:
> <module> <domain> <resource>
>
> linux.mod cmd linux
> ext2.mod fs /"file" like magic-recognition pattern (may have whitespaces)/

How do you know which filesystem to use?  For example when accessing a
ufs filesystem?

How about loading all filesystems explicitly in grub.conf?

--
Marco

Re: Autoloading WAS: normal/cmdline bug & patch

[Tomas Ebenlendr]

> >
> > linux.mod cmd linux
> > ext2.mod fs /"file" like magic-recognition pattern (may have whitespaces)/
> 
> How do you know which filesystem to use?  For example when accessing a
> ufs filesystem?

Do you tried unix command 'file' on some partition?
e.g.:

$ file -sk /dev/hda1
/dev/hda1: x86 boot sector, code offset 0x58, OEM-ID "MSWIN4.1",
sectors/cluster 8, Media descriptor 0xf8, heads 240, hidden sectors
1164303, sectors 10251297 (volumes > 32 MB) , FAT (32 bit), sectors/FAT
9993, rootdir cluster 655, reserved3 0x800000, serial number 0x164d1002,
label: "WIN        "
$ file -sk /dev/hda2
/dev/hda2: x86 boot sector, code offset 0x48\012- Linux rev 0.0 ext2
filesystem data (mounted or unclean)

I think every fs can be identified using some "magic". This was what i
mean by "file" like recognition pattern.

> How about loading all filesystems explicitly in grub.conf?

yes this is much simper (and with less bugs be here), but user must know
which fs he uses. But he can have comfort when booting grub from CD, not
to load every fs module by grub.conf and also not to type insmod.
(Ok, in grub.conf will be specified something like fs-auto.mod which
will implement domain 'fs' for module autoloader.)

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.46332982316

Re: Autoloading WAS: normal/cmdline bug & patch

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

> I think every fs can be identified using some "magic". This was what i
> mean by "file" like recognition pattern.

Well, I know that.  It is how GRUB detects the filesystem type.

The problem is that the filesystem type can not be determined in a
uniform way.  You will have to include all tests in the core module.
That removes the flexibility.

Consider reiserfs.  If you want to detect if this module has to be
loaded, you need some reiserfs specific code to do that.  But what if
a user want to add another module?

>> How about loading all filesystems explicitly in grub.conf?
>
> yes this is much simper (and with less bugs be here), but user must know
> which fs he uses. But he can have comfort when booting grub from CD, not
> to load every fs module by grub.conf and also not to type insmod.
> (Ok, in grub.conf will be specified something like fs-auto.mod which
> will implement domain 'fs' for module autoloader.)

How about just loading every module that is sane for that
architecture?  For example all linux specific filesystems, fat on the
PC/alpha, hfs[+] on the apple, etc.

What do you mean with fs-auto, unfortunately I do not understand.
An example would help.

Thanks,
Marco

Re: Autoloading WAS: normal/cmdline bug & patch

[Marco Gerards]

Marco Gerards <metgerards@student.han.nl> writes:

> How about just loading every module that is sane for that
> architecture?  For example all linux specific filesystems, fat on the
> PC/alpha, hfs[+] on the apple, etc.

Perhaps it would be better to do this just when it is required.  So
whenever all loaded filesystems are scanned and the filesystem is not
detected GRUB needs to load new modules.  So it loads modules in a
fixed order (sorted on how likely it is that the filesystem is used,
perhaps).

We can even make a /boot/grub/fs directory and just load the modules
from there.  Of course you just read the modules until you found the
required module.

The advantage of this approach is that you just have to load modules
when there is a need for it.  The bad thing is that something like `ls
(hd)' will become slower.

Anyway, I'd like to hear more ideas about this.  I am just
brainstorming. :)

Thanks,
Marco

Re: Autoloading WAS: normal/cmdline bug & patch

[Tomas Ebenlendr]

> The problem is that the filesystem type can not be determined in a
> uniform way.  You will have to include all tests in the core module.
> That removes the flexibility.
> 
...
> 
> What do you mean with fs-auto, unfortunately I do not understand.
> An example would help.
> 

Autoloading should not be in 'core'. normal.mod an also work without
autoloading. So I want fs-auto.mod which will contain all test of
filesystems which are known at compile-time. Or, when such test can be
specified in some reasoable format, then fs-auto.mod should provide only
a function to execute this test (nearly same code as in 'file', but
simpler). We need not to determine fs exactly, we need only know that we
should try to load specified module. If this works not, then it's on
user to load the module (manualy or by grub.conf).

The format should be something as /etc/magic (or
/usr/share/misc/file/magic), but without the stuff about number of
heads, sectors and so on. If 'magic' of one filesystem can be written on
one line, we can have one file about autoloading as I decribed before.
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.46413298801

Re: Autoloading WAS: normal/cmdline bug & patch

[Tomas Ebenlendr]

> 
> The advantage of this approach is that you just have to load modules
> when there is a need for it.  The bad thing is that something like `ls
> (hd)' will become slower.
> 
Only the first time, or with low memory. I think there should be also
variable AUTOLOAD (defalt TRUE), which permits autoloading.


-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.46416666667

Re: normal/cmdline bug & patch

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

> If you applied some of patches in this thread before this patch, please
> undo them this is the only and final patch.

No, that is not it.  It just can't be applied.

Can you send the patch in an attachment?  (and preferable -up diffed)

Thanks,
Marco

Re: normal/cmdline bug & patch

[Tomas Ebenlendr]

> 
> Can you send the patch in an attachment?  (and preferable -up diffed)
> 
I did diff once more and compared the two diffs. They differed in some
whitespaces. I probably deleted them when viewing the patch with vim.
(I store my grub2 patches, because I want to be able reconstruct sources
when I mess them up by my debuging prints.)

Here is the correct patch:

Changelog (probably too much detail):
	Bugfix of grub_set_history.
	* normal/cmdline.c(grub_set_history):
	    fix1(55): hist_end points to first _empty_ entry
	    fix2(58,2): pos cannot overflow, but can underflow
	    fix3(65): correction must act as modulo
	    fix4(71)[hist_pos==hist_end]: copy nothing if hist_used == 0
	    fix5(73,9): buffer was copied wrong way

--- grub2_nonpatched/normal/cmdline.c	2004-06-18 23:05:53.000000000 +0200
+++ grub2_patched/normal/cmdline.c	2004-06-15 19:55:45.000000000 +0200
@@ -52,33 +52,35 @@ grub_set_history (int newsize)
 	  int delsize = hist_used - newsize;
 	  hist_used = newsize;
 
-	  for (i = 0; i < delsize; i++)
+	  for (i = 1; i <= delsize; i++)
 	    {
 	      int pos = hist_end - i;
-	      if (pos > hist_size)
-		pos -= hist_size;
+	      if (pos < 0)
+		pos += hist_size;
 	      grub_free (old_hist_lines[pos]);
 	    }
 
 	  hist_end -= delsize;
 	  if (hist_end < 0)
-	    hist_end = hist_size - hist_end;
+	    hist_end += hist_size;
 	}
 
       if (hist_pos < hist_end)
 	grub_memmove (hist_lines, old_hist_lines + hist_pos,
 		      (hist_end - hist_pos) * sizeof (char *));
-      else
+      else if (hist_used)
 	{
-	  /* Copy the first part.  */
-	  grub_memmove (hist_lines, old_hist_lines,
-			hist_pos * sizeof (char *));
-
+	  /* old_hist_lines: 0 <older part> hist_end <empty> hist_pos <newer part> */
+	  /* entry at hist_end is empty, at hist_pos contains first entry */
 
-	  /* Copy the last part.  */
-	  grub_memmove (hist_lines + hist_pos, old_hist_lines + hist_pos,
+	  /* Copy the older part.  */
+	  grub_memmove (hist_lines, old_hist_lines + hist_pos,
 			(hist_size - hist_pos) * sizeof (char *));
 
+	  /* Copy the newer part. */
+	  grub_memmove (hist_lines + hist_size - hist_pos, old_hist_lines,
+			hist_end * sizeof (char *));
+
 	}
     }
 
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.46438850309

Re: Autoloading WAS: normal/cmdline bug & patch

[Yoshinori K. Okuji]

So there are three ways for this approach:

A. Load all modules at the start-up time.

B. Load modules until a module succeeds to recognize a filesystem.

C. Load an appropriate module using signatures or magic.


A is very simple, but it always consumes memory unnecessarily.

B is also simple enough, but it also may consume memory unnecessarily.

C is very efficient, but it makes things a bit complex, because one 
filesystem module must provide two different detection mechanisms.

I'd like to vote for B. I like this simplicity. How about these 
procedures? This is a modified version of B:

for each loaded filesystem module:
  try the filesystem with a specified partition/disk
  return if successful
for each non-loaded filesystem module:
  load the filesystem module
  try the filesystem
  return if successful
  unload the filesystem module

This is memory-efficient (unless the dynamic loader has memory 
leaks...), and this is slow only at the first attempt of the 
partition/disk.

I prefer this to C, because I've seen the command "mount" in GNU/Linux 
not maintained all the time very well. I guess this is because the 
author of code for a filesystem may not use "mount -t auto ..." with 
the filesystem. This would happen even in GRUB, since you wouldn't 
notice that the autoload of your filesystem module is broken, if you 
preload it in core or explicitly load it manually or in a config file.

Okuji

Re: Autoloading WAS: normal/cmdline bug & patch

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> for each loaded filesystem module:
>   try the filesystem with a specified partition/disk
>   return if successful
> for each non-loaded filesystem module:
>   load the filesystem module
>   try the filesystem
>   return if successful
>   unload the filesystem module

This is like what I proposed, with the difference of unloading the
module.  I don't think the memory usage is *that* high.

For example, there are 15 filesystems that should be tried.  The
filesystems that we have now (ext2, fat, ufs and minixfs) are all
about 4KB.  I want to make some changes (sharing code) so that will
become smaller, but there will be more complex filesystem.  Let's
assume for now the average filesystem module will be 5KB.  In that
case (15*5) 75KB will be used as max.

To me that looks like low memory usage, less than reloading those
modules all the time.  For the apple it is no problem to use a lot of
memory.  I don't think there are problems for the pc, unless we have
to use <1MB.

So I think it is not needed to unload those filesystems and that
keeping those loaded will only boost performance.  If it is required
we can use (as Tomas proposed, IIRC) a user setable variable to
indicate if modules should be unloaded or not.

> I prefer this to C, because I've seen the command "mount" in GNU/Linux 
> not maintained all the time very well. I guess this is because the 
> author of code for a filesystem may not use "mount -t auto ..." with 
> the filesystem. This would happen even in GRUB, since you wouldn't 
> notice that the autoload of your filesystem module is broken, if you 
> preload it in core or explicitly load it manually or in a config file.

I agree.

--
Marco

Re: Autoloading WAS: normal/cmdline bug & patch

[Jeroen Dekkers]

At Sat, 19 Jun 2004 18:01:45 +0200,
Marco Gerards wrote:
> 
> "Yoshinori K. Okuji" <okuji@enbug.org> writes:
> 
> > for each loaded filesystem module:
> >   try the filesystem with a specified partition/disk
> >   return if successful
> > for each non-loaded filesystem module:
> >   load the filesystem module
> >   try the filesystem
> >   return if successful
> >   unload the filesystem module
> 
> This is like what I proposed, with the difference of unloading the
> module.  I don't think the memory usage is *that* high.
> 
> For example, there are 15 filesystems that should be tried.  The
> filesystems that we have now (ext2, fat, ufs and minixfs) are all
> about 4KB.  I want to make some changes (sharing code) so that will
> become smaller, but there will be more complex filesystem.  Let's
> assume for now the average filesystem module will be 5KB.  In that
> case (15*5) 75KB will be used as max.
> 
> To me that looks like low memory usage, less than reloading those
> modules all the time.  For the apple it is no problem to use a lot of
> memory.  I don't think there are problems for the pc, unless we have
> to use <1MB.
> 
> So I think it is not needed to unload those filesystems and that
> keeping those loaded will only boost performance.  If it is required
> we can use (as Tomas proposed, IIRC) a user setable variable to
> indicate if modules should be unloaded or not.

If you try to allocate memory and there isn't anything available, it
will first invalidate all caches and after that unload all unused
modules. So I don't think it will be a big problem.

Jeroen Dekkers

Re: Autoloading WAS: normal/cmdline bug & patch

[Tomas Ebenlendr]

...
> A. Load all modules at the start-up time.
> 
> B. Load modules until a module succeeds to recognize a filesystem.
> 
> C. Load an appropriate module using signatures or magic.
...

when we are talking about less than 16 modules of size less than 16k,
variant B seems to be the best for me. (With already implemented lazy
unloading of not used modules).

When this will be done, setup can be then changed to build 'core.img'
only with root filesystem by default. (Now I mean some 'easy' setup
which will build image and then install grub by one command.)
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.46765770467

Re: Autoloading WAS: normal/cmdline bug & patch

[Yoshinori K. Okuji]

On Saturday 19 June 2004 18:01, Marco Gerards wrote:
> So I think it is not needed to unload those filesystems and that
> keeping those loaded will only boost performance.  If it is required
> we can use (as Tomas proposed, IIRC) a user setable variable to
> indicate if modules should be unloaded or not.

Maybe. Since this is easily fixed, I have no strong taste on this. It's 
just one line anyway.

As Tomas also agrees, let's go in this way. Is there any other kind of 
modules which should be autoloaded?

Okuji

Re: normal/cmdline bug & patch

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

>> 
>> Can you send the patch in an attachment?  (and preferable -up diffed)
>> 
> I did diff once more and compared the two diffs. They differed in some
> whitespaces. I probably deleted them when viewing the patch with vim.
> (I store my grub2 patches, because I want to be able reconstruct sources
> when I mess them up by my debuging prints.)

Committed.  Thanks again for fixing this. :)

I've used this changelog entry to describe your changes:

2004-06-27  Tomas Ebenlendr  <ebik@ucw.cz>

	* normal/cmdline.c (grub_set_history): Fix off by one bug.  Fixed
	the history buffer logic.

Thanks,
Marco