grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Migrating to GRUB 2 in Debian (Re: Interesting GSoC project ideas fo


From: phcoder
Subject: Re: Migrating to GRUB 2 in Debian (Re: Interesting GSoC project ideas for 09)
Date: Sun, 22 Mar 2009 11:48:36 +0100
User-agent: Thunderbird 2.0.0.21 (X11/20090318)

Hello, I agree that non-sector aligned writes should be handled correctly. However I disagree with removing of the magic number. I personally would prefer if this file would have magic number and checksum. AFAIK currently grub2 doesn't write to FS except in load_env/save_env so a bug in code calling the hook could easily be present. And I don't want grub2 to corrupt the filesystem because of any such mistakes
Yoshinori K. Okuji wrote:
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.




_______________________________________________
Grub-devel mailing list
address@hidden
http://lists.gnu.org/mailman/listinfo/grub-devel


--

Regards
Vladimir 'phcoder' Serbinenko




reply via email to

[Prev in Thread] Current Thread [Next in Thread]