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

PATCH In review Robert Millan 2 participants 4 messages Jul 1, 2008 – Jul 3, 2008

Share & tools

Robert Millan

author

2008-07-01 13:26 UTC #0 lore raw

PATCH 1/1 pc & gpt partmap iterators don't abort when their hook requests it

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

Signature · 5 lines

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 /.)

2008-07-01 Robert Millan <[email protected]>

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().

kern/device.c renamed from grub2/kern/device.c +1 -1

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/device.c ./kern/device.c
@@ -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;
     }

partmap/gpt.c renamed from grub2/partmap/gpt.c +2 -3

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/gpt.c ./partmap/gpt.c
@@ -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;

partmap/pc.c renamed from grub2/partmap/pc.c +2 -3

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/pc.c ./partmap/pc.c
@@ -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)
     {

Robert Millan

author

2008-07-01 15:34 UTC #1 lore raw

Quoted 4 lines · Robert Millan

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.

Signature · 5 lines

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 /.)

2008-07-01 Robert Millan <[email protected]>

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.

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

disk/lvm.c renamed from grub2/disk/lvm.c +1 -1

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/disk/lvm.c ./disk/lvm.c
@@ -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;

disk/raid.c renamed from grub2/disk/raid.c +1 -1

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/disk/raid.c ./disk/raid.c
@@ -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;

kern/device.c renamed from grub2/kern/device.c +1 -1

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/device.c ./kern/device.c
@@ -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;
     }

kern/partition.c renamed from grub2/kern/partition.c +2 -4

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/partition.c ./kern/partition.c
@@ -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);

partmap/gpt.c renamed from grub2/partmap/gpt.c +2 -3

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/gpt.c ./partmap/gpt.c
@@ -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;

partmap/pc.c renamed from grub2/partmap/pc.c +3 -4

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/partmap/pc.c ./partmap/pc.c
@@ -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)
     {

util/biosdisk.c renamed from grub2/util/biosdisk.c +1 -5

diff -x debian -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/util/biosdisk.c ./util/biosdisk.c
@@ -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)
       {

Marco Gerards

2008-07-03 18:22 UTC #2 lore raw

Quoted 11 lines · Robert Millan

Quoted 4 lines · Robert Millan
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

Robert Millan

author

2008-07-03 22:58 UTC #3 lore raw

Quoted 8 lines · Marco Gerards

Quoted 6 lines · Robert Millan
Quoted 4 lines · Robert Millan
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).

Quoted 8 lines

Quoted 5 lines
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).

Quoted 1 line

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.

Signature · 5 lines

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 /.)