Re: Migrating to GRUB 2 in Debian (Re: Interesting GSoC project ideas for 09)

[Yoshinori K. Okuji]

On Sunday 15 March 2009 14:52:05 Bean wrote:
> On Sun, Mar 15, 2009 at 5:10 AM, Yoshinori K. Okuji <address@hidden> wrote:
> > On Friday 13 March 2009 21:23:19 phcoder wrote:
> >> Look at load_env/save_env commands and grub-editenv util
> >
> > Thanks. Now I really regret that I didn't find those additions earlier.
> >
> > I do not like this implementation for the following reasons:
> >
> > - The saved file is not plain text, unlike GRUB Legacy. This is a bad
> > choice. Please let me know the reason why it must be binary, if any.
>
> Hi,
>
> As the command need to write to disk using blocklist information,
> which is not always correct (such as tail packing, sparse block), I
> use a magic header for verification. The length field is used to
> indicate the length of the block. because the command can't expand
> file, otherwise it would need to update fs information, which is too
> much for grub.

I have read your code deeply, and have found the following:

- in reality, you don't deal with tail packing, but just refuse it, because of 
this check in the hook function:

      if ((offset != 0) || (length != GRUB_DISK_SECTOR_SIZE))
        return;

- grub-editenv and save_env always write the magic at the beginning of a file, 
thus the magic does not make sense (besides an extremely conservative sanity 
check).

I would say that this is a regression from GRUB Legacy, which takes care of 
partial sector allocations gracefully.

So, assuming that every filesystem driver calls a read hook correctly, we 
should change it for:

- eliminating the magic header (although it could be kept for a safety guard 
for accidental writes)

- refusing to write, only if any sparse blocks are in use (as GRUB may not 
allocate new sectors physically)

- dealing with partial sectors - possibly due to tail packing - with some 
complicated code

I will work towards this direction. I will first fix up the sector handling 
and change the format to plain text. Naming changes are quite trivial, so 
they can be done later.

Regards,
Okuji

>
> > - The command names are ugly. Why didn't anybody follow Pavel's advise
> > using "env"?
> >
> > - The utility name is also ugly. I like Pavel's suggestion "grub-env".
> >
> > If nobody stops me, I will rewrite it in one week, without caring about
> > backward compatibility.
>
> I have no objection for the rename, although there should be two env
> commands, one to load and one to save.