[PATCH] pc & gpt partmap iterators don't abort when their hook requests it

[PATCH] pc & gpt partmap iterators don't abort when their hook requests it

[Robert Millan]

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


See ChangeLog for description.  I'd really like to receive some review on
this one, since the code it touches is so fragile (although I tested it on a
typical setup and it works).

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: partmap_iterator_abortion.diff --]
[-- Type: text/x-diff, Size: 2874 bytes --]

2008-07-01  Robert Millan  <rmh@aybabtu.com>

	This fixes a performance issue when pc & gpt partmap iterators
	didn't abort iteration even after our hook found what it was
	looking for (often causing expensive probes of non-existant drives).

	The probe functions relied on previous buggy behaviour, since they
	would rise an error when their own hook (find_func()) caused early
	abortion of its iteration.

	* kern/device.c (grub_device_open): Improve error message.

	* partmap/pc.c (pc_partition_map_iterate): Abort parent iteration
	when hook requests it, independently of grub_errno.
	(pc_partition_map_probe): Do not fail when find_func() caused
	early abortion of pc_partition_map_iterate().

	* partmap/gpt.c (gpt_partition_map_iterate): Abort parent iteration
	when hook requests it, independently of grub_errno.
	(gpt_partition_map_probe): Do not fail when find_func() caused
	early abortion of gpt_partition_map_iterate().

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/device.c ./kern/device.c
--- ../grub2/kern/device.c	2008-01-10 00:25:54.000000000 +0100
+++ ./kern/device.c	2008-06-30 14:15:19.000000000 +0200
@@ -50,7 +50,7 @@ grub_device_open (const char *name)
   disk = grub_disk_open (name);
   if (! disk)
     {
-      grub_error (GRUB_ERR_BAD_DEVICE, "unknown device");
+      grub_error (GRUB_ERR_BAD_DEVICE, "unknown device %s", name);
       goto fail;
     }
 
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/gpt.c ./partmap/gpt.c
--- ../grub2/partmap/gpt.c	2008-02-23 21:33:32.000000000 +0100
+++ ./partmap/gpt.c	2008-07-01 15:06:31.000000000 +0200
@@ -102,7 +102,7 @@ gpt_partition_map_iterate (grub_disk_t d
 			i, part.start, part.len);
 
 	  if (hook (disk, &part))
-	    return grub_errno;
+	    return 1;
 	}
 
       last_offset += grub_le_to_cpu32 (gpt.partentry_size);
@@ -150,8 +150,7 @@ gpt_partition_map_probe (grub_disk_t dis
       return 0;
     }
 
