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

[phcoder <phcoder@gmail.com>]

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 <okuji@enbug.org> 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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko