[RFT,PATCH] Move embedding to appropriate partmap files

[RFT,PATCH] Move embedding to appropriate partmap files

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

Hello. A bug was filed that embedding code in grub-setup.c was
ignoring partmap metadata which could cause its overwriting. To avoid
usage of metadata and because of upcoming sunpc embedding support I
propose to move embedding to partmap/*.c. For this I devised an
interface which enables future reusage of embedding for other too if
necessary.

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: partfix.diff --]
[-- Type: text/plain, Size: 15923 bytes --]

diff --git a/include/grub/partition.h b/include/grub/partition.h
index 37c5f24..c162955 100644
--- a/include/grub/partition.h
+++ b/include/grub/partition.h
@@ -25,6 +25,13 @@ struct grub_disk;
 
 typedef struct grub_partition *grub_partition_t;
 
+#ifdef GRUB_UTIL
+typedef enum
+{
+  GRUB_EMBED_PCBIOS
+} grub_embed_type_t;
+#endif
+
 /* Partition map type.  */
 struct grub_partition_map
 {
@@ -43,6 +50,12 @@ struct grub_partition_map
   /* Return the name of the partition PARTITION.  */
   char *(*get_name) (const grub_partition_t partition);
 
+#ifdef GRUB_UTIL
+  /* Determine sectors available for embedding.  */
+  grub_err_t (*embed) (struct grub_disk *disk, unsigned int nsectors,
+		       grub_embed_type_t embed_type, grub_disk_addr_t *sectors);
+#endif
+
   /* The next partition map type.  */
   struct grub_partition_map *next;
 };
diff --git a/partmap/gpt.c b/partmap/gpt.c
index d646d41..0ac38a4 100644
--- a/partmap/gpt.c
+++ b/partmap/gpt.c
@@ -25,6 +25,13 @@
 #include <grub/pc_partition.h>
 #include <grub/gpt_partition.h>
 
+#ifdef GRUB_UTIL
+#include <stdio.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdlib.h>
+#endif
+
 static grub_uint8_t grub_gpt_magic[8] =
   {
     0x45, 0x46, 0x49, 0x20, 0x50, 0x41, 0x52, 0x54
@@ -32,6 +39,10 @@ static grub_uint8_t grub_gpt_magic[8] =
 
 static const grub_gpt_part_type_t grub_gpt_partition_type_empty = GRUB_GPT_PARTITION_TYPE_EMPTY;
 
+#ifdef GRUB_UTIL
+static const grub_gpt_part_type_t grub_gpt_partition_type_bios_boot = GRUB_GPT_PARTITION_TYPE_BIOS_BOOT;
+#endif
+
 static struct grub_partition_map grub_gpt_partition_map;
 
 \f
@@ -99,7 +110,7 @@ gpt_partition_map_iterate (grub_disk_t disk,
 			(unsigned long long) part.len);
 
 	  if (hook (disk, &part))
-	    return 1;
+	    return grub_errno;
 	}
 
       last_offset += grub_le_to_cpu32 (gpt.partentry_size);
@@ -172,6 +183,60 @@ gpt_partition_map_get_name (const grub_partition_t p)
   return name;
 }
 
+#ifdef GRUB_UTIL
+static grub_err_t
+gpt_partition_map_embed (struct grub_disk *disk, unsigned int nsectors,
+			 grub_embed_type_t embed_type,
+			 grub_disk_addr_t *sectors)
+{
+  grub_disk_addr_t start = 0, len = 0;
+  unsigned i;
+  grub_err_t err;
+
+  auto int NESTED_FUNC_ATTR find_usable_region (grub_disk_t disk,
+						const grub_partition_t p);
+  int NESTED_FUNC_ATTR find_usable_region (grub_disk_t disk __attribute__ ((unused)),
+					   const grub_partition_t p)
+    {
+      struct grub_gpt_partentry *gptdata = p->data;
+
+      /* If there's an embed region, it is in a dedicated partition.  */
+      if (! memcmp (&gptdata->type, &grub_gpt_partition_type_bios_boot, 16))
+	{
+	  start = p->start;
+	  len = p->len;
+
+	  return 1;
+	}
+
+      return 0;
+    }
+
+  if (embed_type != GRUB_EMBED_PCBIOS)
+    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+		       "GPT curently supports only PC-BIOS embedding");
+
+  err = gpt_partition_map_iterate (disk, find_usable_region);
+  if (err)
+    return err;
+
+  if (len == 0)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND,
+		       "This GPT partition label has no BIOS Boot Partition;"
+		       " embedding won't be possible!");
+
+  if (len < nsectors)
+    return grub_error (GRUB_ERR_OUT_OF_RANGE,
+		       "Your BIOS Boot Partition is too small;"
+		       " embedding won't be possible!");
+
+  for (i = 0; i < nsectors; i++)
+    sectors[i] = start + i;
+
+  return GRUB_ERR_NONE;
+}
+#endif
+
 \f
 /* Partition map type.  */
 static struct grub_partition_map grub_gpt_partition_map =
