* [PATCH]: Save boot record before writing to the dest_drive
@ 2009-09-12 14:05 kashyap garimella
2009-09-14 15:35 ` Robert Millan
2009-09-23 8:08 ` Mikko Rantalainen
0 siblings, 2 replies; 9+ messages in thread
From: kashyap garimella @ 2009-09-12 14:05 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1.1: Type: text/plain, Size: 2966 bytes --]
Greetings!
I have added the following new features:
1) when grub-setup runs, it automatically stores the areas of (mbr + embed)
region, which are overwritten, into the file (in the following specified
format) in root directory.
2) grub-setup can restore the stored mbr in the following format back to
destination drive:
Format:
-------------------------------------------------------------------------------------------------
1st 8 bytes - for embed_region.start [ this is of type grub_uint64_t -
8bytes for sure]
next 8 bytes for the core_size, which is of size_t format. I put 8 bytes for
this because if we want to restore the backup file
from i386, it will not cause a problem.
next core_size bytes for core.img
next one byte contains 0xff - used as a signature bit, seperating the embed
and mbr regions
next GRUB_DISK_SECTOR_SIZE bytes for mbr
-------------------------------------------------------------------------------------------------
Typical operations:
1) grub-setup --help:
Usage: grub-setup [OPTION]... DEVICE
Set up images to boot from DEVICE.
DEVICE must be a GRUB device (e.g. ``(hd0,1)'').
-b, --boot-image=FILE use FILE as the boot image [default=boot.img]
-c, --core-image=FILE use FILE as the core image [default=core.img]
-Z, --backup-file=FILE use FILE as the backup/restore file
[default=bootsector.bak]
-d, --directory=DIR use GRUB files in the directory DIR
[default=/boot/grub]
-m, --device-map=FILE use FILE as the device map
[default=/boot/grub/device.map]
-r, --root-device=DEV use DEV as the root device [default=guessed]
-f, --force install even if problems are detected
-h, --help display this message and exit
-z, --restore restore the boot sectors
-V, --version print version information and exit
-v, --verbose print verbose messages
Therefore when we execute grub-setup -d <dir> /dev/sda (optionally -Z
backup-file name), it will create a file called bootsector.bak in <dir> and
write to it.
RESTORATION: (grub-setup --restore ) V ( grub-setup -z ) V (grub-setup -z -Z
backup_file) .... [ according to the modified help ]
PROBLEMS:
A warning:
util/i386/pc/grub-setup.c:93: warning: ‘backup_img’ may be used
uninitialized in this function
When I remove the line (496 in the patched one): free(backup_img);
the warning goes off.
I could not understand how to resolve it. I need some help
PATCH DETAILS:
Patch obtained by
svn di util/i386/pc/grub-setup.c
applyihng patch:
patch util/i386/pc/grub-setup.c patch_save_mbr
I tried my best to follow the coding standards and the style. I am very
enthusiastic in developing grub. Please suggest better format for storing
the boot sectors.
I hope I am not introducing more bugs.
Thank you,
Kashyap Garimella Jagannadh (irc: garimella )
Undergraduate student,
Indian Instition of Technology (IIT), Madras.
[-- Attachment #1.2: Type: text/html, Size: 3124 bytes --]
[-- Attachment #2: patch_save_mbr --]
[-- Type: application/octet-stream, Size: 8470 bytes --]
Index: util/i386/pc/grub-setup.c
===================================================================
--- util/i386/pc/grub-setup.c (revision 2592)
+++ util/i386/pc/grub-setup.c (working copy)
@@ -53,6 +53,7 @@
#define DEFAULT_BOOT_FILE "boot.img"
#define DEFAULT_CORE_FILE "core.img"
+#define DEFAULT_BACKUP_FILE "bootsector.bak"
/* This is the blocklist used in the diskboot image. */
struct boot_blocklist
@@ -85,11 +86,11 @@
static void
setup (const char *dir,
- const char *boot_file, const char *core_file,
- const char *root, const char *dest, int must_embed, int force)
+ const char *boot_file, const char *core_file, const char *backup_file,
+ const char *root, const char *dest, int must_embed, int force, int restore)
{
- char *boot_path, *core_path, *core_path_dev;
- char *boot_img, *core_img;
+ char *boot_path, *core_path, *backup_path, *core_path_dev;
+ char *boot_img, *core_img, *backup_img;
size_t boot_size, core_size;
grub_uint16_t core_sectors;
grub_device_t root_dev, dest_dev;
@@ -107,7 +108,7 @@
= GRUB_BOOT_MACHINE_KERNEL_SEG + (GRUB_DISK_SECTOR_SIZE >> 4);
grub_uint16_t last_length = GRUB_DISK_SECTOR_SIZE;
grub_file_t file;
- FILE *fp;
+ FILE *fp, *backup_fp;
struct { grub_uint64_t start; grub_uint64_t end; } embed_region;
embed_region.start = embed_region.end = ~0UL;
@@ -196,6 +197,73 @@
current_segment += GRUB_DISK_SECTOR_SIZE >> 4;
}
+ if(restore)
+ {
+ /* restore the boot sectors to the previous state */
+
+ /* open the backup file to restore the boot sectors */
+ backup_path = grub_util_get_path (dir, backup_file);
+ backup_img = grub_util_read_image (backup_path);
+
+ /* Open the destination device. */
+ dest_dev = grub_device_open (dest);
+ if (! dest_dev)
+ grub_util_error ("%s", grub_errmsg);
+
+ grub_util_info ("setting the root device to `%s'", root);
+ if (grub_env_set ("root", root) != GRUB_ERR_NONE)
+ grub_util_error ("%s", grub_errmsg);
+
+
+ /* read the image and get the variables */
+ int offset = 0;
+ grub_uint64_t start = *((grub_uint64_t *) backup_img);
+ offset += 8;
+
+ core_size = *((size_t *) (backup_img+offset));
+ offset += 8;
+
+ /* check the size of the backup image */
+ size_t backup_img_size = core_size + GRUB_DISK_SECTOR_SIZE + sizeof(grub_uint64_t) + sizeof(size_t) + 1 /* seperation byte 0xff */ ;
+ if( grub_util_get_image_size (backup_path) != backup_img_size)
+ {
+ strcpy(grub_errmsg,"Invalid size of the image\n");
+ goto unable_to_restore;
+ }
+
+ if( (unsigned char)backup_img[offset + core_size] != 0xff)
+ {
+ strcpy(grub_errmsg,"The backup file is corrupted\n");
+ goto unable_to_restore;
+ }
+
+ /* Write the core image on the disk. */
+ if (grub_disk_write (dest_dev->disk, start, 0, core_size, (char *)(backup_img+offset)))
+ grub_util_error ("%s", grub_errmsg);
+
+ /* the byte after the core image has 0xff in the file */
+ offset += core_size + 1;
+
+ /* Write the mbr on the disk. */
+ if (grub_disk_write (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, (char *)(backup_img+offset)))
+ grub_util_error ("%s", grub_errmsg);
+
+ /* free the used stuff */
+ free(backup_img);
+ grub_device_close (dest_dev);
+
+ return;
+
+unable_to_restore:
+ /* first free the used stuff */
+ free(backup_img);
+ grub_device_close (dest_dev);
+ /* then report errors */
+ grub_util_error("%s\nUnable to restore the boot sector",grub_errmsg);
+ return;
+
+ }
+
/* Read the boot image by the OS service. */
boot_path = grub_util_get_path (dir, boot_file);
boot_size = grub_util_get_image_size (boot_path);
@@ -380,12 +448,60 @@
block->len = 0;
block->segment = 0;
+ /* Save the boot sectors (including the embed region) before writing */
+ backup_path = grub_util_get_path (dir, backup_file);
+ backup_fp = fopen (backup_path, "w");
+
+ if (!backup_fp)
+ {
+ strcpy(grub_errmsg,"Unable to create the backup file");
+ goto unable_to_save;
+ }
+
+ /* 8 bytes for embed_region.start, another 8 for core_size */
+ int backup_img_size = core_size + GRUB_DISK_SECTOR_SIZE + 8 + 8 + 1 /* seperation byte 0xff */ ;
+ backup_img = (char *) xmalloc (backup_img_size);
+
+
+ int offset = 0;
+
+ memcpy (backup_img, (char *)(&embed_region.start), sizeof(grub_uint64_t));
+ offset += 8;
+
+ memcpy ( backup_img + offset, (char *)(&core_size), sizeof(size_t) );
+ offset += 8;
+
+ if(grub_disk_read( dest_dev->disk, embed_region.start, 0, core_size, backup_img+offset ))
+ goto unable_to_save;
+ offset += core_size;
+
+ backup_img[offset++] = 0xff; /* seperation byte between core img and mbr */
+
+ if(grub_disk_read( dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, backup_img+offset ))
+ goto unable_to_save;
+ offset += GRUB_DISK_SECTOR_SIZE;
+
+ /* write image to file */
+ fwrite (backup_img, 1, backup_img_size, backup_fp);
+
+ goto save_finish;
+
+unable_to_save:
+ grub_util_warn("error:%s\n",grub_errmsg);
+ grub_util_warn("Unable to save the boot sector\n");
+
+save_finish:
+ fclose (backup_fp);
+ free(backup_path);
+ free(backup_img);
+
+
/* Write the core image onto the disk. */
if (grub_disk_write (dest_dev->disk, embed_region.start, 0, core_size, core_img))
grub_util_error ("%s", grub_errmsg);
/* FIXME: can this be skipped? */
- *boot_drive = 0xFF;
+ *boot_drive = 0xff;
*kernel_sector = grub_cpu_to_le64 (embed_region.start);
@@ -555,9 +671,11 @@
{"directory", required_argument, 0, 'd'},
{"device-map", required_argument, 0, 'm'},
{"root-device", required_argument, 0, 'r'},
+ {"backup-file", required_argument, 0, 'Z'},
{"force", no_argument, 0, 'f'},
{"help", no_argument, 0, 'h'},
{"version", no_argument, 0, 'V'},
+ {"restore", no_argument, 0, 'z'},
{"verbose", no_argument, 0, 'v'},
{0, 0, 0, 0}
};
@@ -576,17 +694,19 @@
\n\
-b, --boot-image=FILE use FILE as the boot image [default=%s]\n\
-c, --core-image=FILE use FILE as the core image [default=%s]\n\
+ -Z, --backup-file=FILE use FILE as the backup/restore file [default=%s]\n\
-d, --directory=DIR use GRUB files in the directory DIR [default=%s]\n\
-m, --device-map=FILE use FILE as the device map [default=%s]\n\
-r, --root-device=DEV use DEV as the root device [default=guessed]\n\
-f, --force install even if problems are detected\n\
-h, --help display this message and exit\n\
+ -z, --restore restore the boot sectors\n\
-V, --version print version information and exit\n\
-v, --verbose print verbose messages\n\
\n\
Report bugs to <%s>.\n\
",
- DEFAULT_BOOT_FILE, DEFAULT_CORE_FILE, DEFAULT_DIRECTORY,
+ DEFAULT_BOOT_FILE, DEFAULT_CORE_FILE, DEFAULT_BACKUP_FILE, DEFAULT_DIRECTORY,
DEFAULT_DEVICE_MAP, PACKAGE_BUGREPORT);
exit (status);
@@ -609,18 +729,19 @@
{
char *boot_file = 0;
char *core_file = 0;
+ char *backup_file = 0;
char *dir = 0;
char *dev_map = 0;
char *root_dev = 0;
char *dest_dev;
- int must_embed = 0, force = 0;
+ int must_embed = 0, force = 0, restore = 0;
progname = "grub-setup";
/* Check for options. */
while (1)
{
- int c = getopt_long (argc, argv, "b:c:d:m:r:hVvf", options, 0);
+ int c = getopt_long (argc, argv, "b:c:d:m:r:Z:hVvfz", options, 0);
if (c == -1)
break;
@@ -678,6 +799,17 @@
verbosity++;
break;
+ case 'Z':
+ if (backup_file)
+ free (backup_file);
+
+ backup_file = xstrdup (optarg);
+ break;
+
+ case 'z':
+ restore=1;
+ break;
+
default:
usage (1);
break;
@@ -767,7 +899,8 @@
setup (dir ? : DEFAULT_DIRECTORY,
boot_file ? : DEFAULT_BOOT_FILE,
core_file ? : DEFAULT_CORE_FILE,
- root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force);
+ backup_file ? : DEFAULT_BACKUP_FILE,
+ root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force,restore);
}
}
else
@@ -776,7 +909,8 @@
setup (dir ? : DEFAULT_DIRECTORY,
boot_file ? : DEFAULT_BOOT_FILE,
core_file ? : DEFAULT_CORE_FILE,
- root_dev, dest_dev, must_embed, force);
+ backup_file ? : DEFAULT_BACKUP_FILE,
+ root_dev, dest_dev, must_embed, force,restore);
/* Free resources. */
grub_fini_all ();
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-12 14:05 [PATCH]: Save boot record before writing to the dest_drive kashyap garimella
@ 2009-09-14 15:35 ` Robert Millan
2009-09-14 15:44 ` kashyap garimella
2009-09-23 8:08 ` Mikko Rantalainen
1 sibling, 1 reply; 9+ messages in thread
From: Robert Millan @ 2009-09-14 15:35 UTC (permalink / raw)
To: The development of GRUB 2
On Sat, Sep 12, 2009 at 07:35:45PM +0530, kashyap garimella wrote:
> Greetings!
>
> I have added the following new features:
>
> 1) when grub-setup runs, it automatically stores the areas of (mbr + embed)
> region, which are overwritten, into the file (in the following specified
> format) in root directory.
>
> 2) grub-setup can restore the stored mbr in the following format back to
> destination drive:
Thanks for your interest, but I really think this is overkill. Users would
still need dd(1) to restore those images, so if they know dd(1), why can't
they just use that to back them up in first place?
--
Robert Millan
The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
how) you may access your data; but nobody's threatening your freedom: we
still allow you to remove your data and not access it at all."
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-14 15:35 ` Robert Millan
@ 2009-09-14 15:44 ` kashyap garimella
2009-09-19 21:10 ` Robert Millan
0 siblings, 1 reply; 9+ messages in thread
From: kashyap garimella @ 2009-09-14 15:44 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]
On Mon, Sep 14, 2009 at 9:05 PM, Robert Millan <rmh@aybabtu.com> wrote:
> On Sat, Sep 12, 2009 at 07:35:45PM +0530, kashyap garimella wrote:
> > Greetings!
> >
> > I have added the following new features:
> >
> > 1) when grub-setup runs, it automatically stores the areas of (mbr +
> embed)
> > region, which are overwritten, into the file (in the following specified
> > format) in root directory.
> >
> > 2) grub-setup can restore the stored mbr in the following format back to
> > destination drive:
>
> Thanks for your interest, but I really think this is overkill. Users would
> still need dd(1) to restore those images, so if they know dd(1), why can't
> they just use that to back them up in first place?
>
Sir,
I also added a restore option (grub-setup --restore dest_drive). A dd needs
to be done explicitly. But with this patch, it will (by default) store the
mbr and the areas in embed region ( which are written by grub ). And with
the backup file, you can restore
from a live cd also. The backup file contains the start sector and the
length of the core image.
There is also a request for this feature http://savannah.gnu.org/bugs/?27058
Thank you,
garimella kashyap
--
> Robert Millan
>
> The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
> how) you may access your data; but nobody's threatening your freedom: we
> still allow you to remove your data and not access it at all."
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
[-- Attachment #2: Type: text/html, Size: 2349 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-14 15:44 ` kashyap garimella
@ 2009-09-19 21:10 ` Robert Millan
2009-09-19 22:09 ` Colin Watson
0 siblings, 1 reply; 9+ messages in thread
From: Robert Millan @ 2009-09-19 21:10 UTC (permalink / raw)
To: The development of GRUB 2
On Mon, Sep 14, 2009 at 09:14:49PM +0530, kashyap garimella wrote:
> On Mon, Sep 14, 2009 at 9:05 PM, Robert Millan <rmh@aybabtu.com> wrote:
>
> > On Sat, Sep 12, 2009 at 07:35:45PM +0530, kashyap garimella wrote:
> > > Greetings!
> > >
> > > I have added the following new features:
> > >
> > > 1) when grub-setup runs, it automatically stores the areas of (mbr +
> > embed)
> > > region, which are overwritten, into the file (in the following specified
> > > format) in root directory.
> > >
> > > 2) grub-setup can restore the stored mbr in the following format back to
> > > destination drive:
> >
> > Thanks for your interest, but I really think this is overkill. Users would
> > still need dd(1) to restore those images, so if they know dd(1), why can't
> > they just use that to back them up in first place?
> >
> Sir,
>
> I also added a restore option (grub-setup --restore dest_drive). A dd needs
> to be done explicitly. But with this patch, it will (by default) store the
> mbr and the areas in embed region ( which are written by grub ). And with
> the backup file, you can restore
> from a live cd also. The backup file contains the start sector and the
> length of the core image.
I'm sorry, but I really see very limited usefulness in this. It's only
potentially useful to expert users, but those should know how to backup
sectors in their disk already.
But if others reading this think differently, please let it be known...
--
Robert Millan
The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
how) you may access your data; but nobody's threatening your freedom: we
still allow you to remove your data and not access it at all."
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-19 21:10 ` Robert Millan
@ 2009-09-19 22:09 ` Colin Watson
2009-09-20 6:50 ` richardvoigt
2009-09-20 11:22 ` Robert Millan
0 siblings, 2 replies; 9+ messages in thread
From: Colin Watson @ 2009-09-19 22:09 UTC (permalink / raw)
To: The development of GRUB 2
On Sat, Sep 19, 2009 at 11:10:49PM +0200, Robert Millan wrote:
> I'm sorry, but I really see very limited usefulness in this. It's only
> potentially useful to expert users, but those should know how to backup
> sectors in their disk already.
Of course, like many restore facilities, it's only useful if you
remembered or realised that you needed to do the backup before the data
was overwritten ...
It would be nice to have a straightforward answer to the sort of bug
report that goes "argh, I installed Debian/Ubuntu and it ate the special
magic boot loader I carefully installed there five years ago", I must
admin.
--
Colin Watson [cjwatson@ubuntu.com]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-19 22:09 ` Colin Watson
@ 2009-09-20 6:50 ` richardvoigt
2009-09-20 11:22 ` Robert Millan
1 sibling, 0 replies; 9+ messages in thread
From: richardvoigt @ 2009-09-20 6:50 UTC (permalink / raw)
To: The development of GRUB 2
On Sat, Sep 19, 2009 at 5:09 PM, Colin Watson <cjwatson@ubuntu.com> wrote:
> On Sat, Sep 19, 2009 at 11:10:49PM +0200, Robert Millan wrote:
>> I'm sorry, but I really see very limited usefulness in this. It's only
>> potentially useful to expert users, but those should know how to backup
>> sectors in their disk already.
>
> Of course, like many restore facilities, it's only useful if you
> remembered or realised that you needed to do the backup before the data
> was overwritten ...
As a user of grub, I think that automatically making a backup is nice,
as long as:
(1) I can specify not to make the backup
(2) It doesn't overwrite any existing file unless I explicitly specify
the filename
>
> It would be nice to have a straightforward answer to the sort of bug
> report that goes "argh, I installed Debian/Ubuntu and it ate the special
> magic boot loader I carefully installed there five years ago", I must
> admin.
>
> --
> Colin Watson [cjwatson@ubuntu.com]
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-19 22:09 ` Colin Watson
2009-09-20 6:50 ` richardvoigt
@ 2009-09-20 11:22 ` Robert Millan
1 sibling, 0 replies; 9+ messages in thread
From: Robert Millan @ 2009-09-20 11:22 UTC (permalink / raw)
To: The development of GRUB 2
On Sat, Sep 19, 2009 at 11:09:17PM +0100, Colin Watson wrote:
> On Sat, Sep 19, 2009 at 11:10:49PM +0200, Robert Millan wrote:
> > I'm sorry, but I really see very limited usefulness in this. It's only
> > potentially useful to expert users, but those should know how to backup
> > sectors in their disk already.
>
> Of course, like many restore facilities, it's only useful if you
> remembered or realised that you needed to do the backup before the data
> was overwritten ...
>
> It would be nice to have a straightforward answer to the sort of bug
> report that goes "argh, I installed Debian/Ubuntu and it ate the special
> magic boot loader I carefully installed there five years ago", I must
> admin.
So your point is that if backup is done automatically, this would be useful
when dealing with users who didn't plan ahead?
Seems reasonable.
--
Robert Millan
The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
how) you may access your data; but nobody's threatening your freedom: we
still allow you to remove your data and not access it at all."
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-12 14:05 [PATCH]: Save boot record before writing to the dest_drive kashyap garimella
2009-09-14 15:35 ` Robert Millan
@ 2009-09-23 8:08 ` Mikko Rantalainen
2009-09-24 15:45 ` kashyap garimella
1 sibling, 1 reply; 9+ messages in thread
From: Mikko Rantalainen @ 2009-09-23 8:08 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]
kashyap garimella wrote at 2009-09-12:
> Greetings!
>
> I have added the following new features:
>
> 1) when grub-setup runs, it automatically stores the areas of (mbr + embed)
> region, which are overwritten, into the file (in the following specified
> format) in root directory.
>
> 2) grub-setup can restore the stored mbr in the following format back to
> destination drive:
> [...]
>
> PROBLEMS:
> A warning:
> util/i386/pc/grub-setup.c:93: warning: ‘backup_img’ may be used
> uninitialized in this function
> When I remove the line (496 in the patched one): free(backup_img);
> the warning goes off.
> I could not understand how to resolve it. I need some help
I looked at the patch and it seems that the problem could be caused by
following code:
> + if (!backup_fp)
> + {
> + strcpy(grub_errmsg,"Unable to create the backup file");
> + goto unable_to_save;
> + }
> +
> + /* 8 bytes for embed_region.start, another 8 for core_size */
> + int backup_img_size = core_size + GRUB_DISK_SECTOR_SIZE + 8 + 8 + 1 /* seperation byte 0xff */ ;
> + backup_img = (char *) xmalloc (backup_img_size);
...
> +save_finish:
> + fclose (backup_fp);
> + free(backup_path);
> + free(backup_img);
Notice that you're using "goto" to skip initialization of backup_img and
you blindly free()ing it in "save_finish".
I think a nice way to fix this (if you want to use "goto") is to use
multiple clean up labels. Like this:
+save_cleanup_backup_img:
+ free(backup_img);
+save_cleanup_backup_fp:
+ fclose (backup_fp);
+save_cleanup_backup_path:
+ free(backup_path);
Note that you should release the resources in reverse order compared to
the order of acquiring the resources. If you need to bail out after you
have acquired the backup_fp but have not yet acquired the backup_img,
use "goto save_cleanup_backup_fp".
--
Mikko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH]: Save boot record before writing to the dest_drive
2009-09-23 8:08 ` Mikko Rantalainen
@ 2009-09-24 15:45 ` kashyap garimella
0 siblings, 0 replies; 9+ messages in thread
From: kashyap garimella @ 2009-09-24 15:45 UTC (permalink / raw)
To: The development of GRUB 2
[-- Attachment #1.1: Type: text/plain, Size: 2096 bytes --]
On Wed, Sep 23, 2009 at 1:38 PM, Mikko Rantalainen <
mikko.rantalainen@peda.net> wrote:
> I looked at the patch and it seems that the problem could be caused by
> following code:
>
> > + if (!backup_fp)
> > + {
> > + strcpy(grub_errmsg,"Unable to create the backup file");
> > + goto unable_to_save;
> > + }
> > +
> > + /* 8 bytes for embed_region.start, another 8 for core_size */
> > + int backup_img_size = core_size + GRUB_DISK_SECTOR_SIZE + 8 + 8 + 1
> /* seperation byte 0xff */ ;
> > + backup_img = (char *) xmalloc (backup_img_size);
> ...
> > +save_finish:
> > + fclose (backup_fp);
> > + free(backup_path);
> > + free(backup_img);
>
> Notice that you're using "goto" to skip initialization of backup_img and
> you blindly free()ing it in "save_finish".
>
> I think a nice way to fix this (if you want to use "goto") is to use
> multiple clean up labels. Like this:
>
> +save_cleanup_backup_img:
> + free(backup_img);
> +save_cleanup_backup_fp:
> + fclose (backup_fp);
> +save_cleanup_backup_path:
> + free(backup_path);
>
> Note that you should release the resources in reverse order compared to
> the order of acquiring the resources. If you need to bail out after you
> have acquired the backup_fp but have not yet acquired the backup_img,
> use "goto save_cleanup_backup_fp".
>
Thank you for the help. I followed your suggestion. I thought of putting a
check before freeing, but that did not remove the warning.
ChangeLog entry:
* util/i386/pc/grub-setup.c: Added sections for saving the boot + embed
sector before writing to them. Also added another
section for restoring the boot+embed sectors, given the backup file. The
backup file is relative to the root directory.
* grub-setup takes more options ( -Z to specify the backup file, -z to
restore the backup file )
I did not use indent(1), because it indented many other parts of the code,
which I did not write. I indented only my code.
Thank you,
Kashyap Garimella Jagannadh (irc: garimella )
Undergraduate student,
Department of Computer Science & Eng
Indian Instition of Technology (IIT), Madras.
[-- Attachment #1.2: Type: text/html, Size: 2661 bytes --]
[-- Attachment #2: patch_save_mbr_final --]
[-- Type: application/octet-stream, Size: 8543 bytes --]
Index: util/i386/pc/grub-setup.c
===================================================================
--- util/i386/pc/grub-setup.c (revision 2614)
+++ util/i386/pc/grub-setup.c (working copy)
@@ -53,6 +53,7 @@
#define DEFAULT_BOOT_FILE "boot.img"
#define DEFAULT_CORE_FILE "core.img"
+#define DEFAULT_BACKUP_FILE "bootsector.bak"
/* This is the blocklist used in the diskboot image. */
struct boot_blocklist
@@ -85,11 +86,11 @@
static void
setup (const char *dir,
- const char *boot_file, const char *core_file,
- const char *root, const char *dest, int must_embed, int force)
+ const char *boot_file, const char *core_file, const char *backup_file,
+ const char *root, const char *dest, int must_embed, int force, int restore)
{
- char *boot_path, *core_path, *core_path_dev;
- char *boot_img, *core_img;
+ char *boot_path, *core_path, *backup_path, *core_path_dev;
+ char *boot_img, *core_img, *backup_img;
size_t boot_size, core_size;
grub_uint16_t core_sectors;
grub_device_t root_dev, dest_dev;
@@ -107,7 +108,7 @@
= GRUB_BOOT_MACHINE_KERNEL_SEG + (GRUB_DISK_SECTOR_SIZE >> 4);
grub_uint16_t last_length = GRUB_DISK_SECTOR_SIZE;
grub_file_t file;
- FILE *fp;
+ FILE *fp, *backup_fp;
struct { grub_uint64_t start; grub_uint64_t end; } embed_region;
embed_region.start = embed_region.end = ~0UL;
@@ -196,6 +197,73 @@
current_segment += GRUB_DISK_SECTOR_SIZE >> 4;
}
+ if(restore)
+ {
+ /* restore the boot sectors to the previous state */
+
+ /* open the backup file to restore the boot sectors */
+ backup_path = grub_util_get_path (dir, backup_file);
+ backup_img = grub_util_read_image (backup_path);
+
+ /* Open the destination device. */
+ dest_dev = grub_device_open (dest);
+ if (! dest_dev)
+ grub_util_error ("%s", grub_errmsg);
+
+ grub_util_info ("setting the root device to `%s'", root);
+ if (grub_env_set ("root", root) != GRUB_ERR_NONE)
+ grub_util_error ("%s", grub_errmsg);
+
+
+ /* read the image and get the variables */
+ int offset = 0;
+ grub_uint64_t start = *((grub_uint64_t *) backup_img);
+ offset += 8;
+
+ core_size = *((size_t *) (backup_img+offset));
+ offset += 8;
+
+ /* check the size of the backup image */
+ size_t backup_img_size = core_size + GRUB_DISK_SECTOR_SIZE + sizeof(grub_uint64_t) + sizeof(size_t) + 1 /* seperation byte 0xff */ ;
+ if( grub_util_get_image_size (backup_path) != backup_img_size)
+ {
+ strcpy(grub_errmsg,"Invalid size of the image\n");
+ goto unable_to_restore;
+ }
+
+ if( (unsigned char)backup_img[offset + core_size] != 0xff)
+ {
+ strcpy(grub_errmsg,"The backup file is corrupted\n");
+ goto unable_to_restore;
+ }
+
+ /* Write the core image on the disk. */
+ if (grub_disk_write (dest_dev->disk, start, 0, core_size, (char *)(backup_img+offset)))
+ grub_util_error ("%s", grub_errmsg);
+
+ /* the byte after the core image has 0xff in the file */
+ offset += core_size + 1;
+
+ /* Write the mbr on the disk. */
+ if (grub_disk_write (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, (char *)(backup_img+offset)))
+ grub_util_error ("%s", grub_errmsg);
+
+ /* free the used stuff */
+ free(backup_img);
+ grub_device_close (dest_dev);
+
+ return;
+
+unable_to_restore:
+ /* first free the used stuff */
+ free(backup_img);
+ grub_device_close (dest_dev);
+ /* then report errors */
+ grub_util_error("%s\nUnable to restore the boot sector",grub_errmsg);
+ return;
+
+ }
+
/* Read the boot image by the OS service. */
boot_path = grub_util_get_path (dir, boot_file);
boot_size = grub_util_get_image_size (boot_path);
@@ -380,6 +448,63 @@
block->len = 0;
block->segment = 0;
+ /* Save the boot sectors (including the embed region) before writing */
+ backup_path = grub_util_get_path (dir, backup_file);
+ backup_fp = fopen (backup_path, "w");
+
+ if (!backup_fp)
+ {
+ grub_util_warn
+ ("Unable to save the boot sector. Reason: unable to open file\n");
+ goto save_cleanup_backup_fp;
+ }
+
+ /* 8 bytes for embed_region.start, another 8 for core_size, 1 byte for seperation */
+ int backup_img_size =
+ core_size + GRUB_DISK_SECTOR_SIZE + 8 + 8 + 1 /* seperation byte 0xff */ ;
+ backup_img = (char *) xmalloc (backup_img_size);
+
+
+ int offset = 0;
+
+ memcpy (backup_img, (char *) (&embed_region.start), sizeof (grub_uint64_t));
+ offset += 8;
+
+ memcpy (backup_img + offset, (char *) (&core_size), sizeof (size_t));
+ offset += 8;
+
+ if (grub_disk_read
+ (dest_dev->disk, embed_region.start, 0, core_size, backup_img + offset))
+ {
+ grub_util_warn ("Unable to save the boot sector. Reason:%s\n",
+ grub_errmsg);
+ goto save_cleanup_backup_img;
+ }
+ offset += core_size;
+
+ backup_img[offset++] = 0xff; /* seperation byte between core img and mbr */
+
+ if (grub_disk_read
+ (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, backup_img + offset))
+ {
+ grub_util_warn ("Unable to save the boot sector. Reason:%s\n",
+ grub_errmsg);
+ goto save_cleanup_backup_img;
+ }
+ offset += GRUB_DISK_SECTOR_SIZE;
+
+ /* write image to file */
+ fwrite (backup_img, 1, backup_img_size, backup_fp);
+
+ goto save_cleanup_backup_img;
+
+
+save_cleanup_backup_img:
+ free (backup_img);
+save_cleanup_backup_fp:
+ fclose (backup_fp);
+ free (backup_path);
+
/* Write the core image onto the disk. */
if (grub_disk_write (dest_dev->disk, embed_region.start, 0, core_size, core_img))
grub_util_error ("%s", grub_errmsg);
@@ -555,9 +680,11 @@
{"directory", required_argument, 0, 'd'},
{"device-map", required_argument, 0, 'm'},
{"root-device", required_argument, 0, 'r'},
+ {"backup-file", required_argument, 0, 'Z'},
{"force", no_argument, 0, 'f'},
{"help", no_argument, 0, 'h'},
{"version", no_argument, 0, 'V'},
+ {"restore", no_argument, 0, 'z'},
{"verbose", no_argument, 0, 'v'},
{0, 0, 0, 0}
};
@@ -576,17 +703,19 @@
\n\
-b, --boot-image=FILE use FILE as the boot image [default=%s]\n\
-c, --core-image=FILE use FILE as the core image [default=%s]\n\
+ -Z, --backup-file=FILE use FILE as the backup/restore file [default=%s]\n\
-d, --directory=DIR use GRUB files in the directory DIR [default=%s]\n\
-m, --device-map=FILE use FILE as the device map [default=%s]\n\
-r, --root-device=DEV use DEV as the root device [default=guessed]\n\
-f, --force install even if problems are detected\n\
-h, --help display this message and exit\n\
+ -z, --restore restore the boot sectors\n\
-V, --version print version information and exit\n\
-v, --verbose print verbose messages\n\
\n\
Report bugs to <%s>.\n\
",
- DEFAULT_BOOT_FILE, DEFAULT_CORE_FILE, DEFAULT_DIRECTORY,
+ DEFAULT_BOOT_FILE, DEFAULT_CORE_FILE, DEFAULT_BACKUP_FILE, DEFAULT_DIRECTORY,
DEFAULT_DEVICE_MAP, PACKAGE_BUGREPORT);
exit (status);
@@ -609,18 +738,19 @@
{
char *boot_file = 0;
char *core_file = 0;
+ char *backup_file = 0;
char *dir = 0;
char *dev_map = 0;
char *root_dev = 0;
char *dest_dev;
- int must_embed = 0, force = 0;
+ int must_embed = 0, force = 0, restore = 0;
progname = "grub-setup";
/* Check for options. */
while (1)
{
- int c = getopt_long (argc, argv, "b:c:d:m:r:hVvf", options, 0);
+ int c = getopt_long (argc, argv, "b:c:d:m:r:Z:hVvfz", options, 0);
if (c == -1)
break;
@@ -678,6 +808,17 @@
verbosity++;
break;
+ case 'Z':
+ if (backup_file)
+ free (backup_file);
+
+ backup_file = xstrdup (optarg);
+ break;
+
+ case 'z':
+ restore=1;
+ break;
+
default:
usage (1);
break;
@@ -767,7 +908,8 @@
setup (dir ? : DEFAULT_DIRECTORY,
boot_file ? : DEFAULT_BOOT_FILE,
core_file ? : DEFAULT_CORE_FILE,
- root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force);
+ backup_file ? : DEFAULT_BACKUP_FILE,
+ root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force,restore);
}
}
else
@@ -776,7 +918,8 @@
setup (dir ? : DEFAULT_DIRECTORY,
boot_file ? : DEFAULT_BOOT_FILE,
core_file ? : DEFAULT_CORE_FILE,
- root_dev, dest_dev, must_embed, force);
+ backup_file ? : DEFAULT_BACKUP_FILE,
+ root_dev, dest_dev, must_embed, force,restore);
/* Free resources. */
grub_fini_all ();
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-09-24 15:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-12 14:05 [PATCH]: Save boot record before writing to the dest_drive kashyap garimella
2009-09-14 15:35 ` Robert Millan
2009-09-14 15:44 ` kashyap garimella
2009-09-19 21:10 ` Robert Millan
2009-09-19 22:09 ` Colin Watson
2009-09-20 6:50 ` richardvoigt
2009-09-20 11:22 ` Robert Millan
2009-09-23 8:08 ` Mikko Rantalainen
2009-09-24 15:45 ` kashyap garimella
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.