For Wubi, I'd like to do something like this:

  set root=... # actually prepare_grub_to_access_device
  loopback loop0 /ubuntu/disks/root.disk
  set root=(loop0)
  linux /boot/...

Unfortunately, if you set root after running loopback, any attempts to
open files on the loopback device resolve the loopback file name
relative to the *new* root, not the root at the time loopback was
invoked, and so the above recurses until it runs out of stack. I think
it's fairly clear that only the root that was in place when you ran
loopback should be relevant to the loopback file name.

I can see two ways to fix this. One is to save and restore root in
grub_loopback_open while opening the loopback file, and the other is
simply to keep the loopback file open and stash it in the grub_loopback
structure. Having implemented both alternatives, I significantly prefer
the cleanliness of the latter and think it's likely to be more
efficient, but I'm attaching both alternatives to this mail. Please let
me know what you think.

Thanks,

-- 
Colin Watson                                       [cjwat...@ubuntu.com]
2009-09-10  Colin Watson  <cjwat...@ubuntu.com>

	* disk/loopback.c (struct grub_loopback): Add `root' member.
	(delete_loopback): Free `dev->root'.
	(grub_cmd_loopback): Save the `root' environment variable.
	(grub_loopback_open): Set the `root' environment variable
	temporarily to that which was in effect when the `loopback' command
	was run.

Index: disk/loopback.c
===================================================================
--- disk/loopback.c	(revision 2584)
+++ disk/loopback.c	(working copy)
@@ -23,10 +23,12 @@
 #include <grub/disk.h>
 #include <grub/mm.h>
 #include <grub/extcmd.h>
+#include <grub/env.h>
 
 struct grub_loopback
 {
   char *devname;
+  char *root;
   char *filename;
   int has_partitions;
   struct grub_loopback *next;
@@ -62,6 +64,7 @@
   *prev = dev->next;
 
   grub_free (dev->devname);
+  grub_free (dev->root);
   grub_free (dev->filename);
   grub_free (dev);
 
@@ -125,6 +128,13 @@
       return grub_errno;
     }
 
+  newdev->root = grub_strdup (grub_env_get ("root"));
+  if (! newdev->root)
+    {
+      grub_free (newdev);
+      return grub_errno;
+    }
+
   newdev->filename = grub_strdup (args[1]);
   if (! newdev->filename)
     {
@@ -161,6 +171,7 @@
 {
   grub_file_t file;
   struct grub_loopback *dev;
+  char *save_root;
 
   for (dev = loopback_list; dev; dev = dev->next)
     if (grub_strcmp (dev->devname, name) == 0)
@@ -169,9 +180,16 @@
   if (! dev)
     return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't open device");
 
+  save_root = grub_strdup (grub_env_get ("root"));
+  grub_env_set ("root", dev->root);
+
   file = grub_file_open (dev->filename);
   if (! file)
-    return grub_errno;
+    {
+      grub_env_set ("root", save_root);
+      grub_free (save_root);
+      return grub_errno;
+    }
 
   /* Use the filesize for the disk size, round up to a complete sector.  */
   disk->total_sectors = ((file->size + GRUB_DISK_SECTOR_SIZE - 1)
@@ -181,6 +199,9 @@
   disk->has_partitions = dev->has_partitions;
   disk->data = file;
 
+  grub_env_set ("root", save_root);
+  grub_free (save_root);
+
   return 0;
 }
 
2009-09-10  Colin Watson  <cjwat...@ubuntu.com>

	* disk/loopback.c (struct grub_loopback): Add `file' member.
	(delete_loopback): Close `dev->file'.
	(grub_cmd_loopback): Keep `file' open unless this command fails.
	Save it in the loopback structure for later.
	(grub_loopback_open): Use the saved file handle rather than opening
	the loopback file again.
	(grub_loopback_close): Remove.
	(grub_loopback_dev): Remove `close' member.

Index: disk/loopback.c
===================================================================
--- disk/loopback.c	(revision 2584)
+++ disk/loopback.c	(working copy)
@@ -28,6 +28,7 @@
 {
   char *devname;
   char *filename;
+  grub_file_t file;
   int has_partitions;
   struct grub_loopback *next;
 };
@@ -61,6 +62,7 @@
   /* Remove the device from the list.  */
   *prev = dev->next;
 
+  grub_file_close (dev->file);
   grub_free (dev->devname);
   grub_free (dev->filename);
   grub_free (dev);
@@ -90,9 +92,6 @@
   if (! file)
     return grub_errno;
 
-  /* Close the file, the only reason for opening it is validation.  */
-  grub_file_close (file);
-
   /* First try to replace the old device.  */
   for (newdev = loopback_list; newdev; newdev = newdev->next)
     if (grub_strcmp (newdev->devname, args[0]) == 0)
@@ -102,10 +101,12 @@
     {
       char *newname = grub_strdup (args[1]);
       if (! newname)
-	return grub_errno;
+	goto fail;
 
       grub_free (newdev->filename);
       newdev->filename = newname;
+      grub_file_close (newdev->file);
+      newdev->file = file;
 
       /* Set has_partitions when `--partitions' was used.  */
       newdev->has_partitions = state[1].set;