@@ -179,7 +244,10 @@ static struct grub_partition_map grub_gpt_partition_map =
     .name = "gpt_partition_map",
     .iterate = gpt_partition_map_iterate,
     .probe = gpt_partition_map_probe,
-    .get_name = gpt_partition_map_get_name
+    .get_name = gpt_partition_map_get_name,
+#ifdef GRUB_UTIL
+    .embed = gpt_partition_map_embed
+#endif
   };
 
 GRUB_MOD_INIT(gpt_partition_map)
diff --git a/partmap/pc.c b/partmap/pc.c
index 6a2efd2..466e790 100644
--- a/partmap/pc.c
+++ b/partmap/pc.c
@@ -153,7 +153,7 @@ pc_partition_map_iterate (grub_disk_t disk,
 	      pcdata.dos_part++;
 
 	      if (hook (disk, &p))
-		return 1;
+		return grub_errno;
 
 	      /* Check if this is a BSD partition.  */
 	      if (grub_pc_partition_is_bsd (e->type))
@@ -299,6 +299,107 @@ pc_partition_map_get_name (const grub_partition_t p)
   return name;
 }
 
+#ifdef GRUB_UTIL
+static grub_err_t
+pc_partition_map_embed (struct grub_disk *disk, unsigned int nsectors,
+			 grub_embed_type_t embed_type,
+			 grub_disk_addr_t *sectors)
+{
+  grub_disk_addr_t end = ~0ULL;
+  unsigned int i;
+  struct grub_pc_partition_mbr mbr;
+  struct grub_disk raw;
+  grub_uint32_t ext_offset, offset;
+  int index;
+  grub_err_t err;
+
+  if (embed_type != GRUB_EMBED_PCBIOS)
+    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
+		       "PC-style partitions curently support "
+		       "only PC-BIOS embedding");
+
+  /* Enforce raw disk access.  */
+  raw = *disk;
+  raw.partition = 0;
+
+  offset = 0;
+  ext_offset = 0;
+
+  while (1)
+    {
+      struct grub_pc_partition_entry *e;
+
+      /* Read the MBR.  */
+      err = grub_disk_read (&raw, offset, 0, sizeof (mbr), &mbr);
+      if (err)
+	return err;
+
+      /* Check if it is valid.  */
+      if (mbr.signature != grub_cpu_to_le16 (GRUB_PC_PARTITION_SIGNATURE))
+	return grub_error (GRUB_ERR_BAD_PART_TABLE, "no signature");
+
+      for (i = 0; i < 4; i++)
+	if (mbr.entries[i].flag & 0x7f)
+	  return grub_error (GRUB_ERR_BAD_PART_TABLE, "bad boot flag");
+
+      /* Analyze DOS partitions.  */
+      for (index = 0; index < 4; index++)
+	{
+	  e = mbr.entries + index;
+
+	  if (!grub_pc_partition_is_empty (e->type)
+	      && end > offset + grub_le_to_cpu32 (e->start))
+	    end = offset + grub_le_to_cpu32 (e->start);
+
+	  /* If this is a GPT partition, this MBR is just a dummy.  */
+	  if (e->type == GRUB_PC_PARTITION_TYPE_GPT_DISK && index == 0)
+	    return grub_error (GRUB_ERR_BAD_PART_TABLE, "dummy mbr");
+	}
+
+      /* Find an extended partition.  */
+      for (i = 0; i < 4; i++)
+	{
+	  e = mbr.entries + i;
+
+	  if (grub_pc_partition_is_extended (e->type))
+	    {
+	      offset = ext_offset + grub_le_to_cpu32 (e->start);
+	      if (! ext_offset)
+		ext_offset = offset;
+
+	      break;
+	    }
+	}
+
+      /* If no extended partition, the end.  */
+      if (i == 4)
+	break;
+    }
+
+  if (end < nsectors + 1)
+    {
+      if (end <= 1)
+	return grub_error (GRUB_ERR_FILE_NOT_FOUND,
+			   "This msdos-style partition label has no "
+			   "post-MBR gap; embedding won't be possible!");
+
+      if (nsectors > 62)
+	return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			   "Your core.img is unusually large.  "
+			   "It won't fit in the embedding area.");
+
+
+      return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			 "Your embedding area is unusually small.  "
+			 "core.img won't fit in it.");
+    }
+  for (i = 0; i < nsectors; i++)
+    sectors[i] = 1 + i;
+
+  return GRUB_ERR_NONE;
+}
+#endif
+
 \f
 /* Partition map type.  */
 static struct grub_partition_map grub_pc_partition_map =
