Re: [PATCH] Fix verbose error output when device-mapper not supported by kernel

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

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

On 06/03/2010 06:23 PM, Colin Watson wrote:
> Following the merge of my dmraid-probe branch, several people have
> reported extremely verbose error output in the event that the kernel
> doesn't have device-mapper support (perhaps because the module isn't
> loaded; it's modular in the stock Debian kernel).  See e.g.
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=584196.  The attached
> patch fixes this.  OK for trunk?
>
>   
Go ahead.
> (The long patch to util/deviceiter.c is almost entirely due to the extra
> 'if' causing an indentation change.)
>   
For such cases please send a patch with --diff-options=-bpB
> 2010-06-03  Colin Watson  <cjwatson@ubuntu.com>
>
> 	* kern/emu/misc.c (device_mapper_null_log): New function.
> 	(grub_device_mapper_supported): New function.
> 	* include/grub/emu/misc.h (grub_device_mapper_supported): Add
> 	prototype.
> 	* kern/emu/hostdisk.c (find_partition_start): Check whether
> 	device-mapper is supported before trying to use it.
> 	* util/deviceiter.c (grub_util_iterate_devices): Likewise.
>
> === modified file 'include/grub/emu/misc.h'
> --- include/grub/emu/misc.h	2010-05-28 13:48:45 +0000
> +++ include/grub/emu/misc.h	2010-06-03 16:00:06 +0000
> @@ -48,4 +48,8 @@ int EXPORT_FUNC(asprintf) (char **buf, c
>  char * EXPORT_FUNC(xasprintf) (const char *fmt, ...);
>  extern char * canonicalize_file_name (const char *path);
>  
> +#ifdef HAVE_DEVICE_MAPPER
> +int grub_device_mapper_supported (void);
> +#endif
> +
>  #endif /* GRUB_EMU_MISC_H */
>
> === modified file 'kern/emu/hostdisk.c'
> --- kern/emu/hostdisk.c	2010-06-02 22:47:22 +0000
> +++ kern/emu/hostdisk.c	2010-06-03 16:00:06 +0000
> @@ -342,7 +342,7 @@ find_partition_start (const char *dev)
>  # endif /* !defined(__NetBSD__) */
>  
>  # ifdef HAVE_DEVICE_MAPPER
> -  if (device_is_mapped (dev)) {
> +  if (grub_device_mapper_supported () && device_is_mapped (dev)) {
>      struct dm_task *task = NULL;
>      grub_uint64_t start, length;
>      char *target_type, *params, *space;
>
> === modified file 'kern/emu/misc.c'
> --- kern/emu/misc.c	2010-05-27 14:45:41 +0000
> +++ kern/emu/misc.c	2010-06-03 16:00:06 +0000
> @@ -22,6 +22,10 @@
>  #include <grub/time.h>
>  #include <grub/emu/misc.h>
>  
> +#ifdef HAVE_DEVICE_MAPPER
> +# include <libdevmapper.h>
> +#endif
> +
>  int verbosity;
>  
>  void
> @@ -311,3 +315,38 @@ grub_make_system_path_relative_to_its_ro
>  
>    return buf3;
>  }
> +
> +#ifdef HAVE_DEVICE_MAPPER
> +static void device_mapper_null_log (int level __attribute__ ((unused)),
> +				    const char *file __attribute__ ((unused)),
> +				    int line __attribute__ ((unused)),
> +				    int dm_errno __attribute__ ((unused)),
> +				    const char *f __attribute__ ((unused)),
> +				    ...)
> +{
> +}
> +
> +int
> +grub_device_mapper_supported (void)
> +{
> +  static int supported = -1;
> +
> +  if (supported == -1)
> +    {
> +      struct dm_task *dmt;
> +
> +      /* Suppress annoying log messages.  */
> +      dm_log_with_errno_init (&device_mapper_null_log);
> +
> +      dmt = dm_task_create (DM_DEVICE_VERSION);
> +      supported = (dmt != NULL);
> +      if (dmt)
> +	dm_task_destroy (dmt);
> +
> +      /* Restore the original logger.  */
> +      dm_log_with_errno_init (NULL);
> +    }
> +
> +  return supported;
> +}
> +#endif /* HAVE_DEVICE_MAPPER */
>
> === modified file 'util/deviceiter.c'
> --- util/deviceiter.c	2010-01-26 14:26:16 +0000
> +++ util/deviceiter.c	2010-06-03 16:00:06 +0000
> @@ -33,6 +33,7 @@
>  #include <grub/util/deviceiter.h>
>  #include <grub/list.h>
>  #include <grub/misc.h>
> +#include <grub/emu/misc.h>
>  
>  #ifdef __linux__
>  # if !defined(__GLIBC__) || \
> @@ -676,112 +677,113 @@ grub_util_iterate_devices (int NESTED_FU
>      }
>  
>    /* DM-RAID.  */
> -  {
> -    struct dm_tree *tree = NULL;
> -    struct dm_task *task = NULL;
> -    struct dm_names *names = NULL;
> -    unsigned int next = 0;
> -    void *top_handle, *second_handle;
> -    struct dm_tree_node *root, *top, *second;
> -    struct dmraid_seen *seen = NULL;
> -
> -    /* Build DM tree for all devices.  */
> -    tree = dm_tree_create ();
> -    dmraid_check (tree, "dm_tree_create failed\n");
> -    task = dm_task_create (DM_DEVICE_LIST);
> -    dmraid_check (task, "dm_task_create failed\n");
> -    dmraid_check (dm_task_run (task), "dm_task_run failed\n");
> -    names = dm_task_get_names (task);
> -    dmraid_check (names, "dm_task_get_names failed\n");
> -    dmraid_check (names->dev, "No DM devices found\n");
> -    do
> -      {
> -	names = (void *) names + next;
> -	dmraid_check (dm_tree_add_dev (tree, MAJOR (names->dev),
> -				       MINOR (names->dev)),
> -			 "dm_tree_add_dev (%s) failed\n", names->name);
> -	next = names->next;
> -      }
> -    while (next);
> -
> -    /* Walk the second-level children of the inverted tree; that is, devices
> -       which are directly composed of non-DM devices such as hard disks.
> -       This class includes all DM-RAID disks and excludes all DM-RAID
> -       partitions.  */
> -    root = dm_tree_find_node (tree, 0, 0);
> -    top_handle = NULL;
> -    top = dm_tree_next_child (&top_handle, root, 1);
> -    while (top)
> -      {
> -	second_handle = NULL;
> -	second = dm_tree_next_child (&second_handle, top, 1);
> -	while (second)
> -	  {
> -	    const char *node_name, *node_uuid;
> -	    char *name;
> -	    struct dmraid_seen *seen_elt;
> -
> -	    node_name = dm_tree_node_get_name (second);
> -	    dmraid_check (node_name, "dm_tree_node_get_name failed\n");
> -	    node_uuid = dm_tree_node_get_uuid (second);
> -	    dmraid_check (node_uuid, "dm_tree_node_get_uuid failed\n");
> -	    if (strncmp (node_uuid, "DMRAID-", 7) != 0)
> -	      {
> -		grub_dprintf ("deviceiter", "%s is not DM-RAID\n", node_name);
> -		goto dmraid_next_child;
> -	      }
> -
> -	    /* Have we already seen this node?  There are typically very few
> -	       DM-RAID disks, so a list should be fast enough.  */
> -	    if (grub_named_list_find (GRUB_AS_NAMED_LIST (seen), node_name))
> -	      {
> -		grub_dprintf ("deviceiter", "Already seen DM device %s\n",
> -			      node_name);
> -		goto dmraid_next_child;
> -	      }
> -
> -	    name = xasprintf ("/dev/mapper/%s", node_name);
> -	    if (check_device (name))
> -	      {
> -		if (hook (name, 0))
> -		  {
> -		    free (name);
> -		    while (seen)
> -		      {
> -			struct dmraid_seen *seen_elt =
> -			  grub_list_pop (GRUB_AS_LIST_P (&seen));
> -			free (seen_elt);
> -		      }
> -		    if (task)
> -		      dm_task_destroy (task);
> -		    if (tree)
> -		      dm_tree_free (tree);
> -		    return;
> -		  }
> -	      }
> -	    free (name);
> +  if (grub_device_mapper_supported ())
> +    {
> +      struct dm_tree *tree = NULL;
> +      struct dm_task *task = NULL;
> +      struct dm_names *names = NULL;
> +      unsigned int next = 0;
> +      void *top_handle, *second_handle;
> +      struct dm_tree_node *root, *top, *second;
> +      struct dmraid_seen *seen = NULL;
> +
> +      /* Build DM tree for all devices.  */
> +      tree = dm_tree_create ();
> +      dmraid_check (tree, "dm_tree_create failed\n");
> +      task = dm_task_create (DM_DEVICE_LIST);
> +      dmraid_check (task, "dm_task_create failed\n");
> +      dmraid_check (dm_task_run (task), "dm_task_run failed\n");
> +      names = dm_task_get_names (task);
> +      dmraid_check (names, "dm_task_get_names failed\n");
> +      dmraid_check (names->dev, "No DM devices found\n");
> +      do
> +	{
> +	  names = (void *) names + next;
> +	  dmraid_check (dm_tree_add_dev (tree, MAJOR (names->dev),
> +					 MINOR (names->dev)),
> +			   "dm_tree_add_dev (%s) failed\n", names->name);
> +	  next = names->next;
> +	}
> +      while (next);
>  
> -	    seen_elt = xmalloc (sizeof *seen_elt);
> -	    seen_elt->name = node_name;
> -	    grub_list_push (GRUB_AS_LIST_P (&seen), GRUB_AS_LIST (seen_elt));
> +      /* Walk the second-level children of the inverted tree; that is, devices
> +	 which are directly composed of non-DM devices such as hard disks.
> +	 This class includes all DM-RAID disks and excludes all DM-RAID
> +	 partitions.  */
> +      root = dm_tree_find_node (tree, 0, 0);
> +      top_handle = NULL;
> +      top = dm_tree_next_child (&top_handle, root, 1);
> +      while (top)
> +	{
> +	  second_handle = NULL;
> +	  second = dm_tree_next_child (&second_handle, top, 1);
> +	  while (second)
> +	    {
> +	      const char *node_name, *node_uuid;
> +	      char *name;
> +	      struct dmraid_seen *seen_elt;
> +
> +	      node_name = dm_tree_node_get_name (second);
> +	      dmraid_check (node_name, "dm_tree_node_get_name failed\n");
> +	      node_uuid = dm_tree_node_get_uuid (second);
> +	      dmraid_check (node_uuid, "dm_tree_node_get_uuid failed\n");
> +	      if (strncmp (node_uuid, "DMRAID-", 7) != 0)
> +		{
> +		  grub_dprintf ("deviceiter", "%s is not DM-RAID\n", node_name);
> +		  goto dmraid_next_child;
> +		}
> +
> +	      /* Have we already seen this node?  There are typically very few
> +		 DM-RAID disks, so a list should be fast enough.  */
> +	      if (grub_named_list_find (GRUB_AS_NAMED_LIST (seen), node_name))
> +		{
> +		  grub_dprintf ("deviceiter", "Already seen DM device %s\n",
> +				node_name);
> +		  goto dmraid_next_child;
> +		}
> +
> +	      name = xasprintf ("/dev/mapper/%s", node_name);
> +	      if (check_device (name))
> +		{
> +		  if (hook (name, 0))
> +		    {
> +		      free (name);
> +		      while (seen)
> +			{
> +			  struct dmraid_seen *seen_elt =
> +			    grub_list_pop (GRUB_AS_LIST_P (&seen));
> +			  free (seen_elt);
> +			}
> +		      if (task)
> +			dm_task_destroy (task);
> +		      if (tree)
> +			dm_tree_free (tree);
> +		      return;
> +		    }
> +		}
> +	      free (name);
> +
> +	      seen_elt = xmalloc (sizeof *seen_elt);
> +	      seen_elt->name = node_name;
> +	      grub_list_push (GRUB_AS_LIST_P (&seen), GRUB_AS_LIST (seen_elt));
>  
>  dmraid_next_child:
> -	    second = dm_tree_next_child (&second_handle, top, 1);
> -	  }
> -	top = dm_tree_next_child (&top_handle, root, 1);
> -      }
> +	      second = dm_tree_next_child (&second_handle, top, 1);
> +	    }
> +	  top = dm_tree_next_child (&top_handle, root, 1);
> +	}
>  
>  dmraid_end:
> -    while (seen)
> -      {
> -	struct dmraid_seen *seen_elt = grub_list_pop (GRUB_AS_LIST_P (&seen));
> -	free (seen_elt);
> -      }
> -    if (task)
> -      dm_task_destroy (task);
> -    if (tree)
> -      dm_tree_free (tree);
> -  }
> +      while (seen)
> +	{
> +	  struct dmraid_seen *seen_elt = grub_list_pop (GRUB_AS_LIST_P (&seen));
> +	  free (seen_elt);
> +	}
> +      if (task)
> +	dm_task_destroy (task);
> +      if (tree)
> +	dm_tree_free (tree);
> +    }
>  # endif /* HAVE_DEVICE_MAPPER */
>  #endif /* __linux__ */
>  }
>
> Thanks,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]