Re: [Patch] Robustly search for ZFS labels & uberblocks

[Richard Laager <rlaager@wiktel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 485 bytes --]

Attached is a stack of trivial patches that apply *on top of* your
zfs.diff from 2012-01-29. They each deal with one logical change and
should be very easy to review.

After your original patch and this stack have been dealt with, I'll
submit an updated patch for native ZFS on Linux support, which is much
shorter than before.

                                                                     -- 
                                                                 Richard

[-- Attachment #1.2: zfs-on-linux-rlaager0.patch --]
[-- Type: text/x-patch, Size: 1271 bytes --]

Eliminate stray trailing spaces.

Index: grub/util/grub-probe.c
===================================================================
--- grub.orig/util/grub-probe.c	2012-01-31 01:17:56.913537617 -0600
+++ grub/util/grub-probe.c	2012-01-31 01:18:20.531207000 -0600
@@ -269,7 +269,7 @@
   else
     printf ("%s", dname);
   free (dname);
-} 
+}
 
 static void
 probe_abstraction (grub_disk_t disk)
@@ -345,8 +345,8 @@
       grub_util_pull_device (*curdev);
       ndev++;
     }
-  
-  drives_names = xmalloc (sizeof (drives_names[0]) * (ndev + 1)); 
+
+  drives_names = xmalloc (sizeof (drives_names[0]) * (ndev + 1));
 
   for (curdev = device_names, curdrive = drives_names; *curdev; curdev++,
        curdrive++)
@@ -368,7 +368,7 @@
       dev = grub_device_open (drives_names[0]);
       if (! dev)
 	grub_util_error ("%s", _(grub_errmsg));
-      
+
       fs = grub_fs_probe (dev);
       if (! fs)
 	grub_util_error ("%s", _(grub_errmsg));
@@ -470,7 +470,7 @@
 	  grub_device_close (dev);
 	  continue;
 	}