@@ -306,7 +407,10 @@ static struct grub_partition_map grub_pc_partition_map =
     .name = "pc_partition_map",
     .iterate = pc_partition_map_iterate,
     .probe = pc_partition_map_probe,
-    .get_name = pc_partition_map_get_name
+    .get_name = pc_partition_map_get_name,
+#ifdef GRUB_UTIL
+    .embed = pc_partition_map_embed
+#endif
   };
 
 GRUB_MOD_INIT(pc_partition_map)
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 92c69ef..8f903c1 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -26,7 +26,6 @@
 #include <grub/fs.h>
 #include <grub/partition.h>
 #include <grub/pc_partition.h>
-#include <grub/gpt_partition.h>
 #include <grub/env.h>
 #include <grub/util/hostdisk.h>
 #include <grub/machine/boot.h>
@@ -35,8 +34,6 @@
 #include <grub/util/raid.h>
 #include <grub/util/lvm.h>
 
-static const grub_gpt_part_type_t grub_gpt_partition_type_bios_boot = GRUB_GPT_PARTITION_TYPE_BIOS_BOOT;
-
 #include <grub_setup_init.h>
 
 #include <stdio.h>
@@ -93,7 +90,7 @@ setup (const char *dir,
   size_t boot_size, core_size;
   grub_uint16_t core_sectors;
   grub_device_t root_dev, dest_dev;
-  const char *dest_partmap;
+  grub_partition_map_t dest_partmap;
   grub_uint8_t *boot_drive;
   grub_disk_addr_t *kernel_sector;
   grub_uint16_t *boot_drive_check;
@@ -108,52 +105,13 @@ setup (const char *dir,
   grub_uint16_t last_length = GRUB_DISK_SECTOR_SIZE;
   grub_file_t file;
   FILE *fp;
-  struct { grub_uint64_t start; grub_uint64_t end; } embed_region;
-  embed_region.start = embed_region.end = ~0UL;
+  grub_err_t err;
 
   auto void NESTED_FUNC_ATTR save_first_sector (grub_disk_addr_t sector, unsigned offset,
 			       unsigned length);
   auto void NESTED_FUNC_ATTR save_blocklists (grub_disk_addr_t sector, unsigned offset,
 			     unsigned length);
 
-  auto int NESTED_FUNC_ATTR find_usable_region_msdos (grub_disk_t disk,
-						      const grub_partition_t p);
-  int NESTED_FUNC_ATTR find_usable_region_msdos (grub_disk_t disk __attribute__ ((unused)),
-						 const grub_partition_t p)
-    {
-      struct grub_pc_partition *pcdata = p->data;
-
-      /* There's always an embed region, and it starts right after the MBR.  */
-      embed_region.start = 1;
-
-      /* For its end offset, include as many dummy partitions as we can.  */
-      if (! grub_pc_partition_is_empty (pcdata->dos_type)
-	  && ! grub_pc_partition_is_bsd (pcdata->dos_type)
-	  && embed_region.end > p->start)
-	embed_region.end = p->start;
-
-      return 0;
-    }
-
-  auto int NESTED_FUNC_ATTR find_usable_region_gpt (grub_disk_t disk,
-						    const grub_partition_t p);
-  int NESTED_FUNC_ATTR find_usable_region_gpt (grub_disk_t disk __attribute__ ((unused)),
-					       const grub_partition_t p)
-    {
-      struct grub_gpt_partentry *gptdata = p->data;
-
-      /* If there's an embed region, it is in a dedicated partition.  */
-      if (! memcmp (&gptdata->type, &grub_gpt_partition_type_bios_boot, 16))
-	{
-	  embed_region.start = p->start;
-	  embed_region.end = p->start + p->len;
-
-	  return 1;
-	}
-
-      return 0;
-    }
-
   void NESTED_FUNC_ATTR save_first_sector (grub_disk_addr_t sector, unsigned offset,
 			  unsigned length)
     {
@@ -307,6 +265,20 @@ setup (const char *dir,
   grub_util_info ("dos partition is %d, bsd partition is %d",
 		  dos_part, bsd_part);
 
+  /* Clean out the blocklists.  */
+  block = first_block;
+  while (block->len)
+    {
+      block->start = 0;
+      block->len = 0;
+      block->segment = 0;
+
+      block--;
+
+      if ((char *) block <= core_img)
+	grub_util_error ("No terminator in the core image");
+    }
+
   if (! dest_dev->disk->has_partitions)
     {
       grub_util_warn ("Attempting to install GRUB to a partitionless disk.  This is a BAD idea.");
@@ -326,7 +298,7 @@ setup (const char *dir,
   int NESTED_FUNC_ATTR identify_partmap (grub_disk_t disk __attribute__ ((unused)),
 					 const grub_partition_t p)
     {
-      dest_partmap = p->partmap->name;
+      dest_partmap = p->partmap;
       return 1;
     }
   dest_partmap = 0;
@@ -338,62 +310,52 @@ setup (const char *dir,
       goto unable_to_embed;
     }
 
-  grub_partition_iterate (dest_dev->disk, (strcmp (dest_partmap, "pc_partition_map") ?
-					   find_usable_region_gpt : find_usable_region_msdos));
+  if (strcmp (dest_partmap->name, "pc_partition_map") != 0
+      && strcmp (dest_partmap->name, "gpt_partition_map") != 0)
+    grub_util_error ("Can't install on '%s'", dest_partmap);
 
-  if (embed_region.end == embed_region.start)
+  if (!dest_partmap->embed)
     {
-      if (! strcmp (dest_partmap, "pc_partition_map"))
-	grub_util_warn ("This msdos-style partition label has no post-MBR gap; embedding won't be possible!");
-      else
-	grub_util_warn ("This GPT partition label has no BIOS Boot Partition; embedding won't be possible!");
+      grub_util_warn ("Partition style '%s' doesn't support embeding",
+		      dest_partmap->name);
       goto unable_to_embed;
     }
 
-  if ((unsigned long) core_sectors > embed_region.end - embed_region.start)
-    {
-      if (core_sectors > 62)
-	grub_util_warn ("Your core.img is unusually large.  It won't fit in the embedding area.");
-      else if (embed_region.end - embed_region.start < 62)
-	grub_util_warn ("Your embedding area is unusually small.  core.img won't fit in it.");
-      else
-	grub_util_warn ("Embedding area is too small for core.img.");
-      goto unable_to_embed;
-    }
-
-
-  grub_util_info ("will embed the core image at sector 0x%llx", embed_region.start);
-
-  *install_dos_part = grub_cpu_to_le32 (dos_part);
-  *install_bsd_part = grub_cpu_to_le32 (bsd_part);
-
-  /* The first blocklist contains the whole sectors.  */
-  first_block->start = grub_cpu_to_le64 (embed_region.start + 1);
-  first_block->len = grub_cpu_to_le16 (core_sectors - 1);
-  first_block->segment
-    = grub_cpu_to_le16 (GRUB_BOOT_MACHINE_KERNEL_SEG
-			+ (GRUB_DISK_SECTOR_SIZE >> 4));
-
-  /* Make sure that the second blocklist is a terminator.  */
-  block = first_block - 1;
-  block->start = 0;
-  block->len = 0;
-  block->segment = 0;
-
-  /* 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;
-
-  *kernel_sector = grub_cpu_to_le64 (embed_region.start);
-
-  /* Write the boot image onto the disk.  */
-  if (grub_disk_write (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE,
-		       boot_img))
-    grub_util_error ("%s", grub_errmsg);
-
+  {
+    grub_disk_addr_t sectors[core_sectors];
+    int i;
+
+    err = dest_partmap->embed (dest_dev->disk, core_sectors,
+			       GRUB_EMBED_PCBIOS, sectors);
+
+    if (err)
+      {
+	grub_util_warn ("%s", grub_errmsg);
+	grub_errno = GRUB_ERR_NONE;
+	goto unable_to_embed;
+      }
+
+    save_first_sector (sectors[0], 0, GRUB_DISK_SECTOR_SIZE);
+
+    block = first_block;
+    for (i = 1; i < core_sectors; i++)
+      save_blocklists (sectors[i], 0, GRUB_DISK_SECTOR_SIZE);
+
+    /* FIXME: can this be skipped?  */
+    *boot_drive = 0xFF;
+
+    *install_dos_part = grub_cpu_to_le32 (dos_part);
+    *install_bsd_part = grub_cpu_to_le32 (bsd_part);
+
+    /* Write the core image onto the disk.  */
+    for (i = 0; i < core_sectors; i++)
+      grub_disk_write (dest_dev->disk, sectors[i], 0,
+		       (core_size - i * GRUB_DISK_SECTOR_SIZE
+			< GRUB_DISK_SECTOR_SIZE) ? core_size
+		       - i * GRUB_DISK_SECTOR_SIZE
+		       : GRUB_DISK_SECTOR_SIZE,
+		       core_img + i * GRUB_DISK_SECTOR_SIZE);
+  }
   goto finish;
 
 unable_to_embed:
@@ -480,20 +442,6 @@ unable_to_embed:
   if (i == MAX_TRIES)
     grub_util_error ("Cannot read `%s' correctly", core_path_dev);
 
-  /* Clean out the blocklists.  */
-  block = first_block;
-  while (block->len)
-    {
-      block->start = 0;
-      block->len = 0;
-      block->segment = 0;
-
-      block--;
-
-      if ((char *) block <= core_img)
-	grub_util_error ("No terminator in the core image");
-    }
-
   /* Now read the core image to determine where the sectors are.  */
   file = grub_file_open (core_path_dev);
   if (! file)
@@ -515,8 +463,6 @@ unable_to_embed:
   free (core_path_dev);
   free (tmp_img);
 
-  *kernel_sector = grub_cpu_to_le64 (first_sector);
-
   /* FIXME: can this be skipped?  */
   *boot_drive = 0xFF;
 
@@ -532,12 +478,14 @@ unable_to_embed:
   grub_util_write_image (core_img, GRUB_DISK_SECTOR_SIZE * 2, fp);
   fclose (fp);
 
+ finish:
+
+  *kernel_sector = grub_cpu_to_le64 (first_sector);
+
   /* Write the boot image onto the disk.  */
   if (grub_disk_write (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, boot_img))
     grub_util_error ("%s", grub_errmsg);
 
- finish:
-
   /* Sync is a Good Thing.  */
   sync ();
 

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Robert Millan]

On Tue, Aug 11, 2009 at 03:33:13PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Hello. A bug was filed that embedding code in grub-setup.c was
> ignoring partmap metadata which could cause its overwriting. To avoid
> usage of metadata and because of upcoming sunpc embedding support I
> propose to move embedding to partmap/*.c. For this I devised an
> interface which enables future reusage of embedding for other too if
> necessary.

Can this go in util/i386/pc/ instead?  This is basicaly ad-hoc code and
doesn't seem very meaningful in generic partmap/*.c.

-- 
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."

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Michal Suchanek]

2009/8/12 Robert Millan <rmh@aybabtu.com>:
> On Tue, Aug 11, 2009 at 03:33:13PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> Hello. A bug was filed that embedding code in grub-setup.c was
>> ignoring partmap metadata which could cause its overwriting. To avoid
>> usage of metadata and because of upcoming sunpc embedding support I
>> propose to move embedding to partmap/*.c. For this I devised an
>> interface which enables future reusage of embedding for other too if
>> necessary.
>
> Can this go in util/i386/pc/ instead?  This is basicaly ad-hoc code and
> doesn't seem very meaningful in generic partmap/*.c.
>

I wonder, it here any firmware that supports multiple partition formats?

On i386 the only requirement would be that the first sector contains a
loader signature but some BIOSes may be overly smart and perform some
checks on the partition table. Firmware on other platforms (like
OpenFrimware or EFI) might be more picky and require a valid partition
table it understands.

Thanks Michal

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Robert Millan]

On Wed, Aug 12, 2009 at 11:39:01AM +0200, Michal Suchanek wrote:
> 
> I wonder, it here any firmware that supports multiple partition formats?

GRUB does (GRUB+coreboot is a firmware).  Anyhow, we were talking about
BIOS, which is partmap-agnostic.

-- 
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."

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Vladimir 'phcoder' Serbinenko]

On Wed, Aug 12, 2009 at 2:38 AM, Robert Millan<rmh@aybabtu.com> wrote:
> On Tue, Aug 11, 2009 at 03:33:13PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> Hello. A bug was filed that embedding code in grub-setup.c was
>> ignoring partmap metadata which could cause its overwriting. To avoid
>> usage of metadata and because of upcoming sunpc embedding support I
>> propose to move embedding to partmap/*.c. For this I devised an
>> interface which enables future reusage of embedding for other too if
>> necessary.
>
> Can this go in util/i386/pc/ instead?  This is basicaly ad-hoc code and
> doesn't seem very meaningful in generic partmap/*.c.
We may want to embed more in the future. Actually I think it's not
ad-hoc. Basically partition map defines a function which gives back
the sectors available for embedding. This way no code outside partmap/
and parttool/ has to know partition format. It's beneficial for future
maintainance as if someone updates partition map definitions he may
forget that util/i386/pc/.. has another ad-hoc implementation
especially if he doesn't use BIOS himself
>
> --
> 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
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Robert Millan]

On Wed, Aug 12, 2009 at 02:52:12PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Wed, Aug 12, 2009 at 2:38 AM, Robert Millan<rmh@aybabtu.com> wrote:
> > On Tue, Aug 11, 2009 at 03:33:13PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> Hello. A bug was filed that embedding code in grub-setup.c was
> >> ignoring partmap metadata which could cause its overwriting. To avoid
> >> usage of metadata and because of upcoming sunpc embedding support I
> >> propose to move embedding to partmap/*.c. For this I devised an
> >> interface which enables future reusage of embedding for other too if
> >> necessary.
> >
> > Can this go in util/i386/pc/ instead?  This is basicaly ad-hoc code and
> > doesn't seem very meaningful in generic partmap/*.c.
> We may want to embed more in the future. Actually I think it's not
> ad-hoc. Basically partition map defines a function which gives back
> the sectors available for embedding.

Is embedding useful elsewhere?

-- 
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."

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Seth Goldberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 636 bytes --]

Hi,

>>> Can this go in util/i386/pc/ instead?  This is basicaly ad-hoc code and
>>> doesn't seem very meaningful in generic partmap/*.c.
>> We may want to embed more in the future. Actually I think it's not
>> ad-hoc. Basically partition map defines a function which gives back
>> the sectors available for embedding.
>
> Is embedding useful elsewhere?

   Yes!  Solaris currently embeds the legacy GRUB stage2 loader in a 
non-filesystem slice within the Solaris partition (and we put stage1 in the 
PBR, which allows us to play nicely with Linux distributions who like to jam 
their boot loaders in the MBR).

  --S

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Vladimir 'phcoder' Serbinenko]

>> We may want to embed more in the future. Actually I think it's not
>> ad-hoc. Basically partition map defines a function which gives back
>> the sectors available for embedding.
>
> Is embedding useful elsewhere?
Yes. Consider a world of checksummed filesystems. In such world you
can't change the contents of the file by just writing to its blocklist
since it will break the checksum. Similar problems exist with RAIDs
and LVMs. On some systems we can't put grub-env in a file. For these
cases we can embed grub-env somewhere where we can write it with ease
>
> --
> 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
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Robert Millan]

On Thu, Aug 13, 2009 at 10:53:23PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> We may want to embed more in the future. Actually I think it's not
> >> ad-hoc. Basically partition map defines a function which gives back
> >> the sectors available for embedding.
> >
> > Is embedding useful elsewhere?
> Yes. Consider a world of checksummed filesystems. In such world you
> can't change the contents of the file by just writing to its blocklist
> since it will break the checksum. Similar problems exist with RAIDs
> and LVMs. On some systems we can't put grub-env in a file. For these
> cases we can embed grub-env somewhere where we can write it with ease

Ok.  Feel free to use partmap/ then.  But please make sure #ifdefs only
enable those functions where they are going to be used.  That'd be
GRUB_MACHINE_PCBIOS for now, if later code in other ports relies on them,
this can be changed.

-- 
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."

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Thu, Aug 13, 2009 at 10:53:23PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>>>> We may want to embed more in the future. Actually I think it's not
>>>> ad-hoc. Basically partition map defines a function which gives back
>>>> the sectors available for embedding.
>>>>         
>>> Is embedding useful elsewhere?
>>>       
>> Yes. Consider a world of checksummed filesystems. In such world you
>> can't change the contents of the file by just writing to its blocklist
>> since it will break the checksum. Similar problems exist with RAIDs
>> and LVMs. On some systems we can't put grub-env in a file. For these
>> cases we can embed grub-env somewhere where we can write it with ease
>>     
>
> Ok.  Feel free to use partmap/ then.  But please make sure #ifdefs only
> enable those functions where they are going to be used.  That'd be
> GRUB_MACHINE_PCBIOS for now, if later code in other ports relies on them,
> this can be changed.
>
>   
This patch fixes an important bug - namely overwriting extended
partition tables


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Robert Millan]

On Fri, Oct 16, 2009 at 12:58:42PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Robert Millan wrote:
> > On Thu, Aug 13, 2009 at 10:53:23PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >   
> >>>> We may want to embed more in the future. Actually I think it's not
> >>>> ad-hoc. Basically partition map defines a function which gives back
> >>>> the sectors available for embedding.
> >>>>         
> >>> Is embedding useful elsewhere?
> >>>       
> >> Yes. Consider a world of checksummed filesystems. In such world you
> >> can't change the contents of the file by just writing to its blocklist
> >> since it will break the checksum. Similar problems exist with RAIDs
> >> and LVMs. On some systems we can't put grub-env in a file. For these
> >> cases we can embed grub-env somewhere where we can write it with ease
> >>     
> >
> > Ok.  Feel free to use partmap/ then.  But please make sure #ifdefs only
> > enable those functions where they are going to be used.  That'd be
> > GRUB_MACHINE_PCBIOS for now, if later code in other ports relies on them,
> > this can be changed.
> >
> >   
> This patch fixes an important bug - namely overwriting extended
> partition tables

Is there a simpler way to resolve this?  I don't object to the
restructuring you propose, but it seems too intrusive to do this
just a few hours before we release 1.97.

-- 
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."

Re: [RFT,PATCH] Move embedding to appropriate partmap files

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Fri, Oct 16, 2009 at 12:58:42PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>> Robert Millan wrote:
>>     
>>> On Thu, Aug 13, 2009 at 10:53:23PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>   
>>>       
>>>>>> We may want to embed more in the future. Actually I think it's not
>>>>>> ad-hoc. Basically partition map defines a function which gives back
>>>>>> the sectors available for embedding.
>>>>>>         
>>>>>>             
>>>>> Is embedding useful elsewhere?
>>>>>       
>>>>>           
>>>> Yes. Consider a world of checksummed filesystems. In such world you
>>>> can't change the contents of the file by just writing to its blocklist
>>>> since it will break the checksum. Similar problems exist with RAIDs
>>>> and LVMs. On some systems we can't put grub-env in a file. For these
>>>> cases we can embed grub-env somewhere where we can write it with ease
>>>>     
>>>>         
>>> Ok.  Feel free to use partmap/ then.  But please make sure #ifdefs only
>>> enable those functions where they are going to be used.  That'd be
>>> GRUB_MACHINE_PCBIOS for now, if later code in other ports relies on them,
>>> this can be changed.
>>>
>>>   
>>>       
>> This patch fixes an important bug - namely overwriting extended
>> partition tables
>>     
>
> Is there a simpler way to resolve this?  I don't object to the
> restructuring you propose, but it seems too intrusive to do this
> just a few hours before we release 1.97.
>
>   
Yes. It's my concern too. Solving this requires metadata info form
partition map which is normally hidden and new code may have
undiscovered bugs. On the other hand already present code has a bug but
which is unlikely to be triggered.
As a simple alternative one could keep current check + enumerte entries
in MBR - this should be enough.

-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git