Bug#594221: Broken DMRAID support in grub-probe

[Colin Watson]

On Tue, Aug 24, 2010 at 07:30:01PM +0300, Modestas Vainius wrote:
> current versions of grub-probe in squeeze and sid have problems with DMRAID
> partitions.  The problems are demonstrated below. As a result, grub
> lenny->squeeze upgrade failed leaving my system without a bootloader. While
> grub-common/lenny had no support for DMRAID at all, it would be a pity to
> release current grub-common with half working DMRAID support. Therefore, it
> would be great if the bug was fixed in squeeze. A proposed patch is attached
> to this mail. Please forward the bug and/or the patch upstream.

Thanks for this.  Since I'm an upstream developer ... :-)

> From: Modestas Vainius <modax at debian.org>
> Subject: fix DMRAID support in grub-probe
> Forwarded: no
> Last-Update: 2010-08-24
> 
> /dev/mapper/* are symlinks to /dev/dm-* and
> convert_system_partition_to_system_disk() checks against fully a canonicalized
> real path. Therefore, it should look for dm-* when identifying DMRAID devices.
> Likewise, return the same form (/dev/mapper/* or /dev/dm-$minor) which was
> asked for in the input. This is needed since strcmp() is typically used by the
> callers to check if returned device is the same as given.

I disagree with this approach.  In general, we should be treating
/dev/mapper/* as the canonical name, despite the symlink layout.  For
one, those device names convey much more information than the terse
/dev/dm-*; for another, it's consistent with other GNU packages, such as
GNU Parted:

  commit c1eb485b9fd8919e18f192d678bc52b0488e6ee0
  Author: Hans de Goede <hdegoede at redhat.com>
  Date:   Tue Apr 6 15:57:18 2010 +0200
  
      libparted: don't canonicalize /dev/mapper paths
  
      Besides fixing the issue displayed by libparted/tests/symlink.c,
      this has the added advantage that the output of parted p on one of these
      devices now says:
      "Disk /dev/mapper/BigVol2-lv_iscsi_disk2: 34.4GB"
  
      Which is a lot more user friendly then the output before this patch:
      "Disk /dev/dm-6: 34.4GB"
  
      * libparted/device.c (ped_device_get): Don't canonicalize names
      that start with "/dev/mapper/".
      * NEWS (Bug fixes): Mention it.
      Thanks to Ales Kozumplik for the analysis.
      Details in <http://bugzilla.redhat.com/577824>.

I've committed a patch upstream which implements this:

2010-09-17  Colin Watson  <cjwatson at ubuntu.com>

	Fix DM-RAID probing with recent versions of device-mapper udev
	rules.

	* grub-core/kern/emu/hostdisk.c (read_device_map): Don't
	canonicalise device paths under /dev/mapper/.
	(convert_system_partition_to_system_disk): Compare the
	uncanonicalised path to /dev/mapper/ rather than the canonicalised
	path, since device nodes under /dev/mapper/ are often symlinks.

> While at it, the patch also fixes a memory pool leak in dm_* related code.
> devmapper library complains loudly about memory pool leaks.
[...]
> -	  static struct dm_tree *tree = NULL;
> +	  struct dm_tree *tree = NULL;

It's a shame to have to repeatedly reconstruct the dm_tree, but I
suppose we have little choice if libdevmapper is going to complain
loudly at us.

I've committed a patch upstream, based on the general idea of your patch
but I rearranged things so that all the freeing happens on a single exit
path from the function:

2010-09-17  Colin Watson  <cjwatson at ubuntu.com>

	* grub-core/kern/emu/hostdisk.c
	(convert_system_partition_to_system_disk): Fix devmapper memory pool
	leak.
	Reported and based on patch by: Modestas Vainius.

I'll backport these two patches in my next upload.

Thanks,

-- 
Colin Watson                                       [cjwatson at ubuntu.com]