[PATCH] fix for partmap detection on RAID/LVM

[PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

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


This is my proposed approach for fixing the partmap detection problem in
LVM/RAID.

The problem: grub-probe just tests for partmap in the target drive directly
(which usually doesn't have any), and not in the physical drives that make
the RAID array (or LVM volume).

My proposed solution:  RAID implements grub_raid_parent, which returns a
linked list of "parent" (alternate naming suggestions welcome!) drives
conforming a specific array.  grub_disk_t abstracts that by providing a
parent() member (for GRUB_UTIL only) on top of it (so e.g. LVM can
implement the same).  grub-probe receives the list from this interface
and probes partmap in each member.

I had in mind something nifty with probe() recursing into itself, in order
to support bizarre combinations like lvm inside dm-raid, but on second
thoughts my current change is not intrusive and wouldn't preclude this kind
of restructuring if someone feels like it.

Comments?

-- 
Robert Millan

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

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

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../old/disk/raid.c ./disk/raid.c
--- ../old/disk/raid.c	2007-12-30 09:52:03.000000000 +0100
+++ ./disk/raid.c	2008-02-06 21:44:30.000000000 +0100
@@ -67,6 +67,26 @@ grub_raid_iterate (int (*hook) (const ch
   return 0;
 }
 
+#ifdef GRUB_UTIL
+static grub_parent_t
+grub_raid_parent (grub_disk_t disk)
+{
+  struct grub_raid_array *array = disk->data;
+  grub_parent_t list = NULL, tmp;
+  int i;
+  
+  for (i = 0; i < array->total_devs; i++)
+    {
+      tmp = grub_malloc (sizeof (*tmp));
+      tmp->disk = array->device[i];
+      tmp->next = list;
+      list = tmp;
+    }
+  
+  return list;
+}
+#endif
+
 static grub_err_t
 grub_raid_open (const char *name, grub_disk_t disk)
 {
@@ -524,6 +544,9 @@ static struct grub_disk_dev grub_raid_de
     .close = grub_raid_close,
     .read = grub_raid_read,
     .write = grub_raid_write,
+#ifdef GRUB_UTIL
+    .parent = grub_raid_parent,
+#endif
     .next = 0
   };
 
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../old/include/grub/disk.h ./include/grub/disk.h
--- ../old/include/grub/disk.h	2008-01-21 00:20:36.000000000 +0100
+++ ./include/grub/disk.h	2008-02-06 21:43:19.000000000 +0100
@@ -40,6 +40,9 @@ enum grub_disk_dev_id
   };
 
 struct grub_disk;
+#ifdef GRUB_UTIL
+struct grub_parent;
+#endif
 
 /* Disk device.  */
 struct grub_disk_dev
@@ -67,6 +70,10 @@ struct grub_disk_dev
   grub_err_t (*write) (struct grub_disk *disk, grub_disk_addr_t sector,
 		       grub_size_t size, const char *buf);
 
+#ifdef GRUB_UTIL
+  struct grub_parent *(*parent) (struct grub_disk *disk);
+#endif
+
   /* The next disk device.  */
   struct grub_disk_dev *next;
 };
@@ -105,6 +112,15 @@ struct grub_disk
 };
 typedef struct grub_disk *grub_disk_t;
 
+#ifdef GRUB_UTIL
+struct grub_parent
+{
+  grub_disk_t disk;
+  struct grub_parent *next;
+};
+typedef struct grub_parent *grub_parent_t;
+#endif
+
 /* The sector size.  */
 #define GRUB_DISK_SECTOR_SIZE	0x200
 #define GRUB_DISK_SECTOR_BITS	9
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../old/util/grub-probe.c ./util/grub-probe.c
--- ../old/util/grub-probe.c	2008-02-06 22:09:01.000000000 +0100
+++ ./util/grub-probe.c	2008-02-06 22:09:20.000000000 +0100
@@ -75,6 +75,31 @@ grub_refresh (void)
 }
 
 static void
