Re: [PATCH] Backup old boot sectors before installation

[Zhu Yi <yi.zhu@intel.com>]

On Fri, 2009-12-11 at 17:39 +0800, Felix Zielcke wrote:
> 
> Someone already made a patch for this inside grub-setup
> See here and also for the discussion of it:
> http://lists.gnu.org/archive/html/grub-devel/2009-09/msg00242.html

Thanks. If I knew this patch, I won't try to write one my own. I did do
some basic search and asked in #grub2 IRC before writing it though.

Anyway, I did a quick review for Garimella's patch. Both the ideas of
the two patches are the same: backup the mbr and boot sectors will be
overwritten by core.img to a file before grub2 overwrites them. The
implementations slightly differ in:

1. The format of the backup file.

The old boot sectors and the start of embed_region position is required
for both patches. Garimella also added some redundant fields (i.e. image
size and 0xff) for integrity checking. I think it's a good idea. But
something like a md5/sha1 checksum should be even better.

2. The image backup and recovery.

Both of the patches choose to backup the old boot sectors in
grub-setup.c. On the recovery, Garimella selected to do it in
grub-setup.c and I used grub-install script with dd(1). I don't think
this is a very big deal and either way is OK. The reason I choose
grub-install is I want to keep the grub-setup.c as clean as possible.
Because given the backup file format, one can recover it with dd(1) even
without any help with grub-setup.

3. The backup policy.

Garimella's patch backups every time before grub-setup overwrote the
boot sectors. There might be a problem when someone run
grub-setup/grub-install twice. So the backup image ends up with the
grub2 boot sectors. This makes the recovery for the "real" boot sectors
impossible. My patch won't overwrite if a previous backup image already
existed. grub-install will remove the backup image after a successful
recovery.

4. Blocklists installation.

This is not very important because blocklists is not recommended. To be
completeness, my patch backups the mbr if blocklists are used due to
embedding is not possible. This will be detected by grub-install as
well.


In conclusion, I believe this backup feature is useful and either
implementation should do the work. If we can merge the good parts of
them, we can get a better one. I just curious why the thread is stuck
since September. Any blocking issues with it? Can we keep the topic
moving forward? Your comments are highly welcome.

Thanks,
-yi