-      
+
       if ((print == PRINT_COMPATIBILITY_HINT || print == PRINT_BIOS_HINT
 	   || print == PRINT_IEEE1275_HINT || print == PRINT_BAREMETAL_HINT
 	   || print == PRINT_EFI_HINT || print == PRINT_ARC_HINT)

[-- Attachment #1.3: zfs-on-linux-rlaager1.patch --]
[-- Type: text/x-patch, Size: 782 bytes --]

Change device to devices in find_root_devices_from_libzfs

Index: grub/util/getroot.c
===================================================================
--- grub.orig/util/getroot.c	2012-01-31 00:34:31.014033000 -0600
+++ grub/util/getroot.c	2012-01-31 00:36:16.026730000 -0600
@@ -512,7 +512,7 @@
 static char **
 find_root_devices_from_libzfs (const char *dir)
 {
-  char **device = NULL;
+  char **devices = NULL;
   char *poolname;
   char *poolfs;
 
@@ -520,13 +520,13 @@
   if (! poolname)
     return NULL;
 
-  device = find_root_devices_from_poolname (poolname);
+  devices = find_root_devices_from_poolname (poolname);
 
   free (poolname);
   if (poolfs)
     free (poolfs);
 
-  return device;
+  return devices;
 }
 
 #ifdef __MINGW32__

[-- Attachment #1.4: zfs-on-linux-rlaager2.patch --]
[-- Type: text/x-patch, Size: 794 bytes --]

Change strlen to sizeof

Index: grub/util/getroot.c
===================================================================
--- grub.orig/util/getroot.c	2012-01-31 00:36:16.026730000 -0600
+++ grub/util/getroot.c	2012-01-31 00:36:25.296311000 -0600
@@ -799,9 +799,9 @@
     grub_util_error (_("Storage name for `%s' not NUL-terminated"), dir);
 
   os_dev = xmalloc (2 * sizeof (os_dev[0]));
-  os_dev[0] = xmalloc (strlen ("/dev/") + data_len);
-  memcpy (os_dev[0], "/dev/", strlen ("/dev/"));
-  memcpy (os_dev[0] + strlen ("/dev/"), data, data_len);
+  os_dev[0] = xmalloc (sizeof ("/dev/") - 1 + data_len);
+  memcpy (os_dev[0], "/dev/", sizeof ("/dev/") - 1);
+  memcpy (os_dev[0] + sizeof ("/dev/") - 1, data, data_len);
   os_dev[1] = 0;
 
   if (ports && num_ports > 0)

[-- Attachment #1.5: zfs-on-linux-rlaager3.patch --]
[-- Type: text/x-patch, Size: 749 bytes --]

Avoid crashing when canonicalize_file_name() fails

I could reproduce this with: grub-probe /dev/sda1

Index: grub/util/getroot.c
===================================================================
--- grub.orig/util/getroot.c	2012-01-30 00:29:30.754018000 -0600
+++ grub/util/getroot.c	2012-01-30 23:31:58.941211000 -0600
@@ -842,7 +931,12 @@
 	{
 	  char *tmp = *cur;
 	  int root, dm;
-	  *cur = canonicalize_file_name (*cur);
+	  *cur = canonicalize_file_name (tmp);
+	  if (*cur == NULL)
+	    {
+	      grub_util_error (_("failed to get canonical path of %s"), tmp);
+	      break;
+	    }
 	  free (tmp);
 	  root = (strcmp (*cur, "/dev/root") == 0);
 	  dm = (strncmp (*cur, "/dev/dm-", sizeof ("/dev/dm-") - 1) == 0);

[-- Attachment #1.6: zfs-on-linux-rlaager4.patch --]
[-- Type: text/x-patch, Size: 478 bytes --]

Drop an unused variable.

Index: grub/util/getroot.c
===================================================================
--- grub.orig/util/getroot.c	2012-01-31 00:50:08.674484255 -0600
+++ grub/util/getroot.c	2012-01-31 00:50:24.200438000 -0600
@@ -293,7 +293,6 @@
 		&& !sscanf (name, "raidz%u", &dummy)
 		&& !strcmp (state, "ONLINE"))
 	      {
-		char *tmp;
 		if (ndevices >= devices_allocated)
 		  {
 		    devices_allocated = 2 * (devices_allocated + 8);

[-- Attachment #1.7: zfs-on-linux-rlaager5.patch --]
[-- Type: text/x-patch, Size: 2285 bytes --]

Add braces around and indent the `zpool status` parsing loop

The switch is one statement, so this doesn't change the actual effect
of the code, but it makes it more clear.  Also, if someone were to try
to add a debugging printf() after the if statement, it would work
correctly. ;)

Index: grub/util/getroot.c
===================================================================
--- grub.orig/util/getroot.c	2012-01-31 00:50:24.000000000 -0600
+++ grub/util/getroot.c	2012-01-31 00:51:15.352398000 -0600
@@ -274,36 +274,38 @@
 	
       if (sscanf (line, " %s %256s %256s %256s %256s %256s",
 		  name, state, readlen, writelen, cksum, notes) >= 5)
-	switch (st)
-	  {
-	  case 0:
-	    if (!strcmp (name, "NAME")
-		&& !strcmp (state, "STATE")
-		&& !strcmp (readlen, "READ")
-		&& !strcmp (writelen, "WRITE")
-		&& !strcmp (cksum, "CKSUM"))
-	      st++;
-	    break;
-	  case 1:
-	    if (!strcmp (name, poolname))
-	      st++;
-	    break;
-	  case 2:
-	    if (strcmp (name, "mirror") && !sscanf (name, "mirror-%u", &dummy)
-		&& !sscanf (name, "raidz%u", &dummy)
-		&& !strcmp (state, "ONLINE"))
-	      {
-		if (ndevices >= devices_allocated)
-		  {
-		    devices_allocated = 2 * (devices_allocated + 8);
-		    devices = xrealloc (devices, sizeof (devices[0])
-					* devices_allocated);
-		  }
-		devices[ndevices++] = xasprintf ("/dev/%s", name);
-	      }
-	    break;
-	  }
-	
+	{
+	  switch (st)
+	    {
+	    case 0:
+	      if (!strcmp (name, "NAME")
+		  && !strcmp (state, "STATE")
+		  && !strcmp (readlen, "READ")
+		  && !strcmp (writelen, "WRITE")
+		  && !strcmp (cksum, "CKSUM"))
+		st++;
+	      break;
+	    case 1:
+	      if (!strcmp (name, poolname))
+		st++;
+	      break;
+	    case 2:
+	      if (strcmp (name, "mirror") && !sscanf (name, "mirror-%u", &dummy)
+		  && !sscanf (name, "raidz%u", &dummy)
+		  && !strcmp (state, "ONLINE"))
+		{
+		  if (ndevices >= devices_allocated)
+		    {
+		      devices_allocated = 2 * (devices_allocated + 8);
+		      devices = xrealloc (devices, sizeof (devices[0])
+					  * devices_allocated);
+		    }
+		  devices[ndevices++] = xasprintf ("/dev/%s", name);
+	        }
+	      break;
+	    }
+	}
+
       free (line);
     }
 

[-- Attachment #1.8: zfs-on-linux-rlaager6.patch --]
[-- Type: text/x-patch, Size: 786 bytes --]

Handle pool names with trailing spaces

Index: grub/util/getroot.c
===================================================================
--- grub.orig/util/getroot.c	2012-01-31 01:05:12.387005000 -0600
+++ grub/util/getroot.c	2012-01-31 01:05:13.196100000 -0600
@@ -260,7 +260,7 @@
   char cksum[257], notes[257];
   unsigned int dummy;
 
-  cmd = xasprintf ("zpool status %s", poolname);
+  cmd = xasprintf ("zpool status \"%s\"", poolname);
   fp = popen (cmd, "r");
   free (cmd);
 
@@ -286,7 +286,8 @@
 		st++;
 	      break;
 	    case 1:
-	      if (!strcmp (name, poolname))
+              /* strncmp is used because pools can technically have trailing spaces. */
+	      if (!strncmp (name, poolname, strlen (name)))
 		st++;
 	      break;
 	    case 2:

[-- Attachment #1.9: zfs-on-linux-rlaager7.patch --]
[-- Type: text/x-patch, Size: 648 bytes --]

Handle all raidz types in `zpool status` output

Index: grub/util/getroot.c
===================================================================
--- grub.orig/util/getroot.c	2012-01-31 01:05:13.000000000 -0600
+++ grub/util/getroot.c	2012-01-31 01:06:53.856704000 -0600
@@ -293,6 +293,9 @@
 	    case 2:
 	      if (strcmp (name, "mirror") && !sscanf (name, "mirror-%u", &dummy)
 		  && !sscanf (name, "raidz%u", &dummy)
+		  && !sscanf (name, "raidz1-%u", &dummy)
+		  && !sscanf (name, "raidz2-%u", &dummy)
+		  && !sscanf (name, "raidz3-%u", &dummy)
 		  && !strcmp (state, "ONLINE"))
 		{
 		  if (ndevices >= devices_allocated)

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