+probe_partmap (grub_disk_t disk)
+{
+  char *name;
+  char *underscore;
+  
+  if (disk->partition == NULL)
+    {
+      grub_util_info ("No partition map found for %s", disk->name);
+      return;
+    }
+  
+  name = strdup (disk->partition->partmap->name);
+  if (! name)
+    grub_util_error ("Not enough memory");
+  
+  underscore = strchr (name, '_');
+  if (! underscore)
+    grub_util_error ("Invalid partition map %s", name);
+  
+  *underscore = '\0';
+  printf ("%s\n", name);
+  free (name);
+}
+
+static void
 probe (const char *path)
 {
   char *device_name;
@@ -133,23 +158,17 @@ probe (const char *path)
 
   if (print == PRINT_PARTMAP)
     {
-      char *name;
-      char *underscore;
-      
-      if (dev->disk->partition == NULL)
-        grub_util_error ("Cannot detect partition map for %s", drive_name);
-
-      name = strdup (dev->disk->partition->partmap->name);
-      if (! name)
-	grub_util_error ("not enough memory");
-
-      underscore = strchr (name, '_');
-      if (! underscore)
-	grub_util_error ("Invalid partition map %s", name);
-
-      *underscore = '\0';
-      printf ("%s\n", name);
-      free (name);
+      grub_parent_t list = NULL;
+      /* Check if dev->disk itself is contained in a partmap.  */
+      probe_partmap (dev->disk);
+      /* In case of LVM/RAID, check the parent devices as well.  */
+      if (dev->disk->dev->parent)
+	list = dev->disk->dev->parent (dev->disk);
+      while (list)
+	{
+	  probe_partmap (list->disk);
+	  list = list->next;
+	}
       goto end;
     }
 

Re: [PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

On Wed, Feb 06, 2008 at 10:18:40PM +0100, Robert Millan wrote:
> +      while (list)
> +	{
> +	  probe_partmap (list->disk);
> +	  list = list->next;
> +	}

Ah yes, I forgot to free() the structure here.

-- 
Robert Millan

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

Re: [PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

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


New patch to fix partmap detection in LVM/RAID.  Changes in comparison to
previous patch:

  - Add LVM detection.
  - Rename the structures/functions to make them more descriptive.
  - Fix a minor memleak.

This is only known to work on proof-of-concept test systems.  *Please* anyone
who was affected on real, production systems, test it (against latest CVS) and
send feedback.  The problem was that:

  grub-probe -t partmap /some_path

is unable to detect the partition map when /some_path was part of some LVM or
RAID abstraction.

Note for those of you using Debian packages: for testing this on CVS, you don't
need to install it, just build and run ./grub-probe in-tree.

Thanks

-- 
Robert Millan

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

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


	* disk/lvm.c [GRUB_UTIL] (grub_lvm_memberlist): New function.  Construct
	and return a grub_diskmemberlist_t composed of LVM physical volumes.
	[GRUB_UTIL] (grub_lvm_dev): Add `memberlist' member.

	* disk/raid.c [GRUB_UTIL] (grub_raid_memberlist): New function.  Construct
	and return a grub_diskmemberlist_t composed of physical array members.
	[GRUB_UTIL] (grub_raid_dev): Add `memberlist' member.

	* include/grub/disk.h [GRUB_UTIL] (grub_disk_memberlist): New struct
	prototype.
	[GRUB_UTIL] (struct grub_disk_dev): Add `memberlist' function pointer.
	[GRUB_UTIL] (struct grub_disk_memberlist): New struct declaration.
	[GRUB_UTIL] (grub_disk_memberlist_t): New typedef.

	* util/grub-probe.c (probe): Move partmap probing code from here ...
	(probe_partmap): ... to here.
	(probe): Use probe_partmap() once for the disk we're probing, and
	additionally, when such disk contains a memberlist() struct member,
	once for each disk that is contained in the structure returned by
	memberlist().

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/disk/lvm.c ./disk/lvm.c
--- ../grub2/disk/lvm.c	2007-07-22 01:32:20.000000000 +0200
+++ ./disk/lvm.c	2008-02-08 15:37:27.000000000 +0100
@@ -52,6 +52,26 @@ grub_lvm_iterate (int (*hook) (const cha
   return 0;
 }
 
+#ifdef GRUB_UTIL
+static grub_disk_memberlist_t
+grub_lvm_memberlist (grub_disk_t disk)
+{
+  struct grub_lvm_lv *lv = disk->data;
+  grub_disk_memberlist_t list = NULL, tmp;
+  struct grub_lvm_pv *pv;
+
+  for (pv = lv->vg->pvs; pv; pv = pv->next)
+    {
+      tmp = grub_malloc (sizeof (*tmp));
+      tmp->disk = pv->disk;
+      tmp->next = list;
+      list = tmp;
+    }
+
+  return list;
+}
+#endif
+
 static grub_err_t
 grub_lvm_open (const char *name, grub_disk_t disk)
 {
@@ -479,6 +499,9 @@ static struct grub_disk_dev grub_lvm_dev
     .close = grub_lvm_close,
     .read = grub_lvm_read,
     .write = grub_lvm_write,
+#ifdef GRUB_UTIL
+    .memberlist = grub_lvm_memberlist,
+#endif
     .next = 0
   };
 
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/disk/raid.c ./disk/raid.c
--- ../grub2/disk/raid.c	2008-02-08 13:35:05.000000000 +0100
+++ ./disk/raid.c	2008-02-08 15:37:05.000000000 +0100
@@ -67,6 +67,26 @@ grub_raid_iterate (int (*hook) (const ch
   return 0;
 }
 
+#ifdef GRUB_UTIL
+static grub_disk_memberlist_t
+grub_raid_memberlist (grub_disk_t disk)
+{
+  struct grub_raid_array *array = disk->data;
+  grub_disk_memberlist_t list = NULL, tmp;
+  int i;
+  
+  for (i = 0; i < array->total_devs; i++)
+    {
+      tmp = grub_malloc (sizeof (*tmp));
+      tmp->disk = array->device[i];
+      tmp->next = list;
+      list = tmp;
+    }
+  
+  return list;
+}
+#endif
+
 static grub_err_t
 grub_raid_open (const char *name, grub_disk_t disk)
 {
@@ -531,6 +551,9 @@ static struct grub_disk_dev grub_raid_de
     .close = grub_raid_close,
     .read = grub_raid_read,
     .write = grub_raid_write,
+#ifdef GRUB_UTIL
+    .memberlist = grub_raid_memberlist,
+#endif
     .next = 0
   };
 
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/include/grub/disk.h ./include/grub/disk.h
--- ../grub2/include/grub/disk.h	2008-01-21 00:20:36.000000000 +0100
+++ ./include/grub/disk.h	2008-02-08 15:37:05.000000000 +0100
@@ -40,6 +40,9 @@ enum grub_disk_dev_id
   };
 
 struct grub_disk;
+#ifdef GRUB_UTIL
+struct grub_disk_memberlist;
+#endif
 
 /* Disk device.  */
 struct grub_disk_dev
@@ -67,6 +70,10 @@ struct grub_disk_dev
   grub_err_t (*write) (struct grub_disk *disk, grub_disk_addr_t sector,
 		       grub_size_t size, const char *buf);
 
+#ifdef GRUB_UTIL
+  struct grub_disk_memberlist *(*memberlist) (struct grub_disk *disk);
+#endif
+
   /* The next disk device.  */
   struct grub_disk_dev *next;
 };
@@ -105,6 +112,15 @@ struct grub_disk
 };
 typedef struct grub_disk *grub_disk_t;
 
+#ifdef GRUB_UTIL
+struct grub_disk_memberlist
+{
+  grub_disk_t disk;
+  struct grub_disk_memberlist *next;
+};
+typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
+#endif
+
 /* The sector size.  */
 #define GRUB_DISK_SECTOR_SIZE	0x200
 #define GRUB_DISK_SECTOR_BITS	9
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/util/grub-probe.c ./util/grub-probe.c
--- ../grub2/util/grub-probe.c	2008-02-06 22:07:49.000000000 +0100
+++ ./util/grub-probe.c	2008-02-08 15:37:05.000000000 +0100
@@ -75,6 +75,31 @@ grub_refresh (void)
 }
 
 static void
+probe_partmap (grub_disk_t disk)
+{
+  char *name;
+  char *underscore;
+  
+  if (disk->partition == NULL)
+    {
+      grub_util_info ("No partition map found for %s", disk->name);
+      return;
+    }
+  
+  name = strdup (disk->partition->partmap->name);
+  if (! name)
+    grub_util_error ("Not enough memory");
+  
+  underscore = strchr (name, '_');
+  if (! underscore)
+    grub_util_error ("Invalid partition map %s", name);
+  
+  *underscore = '\0';
+  printf ("%s\n", name);
+  free (name);
+}
+
+static void
 probe (const char *path)
 {
   char *device_name;
@@ -133,23 +158,19 @@ probe (const char *path)
 
   if (print == PRINT_PARTMAP)
     {
-      char *name;
-      char *underscore;
-      
-      if (dev->disk->partition == NULL)
-        grub_util_error ("Cannot detect partition map for %s", drive_name);
-
-      name = strdup (dev->disk->partition->partmap->name);
-      if (! name)
-	grub_util_error ("not enough memory");
-
-      underscore = strchr (name, '_');
-      if (! underscore)
-	grub_util_error ("Invalid partition map %s", name);
-
-      *underscore = '\0';
-      printf ("%s\n", name);
-      free (name);
+      grub_disk_memberlist_t list = NULL, tmp;
+      /* Check if dev->disk itself is contained in a partmap.  */
+      probe_partmap (dev->disk);
+      /* In case of LVM/RAID, check the member devices as well.  */
+      if (dev->disk->dev->memberlist)
+	list = dev->disk->dev->memberlist (dev->disk);
+      while (list)
+	{
+	  probe_partmap (list->disk);
+	  tmp = list->next;
+	  free (list);
+	  list = tmp;
+	}
       goto end;
     }
 

Re: [PATCH] fix for partmap detection on RAID/LVM

[Sam Morris]

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

On Fri, 2008-02-08 at 15:52 +0100, Robert Millan wrote:
> New patch to fix partmap detection in LVM/RAID.  Changes in comparison to
> previous patch:

        (gdb) run -t partmap /boot/grub/
        Starting program: /home/sam/grub/grub2/grub-probe -t partmap /boot/grub/
        
        Program received signal SIGSEGV, Segmentation fault.
        0x0806035a in grub_raid_scan_device (name=0x806b080 "hd3,2") at disk/raid.c:442
        442	      if (array->device[sb.this_disk.number]->name != 0)
        
        (gdb) print array
        $1 = (struct grub_raid_array *) 0x806f1f8
        
        (gdb) print array->device
        $2 = {0x806b118, 0x0 <repeats 31 times>}
        
        (gdb) print sb.this_disk.number
        $3 = 1
        
        (gdb) bt full
        #0  0x0806035a in grub_raid_scan_device (name=0x806b080 "hd3,2")
            at disk/raid.c:442
        	err = <value optimized out>
        	disk = <value optimized out>
        	size = 578415649617870848
        	sb = {md_magic = 2838187772, major_version = 0, minor_version = 90, 
          patch_version = 3, gvalid_words = 0, set_uuid0 = 395026485, 
          ctime = 1139358627, level = 1, size = 292519488, nr_disks = 2, 
          raid_disks = 2, md_minor = 0, not_persistent = 0, set_uuid1 = 2852015029, 
          set_uuid2 = 1808090526, set_uuid3 = 3461511789, gstate_creserved = {
            0 <repeats 16 times>}, utime = 1202498002, state = 1, active_disks = 2, 
          working_disks = 2, failed_disks = 0, spare_disks = 0, sb_csum = 1132908899, 
          events_lo = 14301400, events_hi = 0, cp_events_lo = 14301400, 
          cp_events_hi = 0, recovery_cp = 4294967295, gstate_sreserved = {
            0 <repeats 20 times>}, layout = 0, chunk_size = 0, root_pv = 0, 
          root_block = 0, pstate_reserved = {0 <repeats 60 times>}, disks = {{
              number = 0, major = 3, minor = 66, raid_disk = 0, state = 6, reserved = {
                0 <repeats 27 times>}}, {number = 1, major = 34, minor = 2, 
              raid_disk = 1, state = 6, reserved = {0 <repeats 27 times>}}, {
              number = 0, major = 0, minor = 0, raid_disk = 0, state = 0, reserved = {
                0 <repeats 27 times>}} <repeats 25 times>}, reserved = 0xbfb8175c, 
          this_disk = {number = 1, major = 34, minor = 2, raid_disk = 1, state = 6, 
            reserved = {0 <repeats 27 times>}}}
        	p = (struct grub_raid_array *) 0x806f1f8
        	array = (struct grub_raid_array *) 0x806f1f8
        #1  0x0804b69e in iterate_partition (disk=0x806b008, partition=0xbfb81b94)
            at kern/device.c:132
        	partition_name = 0x806f378 "\001"
        	ret = 1
        	hook = (int (*)(const char *)) 0x80601c0 <grub_raid_scan_device>
        #2  0x0805cd0c in pc_partition_map_iterate (disk=0x806b008, hook=0xbfb81c9e)
            at partmap/pc.c:153
        	i = <value optimized out>
        	e = (struct grub_pc_partition_entry *) 0xbfb81a2a
        	p = {start = 1060290, len = 585039105, offset = 0, index = 1, 
          data = 0xbfb81bb8, partmap = 0x8063a1c}
        	pcdata = {dos_part = 1, bsd_part = -1, dos_type = 253, bsd_type = -1, 
          ext_offset = 0}
        	mbr = {
          code = "��\000\020\216м\000��\000\000\216�\216���\000|�\000\006�\000\002���!\006\000\000��\a8\004u\v\203�\020\201��\au��\026�\002�\001�\000|�\200\212t\001\213L\002�\023�\000|\000\000��", '\0' <repeats 365 times>, "\204�\002\000\000", 
          entries = {{flag = 0 '\0', start_head = 1 '\001', start_sector = 1 '\001', 
              start_cylinder = 0 '\0', type = 130 '\202', end_head = 254 '�', 
              end_sector = 63 '?', end_cylinder = 65 'A', start = 63, 
              length = 1060227}, {flag = 0 '\0', start_head = 0 '\0', 
              start_sector = 1 '\001', start_cylinder = 66 'B', type = 253 '
              end_head = 254 '�', end_sector = 255 '�', end_cylinder = 255 '�', 
              start = 1060290, length = 585039105}, {flag = 0 '\0', 
              start_head = 0 '\0', start_sector = 0 '\0', start_cylinder = 0 '\0', 
              type = 0 '\0', end_head = 0 '\0', end_sector = 0 '\0', 
              end_cylinder = 0 '\0', start = 0, length = 0}, {flag = 0 '\0', 
              start_head = 0 '\0', start_sector = 0 '\0', start_cylinder = 0 '\0', 
              type = 0 '\0', end_head = 0 '\0', end_sector = 0 '\0', 
              end_cylinder = 0 '\0', start = 0, length = 0}}, signature = 43605}
        	label = {magic = 0, padding = '\0' <repeats 127 times>, magic2 = 0, 
          checksum = 0, num_partitions = 0, boot_size = 0, superblock_size = 0, 
          entries = {{size = 0, offset = 0, fragment_size = 0, fs_type = 0 '\0', 
              fs_fragments = 0 '\0', fs_cylinders = 0}, {size = 0, offset = 0, 
              fragment_size = 0, fs_type = 0 '\0', fs_fragments = 0 '\0', 
              fs_cylinders = 0}, {size = 0, offset = 0, fragment_size = 0, 
              fs_type = 0 '\0', fs_fragments = 0 '\0', fs_cylinders = 0}, {
              size = 134656064, offset = 134625056, fragment_size = 3216513864, 
              fs_type = 255 '�', fs_fragments = 225 '�', fs_cylinders = 2053}, {
              size = 128, offset = 131, fragment_size = 0, fs_type = 0 '\0', 
              fs_fragments = 0 '\0', fs_cylinders = 0}, {size = 134625820, 
              offset = 3216514036, fragment_size = 3216513880, fs_type = 158 '\236', 
              fs_fragments = 227 '�', fs_cylinders = 2053}, {size = 0, offset = 0, 
              fragment_size = 3216513928, fs_type = 131 '\203', 
              fs_fragments = 220 '�', fs_cylinders = 2052}, {size = 134615777, 
              offset = 134625820, fragment_size = 0, fs_type = 255 '�', 
              fs_fragments = 255 '�', fs_cylinders = 65535}}}
        	raw = {name = 0x806b040 "hd3", dev = 0x8063720, 
          total_sectors = 586114704, has_partitions = 128, id = 131, partition = 0x0, 
          read_hook = 0, data = 0x0}
        #3  0x0804e1bf in grub_partition_iterate (hook=0xbfb81c9e)
            at kern/partition.c:126
        	ret = <value optimized out>
        	partmap = (grub_partition_map_t) 0x8063a1c
        	disk = (struct grub_disk *) 0x806b008
        #4  0x0804b812 in iterate_disk (disk_name=0xbfb81c42 "hd3")
            at kern/device.c:101
        	dev = (grub_device_t) 0x806b030
        	hook = (int (*)(const char *)) 0x80601c0 <grub_raid_scan_device>
        #5  0x08049810 in grub_util_biosdisk_iterate (hook=0xbfb81c94)
            at util/biosdisk.c:131
        	i = 131
        #6  0x0804b914 in grub_disk_dev_iterate (hook=0xbfb81c94) at kern/disk.c:205
        	p = (grub_disk_dev_t) 0x8063720
        #7  0x0804b614 in grub_device_iterate (hook=0x80601c0 <grub_raid_scan_device>)
            at kern/device.c:138
        No locals.
        #8  0x0805f9e2 in grub_mod_init (mod=0x0) at disk/raid.c:563
        No locals.
        #9  0x0805fa02 in grub_raid_init () at disk/raid.c:561
        No locals.
        #10 0x0804927c in main (argc=Cannot access memory at address 0xa9fe4bb5
        ) at util/grub-probe.c:338
        	c = <value optimized out>
        	dev_map = 0x0
        	path = 0xbfb83bd7 "/boot/grub/"
        

I'll debug this further later if you don't know why it happened.

-- 
Sam Morris
http://robots.org.uk/

PGP key id 1024D/5EA01078
3412 EA18 1277 354B 991B  C869 B219 7FDB 5EA0 1078

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

On Fri, Feb 08, 2008 at 07:20:31PM +0000, Sam Morris wrote:
> On Fri, 2008-02-08 at 15:52 +0100, Robert Millan wrote:
> > New patch to fix partmap detection in LVM/RAID.  Changes in comparison to
> > previous patch:
> 
>         (gdb) run -t partmap /boot/grub/
>         Starting program: /home/sam/grub/grub2/grub-probe -t partmap /boot/grub/
>         
>         Program received signal SIGSEGV, Segmentation fault.
>         0x0806035a in grub_raid_scan_device (name=0x806b080 "hd3,2") at disk/raid.c:442
>         442	      if (array->device[sb.this_disk.number]->name != 0)

I didn't touch this function.  I assume this was introduced with my previous
commit that redefined this structure.

.name used to be initialized altogether with .disk, so checking for .name
initialization amounts to checking for .disk initialization, which is what
we still have (but with a different name)).

So:

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp -N ../grub2/disk/raid.c ./disk/raid.c
--- ../grub2/disk/raid.c        2008-02-08 13:35:05.000000000 +0100
+++ ./disk/raid.c       2008-02-08 20:36:47.000000000 +0100
@@ -419,7 +419,7 @@ grub_raid_scan_device (const char *name)
          return 0;
        }

-      if (array->device[sb.this_disk.number]->name != 0)
+      if (array->device[sb.this_disk.number] != NULL)
        {
          /* We found multiple devices with the same number. Again,
             this shouldn't happen.*/

does this work?

-- 
Robert Millan

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

Re: [PATCH] fix for partmap detection on RAID/LVM

[Sam Morris]

On Fri, 08 Feb 2008 20:38:56 +0100, Robert Millan wrote:

> On Fri, Feb 08, 2008 at 07:20:31PM +0000, Sam Morris wrote:
>> On Fri, 2008-02-08 at 15:52 +0100, Robert Millan wrote:
>> > New patch to fix partmap detection in LVM/RAID.  Changes in
>> > comparison to previous patch:
>> 
>>         (gdb) run -t partmap /boot/grub/
>>         Starting program: /home/sam/grub/grub2/grub-probe -t partmap
>>         /boot/grub/
>>         
>>         Program received signal SIGSEGV, Segmentation fault. 0x0806035a
>>         in grub_raid_scan_device (name=0x806b080 "hd3,2") at
>>         disk/raid.c:442 442	      if
>>         (array->device[sb.this_disk.number]->name != 0)
> 
> I didn't touch this function.  I assume this was introduced with my
> previous commit that redefined this structure.
> 
> .name used to be initialized altogether with .disk, so checking for
> .name initialization amounts to checking for .disk initialization, which
> is what we still have (but with a different name)).
> 
> So:
> 
> diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp -N
> ../grub2/disk/raid.c ./disk/raid.c --- ../grub2/disk/raid.c       
> 2008-02-08 13:35:05.000000000 +0100 +++ ./disk/raid.c       2008-02-08
> 20:36:47.000000000 +0100 @@ -419,7 +419,7 @@ grub_raid_scan_device
> (const char *name)
>           return 0;
>         }
> 
> -      if (array->device[sb.this_disk.number]->name != 0) +      if
> (array->device[sb.this_disk.number] != NULL)
>         {
>           /* We found multiple devices with the same number. Again,
>              this shouldn't happen.*/
> 
> does this work?

Looks like it!

$ sudo ./grub-probe -t partmap /boot/grub/
pc
pc

-- 
Sam Morris
http://robots.org.uk/
 
PGP key id 1024D/5EA01078
3412 EA18 1277 354B 991B  C869 B219 7FDB 5EA0 1078

Re: [PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

On Fri, Feb 08, 2008 at 09:38:25PM +0000, Sam Morris wrote:
> > diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp -N
> > ../grub2/disk/raid.c ./disk/raid.c --- ../grub2/disk/raid.c       
> > 2008-02-08 13:35:05.000000000 +0100 +++ ./disk/raid.c       2008-02-08
> > 20:36:47.000000000 +0100 @@ -419,7 +419,7 @@ grub_raid_scan_device
> > (const char *name)
> >           return 0;
> >         }
> > 
> > -      if (array->device[sb.this_disk.number]->name != 0) +      if
> > (array->device[sb.this_disk.number] != NULL)
> >         {
> >           /* We found multiple devices with the same number. Again,
> >              this shouldn't happen.*/
> > 
> > does this work?
> 
> Looks like it!

Great.  I have committed the segfault bugfix (but not the partmap patch
yet).

> $ sudo ./grub-probe -t partmap /boot/grub/
> pc
> pc

Now that I think about it... does this cause grub-install / grub-mkimage
to load pc.mod twice?

-- 
Robert Millan

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

Re: [PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

On Fri, Feb 08, 2008 at 11:39:49PM +0100, Robert Millan wrote:
> > $ sudo ./grub-probe -t partmap /boot/grub/
> > pc
> > pc
> 
> Now that I think about it... does this cause grub-install / grub-mkimage
> to load pc.mod twice?

Ok, I see that it doesn't.  util/resolve.c takes care of that.

-- 
Robert Millan

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

Re: [PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

On Fri, Feb 08, 2008 at 03:52:37PM +0100, Robert Millan wrote:
> 
> 	* disk/lvm.c [GRUB_UTIL] (grub_lvm_memberlist): New function.  Construct
> 	and return a grub_diskmemberlist_t composed of LVM physical volumes.
> 	[GRUB_UTIL] (grub_lvm_dev): Add `memberlist' member.
> 
> 	* disk/raid.c [GRUB_UTIL] (grub_raid_memberlist): New function.  Construct
> 	and return a grub_diskmemberlist_t composed of physical array members.
> 	[GRUB_UTIL] (grub_raid_dev): Add `memberlist' member.
> 
> 	* include/grub/disk.h [GRUB_UTIL] (grub_disk_memberlist): New struct
> 	prototype.
> 	[GRUB_UTIL] (struct grub_disk_dev): Add `memberlist' function pointer.
> 	[GRUB_UTIL] (struct grub_disk_memberlist): New struct declaration.
> 	[GRUB_UTIL] (grub_disk_memberlist_t): New typedef.
> 
> 	* util/grub-probe.c (probe): Move partmap probing code from here ...
> 	(probe_partmap): ... to here.
> 	(probe): Use probe_partmap() once for the disk we're probing, and
> 	additionally, when such disk contains a memberlist() struct member,
> 	once for each disk that is contained in the structure returned by
> 	memberlist().

Committed.

-- 
Robert Millan

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

Re: [PATCH] fix for partmap detection on RAID/LVM

[Robert Millan]

On Mon, Feb 11, 2008 at 01:43:21PM +0100, Jan Nieuwenhuizen wrote:
> Robert Millan writes:
> 
> > This is only known to work on proof-of-concept test systems.  *Please* anyone
> > who was affected on real, production systems, test it (against latest CVS) and
> > send feedback.  The problem was that:
> > 
> >   grub-probe -t partmap /some_path
> > 
> > is unable to detect the partition map when /some_path was part of some LVM or
> > RAID abstraction.
> 
> Yes, works fine for me.
> 
> 13:41:19 janneke@peder:~/vc/grub2
> $ /home/janneke/pkg/grub2/sbin/grub-probe -t abstraction /boot
> lvm
> 13:41:23 janneke@peder:~/vc/grub2
> $ /home/janneke/pkg/grub2/sbin/grub-probe -t partmap /boot
> grub-probe: error: unknown device
> [1]13:41:33 janneke@peder:~/vc/grub2
> $ sudo /home/janneke/pkg/grub2/sbin/grub-probe -t partmap /boot
> pc

But the first time it reports unknown device?  What did you change?

-- 
Robert Millan

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