@@ -116,13 +117,13 @@
   /* Unable to replace it, make a new entry.  */
   newdev = grub_malloc (sizeof (struct grub_loopback));
   if (! newdev)
-    return grub_errno;
+    goto fail;
 
   newdev->devname = grub_strdup (args[0]);
   if (! newdev->devname)
     {
       grub_free (newdev);
-      return grub_errno;
+      goto fail;
     }
 
   newdev->filename = grub_strdup (args[1]);
@@ -130,9 +131,11 @@
     {
       grub_free (newdev->devname);
       grub_free (newdev);
-      return grub_errno;
+      goto fail;
     }
 
+  newdev->file = file;
+
   /* Set has_partitions when `--partitions' was used.  */
   newdev->has_partitions = state[1].set;
 
@@ -141,6 +144,10 @@
   loopback_list = newdev;
 
   return 0;
+
+fail:
+  grub_file_close (file);
+  return grub_errno;
 }
 
 
@@ -159,7 +166,6 @@
 static grub_err_t
 grub_loopback_open (const char *name, grub_disk_t disk)
 {
-  grub_file_t file;
   struct grub_loopback *dev;
 
   for (dev = loopback_list; dev; dev = dev->next)
@@ -169,29 +175,17 @@
   if (! dev)
     return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't open device");
 
-  file = grub_file_open (dev->filename);
-  if (! file)
-    return grub_errno;
-
   /* Use the filesize for the disk size, round up to a complete sector.  */
-  disk->total_sectors = ((file->size + GRUB_DISK_SECTOR_SIZE - 1)
+  disk->total_sectors = ((dev->file->size + GRUB_DISK_SECTOR_SIZE - 1)
 			 / GRUB_DISK_SECTOR_SIZE);
   disk->id = (unsigned long) dev;
 
   disk->has_partitions = dev->has_partitions;
-  disk->data = file;
+  disk->data = dev->file;
 
   return 0;
 }
 
-static void
-grub_loopback_close (grub_disk_t disk)
-{
-  grub_file_t file = (grub_file_t) disk->data;
-
-  grub_file_close (file);
-}
-
 static grub_err_t
 grub_loopback_read (grub_disk_t disk, grub_disk_addr_t sector,
 		    grub_size_t size, char *buf)
@@ -233,7 +227,6 @@
     .id = GRUB_DISK_DEVICE_LOOPBACK_ID,
     .iterate = grub_loopback_iterate,
     .open = grub_loopback_open,
-    .close = grub_loopback_close,
     .read = grub_loopback_read,
     .write = grub_loopback_write,
     .next = 0
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Reply via email to