-  if (gpt_partition_map_iterate (disk, find_func))
-    goto fail;
+  gpt_partition_map_iterate (disk, find_func);
 
   return p;
 
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/pc.c ./partmap/pc.c
--- ../grub2/partmap/pc.c	2007-07-22 01:32:30.000000000 +0200
+++ ./partmap/pc.c	2008-07-01 15:06:03.000000000 +0200
@@ -151,7 +151,7 @@ pc_partition_map_iterate (grub_disk_t di
 	      pcdata.dos_part++;
 	      
 	      if (hook (disk, &p))
-		goto finish;
+		return 1;
 
 	      /* Check if this is a BSD partition.  */
 	      if (grub_pc_partition_is_bsd (e->type))
@@ -255,8 +255,7 @@ pc_partition_map_probe (grub_disk_t disk
     return 0;
   
   pcdata = p->data;
-  if (pc_partition_map_iterate (disk, find_func))
-    goto fail;
+  pc_partition_map_iterate (disk, find_func);
 
   if (p->index < 0)
     {

Re: [PATCH] pc & gpt partmap iterators don't abort when their hook requests it

[Robert Millan]

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

On Tue, Jul 01, 2008 at 03:25:32PM +0200, Robert Millan wrote:
> 
> See ChangeLog for description.  I'd really like to receive some review on
> this one, since the code it touches is so fragile (although I tested it on a
> typical setup and it works).

Tough luck.  Inmediately after this I noticed it breaks grub-setup (I tested it
by loading core.img directly).

I found a few other callers that relied on the buggy behaviour.  Here's a new
patch.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: partmap_iterator_abortion.diff --]
[-- Type: text/x-diff, Size: 5877 bytes --]

2008-07-01  Robert Millan  <rmh@aybabtu.com>

	This fixes a performance issue when pc & gpt partmap iterators
	didn't abort iteration even after our hook found what it was
	looking for (often causing expensive probes of non-existant drives).

	Some callers relied on previous buggy behaviour, since they would
	rise an error when their own hooks caused early	abortion of its
	iteration.

	* kern/device.c (grub_device_open): Improve error message.
	* disk/lvm.c (grub_lvm_open): Likewise.
	* disk/raid.c (grub_raid_open): Likewise.

	* partmap/pc.c (pc_partition_map_iterate): Abort parent iteration
	when hook requests it, independently of grub_errno.
	(pc_partition_map_probe): Do not fail when find_func() caused
	early abortion of pc_partition_map_iterate().

	* partmap/gpt.c (gpt_partition_map_iterate): Abort parent iteration
	when hook requests it, independently of grub_errno.
	(gpt_partition_map_probe): Do not fail when find_func() caused
	early abortion of gpt_partition_map_iterate().

	* kern/partition.c (grub_partition_iterate): Abort parent iteration
	when hook requests it, independently of grub_errno.  Do not fail when
	part_map_iterate_hook() caused early abortion of p->iterate().

	* util/biosdisk.c (grub_util_biosdisk_get_grub_dev): Do not fail
	when grub_partition_iterate() returned with non-zero.

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/disk/lvm.c ./disk/lvm.c
--- ../grub2/disk/lvm.c	2008-05-30 12:41:54.000000000 +0200
+++ ./disk/lvm.c	2008-07-01 16:58:38.000000000 +0200
@@ -95,7 +95,7 @@ grub_lvm_open (const char *name, grub_di
     }
 
   if (! lv)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown device");
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown LVM device %s", name);
 
   disk->has_partitions = 0;
   disk->id = lv->number;
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/disk/raid.c ./disk/raid.c
--- ../grub2/disk/raid.c	2008-04-07 16:34:45.000000000 +0200
+++ ./disk/raid.c	2008-07-01 16:59:00.000000000 +0200
@@ -100,7 +100,7 @@ grub_raid_open (const char *name, grub_d
     }
 
   if (!array)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown device");
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown RAID device %s", name);
 
   disk->has_partitions = 1;
   disk->id = array->number;
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/device.c ./kern/device.c
--- ../grub2/kern/device.c	2008-07-01 15:50:21.000000000 +0200
+++ ./kern/device.c	2008-07-01 16:44:39.000000000 +0200
@@ -50,7 +50,7 @@ grub_device_open (const char *name)
   disk = grub_disk_open (name);
   if (! disk)
     {
-      grub_error (GRUB_ERR_BAD_DEVICE, "unknown device");
+      grub_error (GRUB_ERR_BAD_DEVICE, "unknown device %s", name);
       goto fail;
     }
 
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/partition.c ./kern/partition.c
--- ../grub2/kern/partition.c	2007-07-22 01:32:26.000000000 +0200
+++ ./kern/partition.c	2008-07-01 17:12:22.000000000 +0200
@@ -103,12 +103,10 @@ grub_partition_iterate (struct grub_disk
   
   int part_map_iterate (const grub_partition_map_t p)
     {
-      grub_err_t err;
-
       grub_dprintf ("partition", "Detecting %s...\n", p->name);
-      err = p->iterate (disk, part_map_iterate_hook);
+      p->iterate (disk, part_map_iterate_hook);
 
-      if (err != GRUB_ERR_NONE)
+      if (grub_errno != GRUB_ERR_NONE)
 	{
 	  /* Continue to next partition map type.  */
 	  grub_dprintf ("partition", "%s detection failed.\n", p->name);
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/gpt.c ./partmap/gpt.c
--- ../grub2/partmap/gpt.c	2008-07-01 15:50:21.000000000 +0200
+++ ./partmap/gpt.c	2008-07-01 16:44:39.000000000 +0200
@@ -102,7 +102,7 @@ gpt_partition_map_iterate (grub_disk_t d
 			i, part.start, part.len);
 
 	  if (hook (disk, &part))
-	    return grub_errno;
+	    return 1;
 	}
 
       last_offset += grub_le_to_cpu32 (gpt.partentry_size);
@@ -150,8 +150,7 @@ gpt_partition_map_probe (grub_disk_t dis
       return 0;
     }
 
-  if (gpt_partition_map_iterate (disk, find_func))
-    goto fail;
+  gpt_partition_map_iterate (disk, find_func);
 
   return p;
 
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/pc.c ./partmap/pc.c
--- ../grub2/partmap/pc.c	2008-07-01 15:50:21.000000000 +0200
+++ ./partmap/pc.c	2008-07-01 16:45:25.000000000 +0200
@@ -151,7 +151,7 @@ pc_partition_map_iterate (grub_disk_t di
 	      pcdata.dos_part++;
 	      
 	      if (hook (disk, &p))
-		goto finish;
+		return 1;
 
 	      /* Check if this is a BSD partition.  */
 	      if (grub_pc_partition_is_bsd (e->type))
@@ -190,7 +190,7 @@ pc_partition_map_iterate (grub_disk_t di
 		      
 		      if (be->fs_type != GRUB_PC_PARTITION_BSD_TYPE_UNUSED)
 			if (hook (disk, &p))
-			  goto finish;
+			  return 1;
 		    }
 		}
 	    }
@@ -255,8 +255,7 @@ pc_partition_map_probe (grub_disk_t disk
     return 0;
   
   pcdata = p->data;
-  if (pc_partition_map_iterate (disk, find_func))
-    goto fail;
+  pc_partition_map_iterate (disk, find_func);
 
   if (p->index < 0)
     {
diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/util/biosdisk.c ./util/biosdisk.c
--- ../grub2/util/biosdisk.c	2008-07-01 15:50:21.000000000 +0200
+++ ./util/biosdisk.c	2008-07-01 17:25:19.000000000 +0200
@@ -863,11 +863,7 @@ grub_util_biosdisk_get_grub_dev (const c
     if (! disk)
       return 0;
     
-    if (grub_partition_iterate (disk, find_partition) != GRUB_ERR_NONE)
-      {
-	grub_disk_close (disk);
-	return 0;
-      }
+    grub_partition_iterate (disk, find_partition);
     
     if (dos_part < 0)
       {

Re: [PATCH] pc & gpt partmap iterators don't abort when their hook requests it

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Tue, Jul 01, 2008 at 03:25:32PM +0200, Robert Millan wrote:
>> 
>> See ChangeLog for description.  I'd really like to receive some review on
>> this one, since the code it touches is so fragile (although I tested it on a
>> typical setup and it works).
>
> Tough luck.  Inmediately after this I noticed it breaks grub-setup (I tested it
> by loading core.img directly).
>
> I found a few other callers that relied on the buggy behaviour.  Here's a new
> patch.

Thanks for fixing this.  I had a quick look and it looks sane at first
sight.  Did you have a look at the other modules as well?

--
Marco

Re: [PATCH] pc & gpt partmap iterators don't abort when their hook requests it

[Robert Millan]

On Thu, Jul 03, 2008 at 08:31:05PM +0200, Marco Gerards wrote:
> Robert Millan <rmh@aybabtu.com> writes:
> 
> > On Tue, Jul 01, 2008 at 03:25:32PM +0200, Robert Millan wrote:
> >> 
> >> See ChangeLog for description.  I'd really like to receive some review on
> >> this one, since the code it touches is so fragile (although I tested it on a
> >> typical setup and it works).
> >
> > Tough luck.  Inmediately after this I noticed it breaks grub-setup (I tested it
> > by loading core.img directly).
> >
> > I found a few other callers that relied on the buggy behaviour.  Here's a new
> > patch.
> 
> Thanks for fixing this.  I had a quick look and it looks sane at first
> sight.

I just committed it (after fixing a pair of mistakes with grub_errno handling).

> Did you have a look at the other modules as well?

Yes.  At first glance, they don't appear to be affected.  However, since I
can't debug them, I wouldn't want to mess with them anyway.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)