zfs pool devices detection in core/osdep/unix/getroot.c

zfs pool devices detection in core/osdep/unix/getroot.c

[Toomas Soome]

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

hi!

this update is written by Richard Yao <ryao@gentoo.org> and is addressing the issue that device names are not correctly discovered in more complex pool setups. This code walks over pool vdev list and will collect device names from vdev children, ensuring the all device names will be presented to caller.

Since Richard has been busy, Im posting this update on his behalf and based on fact that the code is been quite extensively tested by me and few other people anyhow:) 

rgds,
toomas



[-- Attachment #2: getroot.diff --]
[-- Type: application/octet-stream, Size: 3253 bytes --]

diff --git a/grub-core/osdep/unix/getroot.c b/grub-core/osdep/unix/getroot.c
index b98b2df..329938a 100644
--- a/grub-core/osdep/unix/getroot.c
+++ b/grub-core/osdep/unix/getroot.c
@@ -159,43 +159,17 @@ xgetcwd (void)
   return path;
 }
 
-char **
-grub_util_find_root_devices_from_poolname (char *poolname)
-{
-  char **devices = 0;
-  size_t ndevices = 0;
-  size_t devices_allocated = 0;
-
 #if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
-  zpool_handle_t *zpool;
-  libzfs_handle_t *libzfs;
-  nvlist_t *config, *vdev_tree;
+static void
+grub_util_find_child_vdevs(nvlist_t *vdev_tree, char ***devices, size_t *ndevices, size_t *devices_allocated)
+{
   nvlist_t **children;
   unsigned int nvlist_count;
   unsigned int i;
   char *device = 0;
 
-  libzfs = grub_get_libzfs_handle ();
-  if (! libzfs)
-    return NULL;
-
-  zpool = zpool_open (libzfs, poolname);
-  config = zpool_get_config (zpool, NULL);
-
-  if (nvlist_lookup_nvlist (config, "vdev_tree", &vdev_tree) != 0)
-    error (1, errno, "nvlist_lookup_nvlist (\"vdev_tree\")");
-
-  if (nvlist_lookup_nvlist_array (vdev_tree, "children", &children, &nvlist_count) != 0)
-    error (1, errno, "nvlist_lookup_nvlist_array (\"children\")");
-  assert (nvlist_count > 0);
-
-  while (nvlist_lookup_nvlist_array (children[0], "children",
-				     &children, &nvlist_count) == 0)
-    assert (nvlist_count > 0);
-
-  for (i = 0; i < nvlist_count; i++)
-    {
-      if (nvlist_lookup_string (children[i], "path", &device) != 0)
+  if (nvlist_lookup_nvlist_array (vdev_tree, "children", &children, &nvlist_count) != 0){
+      if (nvlist_lookup_string (vdev_tree, "path", &device) != 0)
 	error (1, errno, "nvlist_lookup_string (\"path\")");
 
       struct stat st;
@@ -214,17 +188,50 @@ grub_util_find_root_devices_from_poolname (char *poolname)
 	  else
 #endif
 	    device = xstrdup (device);
-	  if (ndevices >= devices_allocated)
+	  if (*ndevices >= *devices_allocated)
 	    {
-	      devices_allocated = 2 * (devices_allocated + 8);
-	      devices = xrealloc (devices, sizeof (devices[0])
-				  * devices_allocated);
+	      *devices_allocated = 2 * (*devices_allocated + 8);
+	      *devices = xrealloc (*devices, sizeof ((*devices)[0])
+				  * *devices_allocated);
 	    }
-	  devices[ndevices++] = device;
+	  (*devices)[(*ndevices)++] = device;
 	}
 
       device = NULL;
-    }
+
+  } else {
+    for (i = 0; i < nvlist_count; i++)
+      {
+        grub_util_find_child_vdevs(children[i], devices, ndevices, devices_allocated);
+      }
+  }
+
+}
+#endif
+
+char **
+grub_util_find_root_devices_from_poolname (char *poolname)
+{
+  char **devices = 0;
+  size_t ndevices = 0;
+  size_t devices_allocated = 0;
+
+#if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
+  zpool_handle_t *zpool;
+  libzfs_handle_t *libzfs;
+  nvlist_t *config, *vdev_tree;
+
+  libzfs = grub_get_libzfs_handle ();
+  if (! libzfs)
+    return NULL;
+
+  zpool = zpool_open (libzfs, poolname);
+  config = zpool_get_config (zpool, NULL);
+
+  if (nvlist_lookup_nvlist (config, "vdev_tree", &vdev_tree) != 0)
+    error (1, errno, "nvlist_lookup_nvlist (\"vdev_tree\")");
+
+  grub_util_find_child_vdevs(vdev_tree, &devices, &ndevices, &devices_allocated);
 
   zpool_close (zpool);
 #else

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

Re: zfs pool devices detection in core/osdep/unix/getroot.c

[Andrei Borzenkov]

В Mon, 13 Apr 2015 21:00:20 +0300
Toomas Soome <tsoome@me.com> пишет:

> hi!
> 
> this update is written by Richard Yao <ryao@gentoo.org> and is addressing the issue that device names are not correctly discovered in more complex pool setups. This code walks over pool vdev list and will collect device names from vdev children, ensuring the all device names will be presented to caller.
> 
> Since Richard has been busy, Im posting this update on his behalf and based on fact that the code is been quite extensively tested by me and few other people anyhow:) 
> 
> rgds,
> toomas
> 
> 

Could you please send git format-patch (or git send-email) ready to
apply? As far as I know policy, patches on behalf of others are rather
frowned upon.

Anyway minor nitpicks

> diff --git a/grub-core/osdep/unix/getroot.c b/grub-core/osdep/unix/getroot.c
> index b98b2df..329938a 100644
> --- a/grub-core/osdep/unix/getroot.c
> +++ b/grub-core/osdep/unix/getroot.c
> @@ -159,43 +159,17 @@ xgetcwd (void)
>    return path;
>  }
>  
> -char **
> -grub_util_find_root_devices_from_poolname (char *poolname)
> -{
> -  char **devices = 0;
> -  size_t ndevices = 0;
> -  size_t devices_allocated = 0;
> -
>  #if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
> -  zpool_handle_t *zpool;
> -  libzfs_handle_t *libzfs;
> -  nvlist_t *config, *vdev_tree;
> +static void
> +grub_util_find_child_vdevs(nvlist_t *vdev_tree, char ***devices, size_t *ndevices, size_t *devices_allocated)
> +{
>    nvlist_t **children;
>    unsigned int nvlist_count;
>    unsigned int i;
>    char *device = 0;
>  
> -  libzfs = grub_get_libzfs_handle ();
> -  if (! libzfs)
> -    return NULL;
> -
> -  zpool = zpool_open (libzfs, poolname);
> -  config = zpool_get_config (zpool, NULL);
> -
> -  if (nvlist_lookup_nvlist (config, "vdev_tree", &vdev_tree) != 0)
> -    error (1, errno, "nvlist_lookup_nvlist (\"vdev_tree\")");
> -
> -  if (nvlist_lookup_nvlist_array (vdev_tree, "children", &children, &nvlist_count) != 0)
> -    error (1, errno, "nvlist_lookup_nvlist_array (\"children\")");
> -  assert (nvlist_count > 0);
> -
> -  while (nvlist_lookup_nvlist_array (children[0], "children",
> -				     &children, &nvlist_count) == 0)
> -    assert (nvlist_count > 0);
> -
> -  for (i = 0; i < nvlist_count; i++)
> -    {
> -      if (nvlist_lookup_string (children[i], "path", &device) != 0)
> +  if (nvlist_lookup_nvlist_array (vdev_tree, "children", &children, &nvlist_count) != 0){

curly braces style

> +      if (nvlist_lookup_string (vdev_tree, "path", &device) != 0)
>  	error (1, errno, "nvlist_lookup_string (\"path\")");
>  
>        struct stat st;
> @@ -214,17 +188,50 @@ grub_util_find_root_devices_from_poolname (char *poolname)
>  	  else
>  #endif
>  	    device = xstrdup (device);
> -	  if (ndevices >= devices_allocated)
> +	  if (*ndevices >= *devices_allocated)
>  	    {
> -	      devices_allocated = 2 * (devices_allocated + 8);
> -	      devices = xrealloc (devices, sizeof (devices[0])
> -				  * devices_allocated);
> +	      *devices_allocated = 2 * (*devices_allocated + 8);
> +	      *devices = xrealloc (*devices, sizeof ((*devices)[0])
> +				  * *devices_allocated);
>  	    }
> -	  devices[ndevices++] = device;
> +	  (*devices)[(*ndevices)++] = device;
>  	}
>  
>        device = NULL;
> -    }
> +
> +  } else {

curly braces style

> +    for (i = 0; i < nvlist_count; i++)
> +      {
> +        grub_util_find_child_vdevs(children[i], devices, ndevices, devices_allocated);
> +      }
> +  }

all braces in this branch are reundant.

Re: zfs pool devices detection in core/osdep/unix/getroot.c

[Toomas Soome]

> 
> Could you please send git format-patch (or git send-email) ready to
> apply? As far as I know policy, patches on behalf of others are rather
> frowned upon.
> 

will do. however, I just realised, this one needs to be delayed till actual zfs update is passed and accepted, which probably will take some time due to size… 



>> -  for (i = 0; i < nvlist_count; i++)
>> -    {
>> -      if (nvlist_lookup_string (children[i], "path", &device) != 0)
>> +  if (nvlist_lookup_nvlist_array (vdev_tree, "children", &children, &nvlist_count) != 0){
> 
> curly braces style
> 

updated.
>> 
>> +  } else {
> 
> curly braces style
> 

updated.

>> +    for (i = 0; i < nvlist_count; i++)
>> +      {
>> +        grub_util_find_child_vdevs(children[i], devices, ndevices, devices_allocated);
>> +      }
>> +  }
> 
> all braces in this branch are reundant.

removed inner ones, however, I think outer ones, even as strictly not needed, might still help readability.

rgds,
toomas