Merge lp:~psusi/ubuntu/natty/parted/fix-dmraid into lp:ubuntu/natty/parted

[Phillip Susi]

  * Fix probe_partition_for_geom() to fail gracefully rather
    than abort with PED_ASSERT(). (LP: #545911)
  * Add dm_p_separator.patch: Device mapper type should not
    automatically mean add 'p' before the partition number.
    Fall back to adding it only if the previous character is
    a digit. This compilies with kpartx behavior.
  * Refresh dmraid.patch to apply with dm_p_separator.patch

[Colin Watson (cjwatson)]

On Sat, Mar 05, 2011 at 05:44:10AM -0000, Phillip Susi wrote:
> + a digit. This compilies with kpartx behavior.

Typo: "complies".

> === added file 'debian/patches/dm_p_separator.patch'
> --- debian/patches/dm_p_separator.patch 1970-01-01 00:00:00 +0000
> +++ debian/patches/dm_p_separator.patch 2011-03-05 05:43:54 +0000
> @@ -0,0 +1,30 @@
> +Device mapper type should not automatically mean add 'p'
> +before the partition number. Fall back to adding it only
> +if the previous character is a digit. This compilies
> +with kpartx behavior.

The comments I made on your dmraid branch about DEP-3 patch headers
apply here too.

> +@@ -2735,7 +2734,11 @@
> +
> + dev_name = dm_task_get_name (task);
> +
> +- if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
> ++ if (isdigit(dev_name[strlen(dev_name)-1]))
> ++ {

Please use consistent formatting within files/functions. The local
style is to put a space before open parentheses and around arithmetic
operators, and to cuddle open braces onto the same line as ifs, so:

  if (isdigit (dev_name[strlen (dev_name) - 1])) {

> === modified file 'debian/patches/dmraid.patch'
> --- debian/patches/dmraid.patch 2011-02-02 10:11:46 +0000
> +++ debian/patches/dmraid.patch 2011-03-05 05:43:54 +0000
> @@ -5,10 +5,10 @@
> nodes created needlessly, and make sure that partition nodes for DM-RAID
> devices are not probed.
>
> -Index: b/libparted/arch/linux.c
> +Index: parted/libparted/arch/linux.c
> ===================================================================
> ---- a/libparted/arch/linux.c
> -+++ b/libparted/arch/linux.c
> +--- parted.orig/libparted/arch/linux.c 2011-03-04 20:27:35.611473999 -0500
> ++++ parted/libparted/arch/linux.c 2011-03-04 20:28:46.431473991 -0500

Could you use 'quilt refresh -pab' here to avoid these deltas, since
that's how the previous patches were constructed?

> === modified file 'debian/patches/series'
> --- debian/patches/series 2010-12-24 17:00:39 +0000
> +++ debian/patches/series 2011-03-05 05:43:54 +0000
> @@ -26,3 +26,4 @@
>
> # Symbols for this ABI (amd64 as reference)
> update-abi-symbols.patch
> +dm_p_separator.patch

Please make sure that new patches go above the terminating
update-abi-symbols.patch. You should just be able to edit
debian/patches/series and move this line up to the end of the "Ubuntu
additions" section.

> === modified file 'libparted/labels/dos.c'
> --- libparted/labels/dos.c 2010-08-05 21:06:19 +0000
> +++ libparted/labels/dos.c 2011-03-05 05:43:54 +0000

Please put this in a quilt patch in the "Ubuntu additions" section
rather than applying it directly.

I don't see any substantive problems with this branch, so, like the
dmraid branch, it can be merged once you've fixed these cosmetic
problems. Thanks!

 review needs-fixing

review: Needs Fixing

[lp:~psusi/ubuntu/natty/parted/fix-dmraid]

89. By Phillip Susi on 2011-03-08

Add fix-head-size-assertion.patch:
Fix probe_partition_for_geom() to fail gracefully rather
than abort with PED_ASSERT(). (LP: #545911)

[Phillip Susi (psusi)]

How about dem apples?

review: Needs Resubmitting

[lp:~psusi/ubuntu/natty/parted/fix-dmraid]

90. By Phillip Susi on 2011-03-09

* Add dm_p_separator.patch: Device mapper type should not
  automatically mean add 'p' before the partition number.
  Fall back to adding it only if the previous character is
  a digit. This complies with kpartx behavior and linux
  behavior "since the dawn of time".
* Refresh dasd-sync.patch, dmraid.patch, tiny-disk-constraint.patch,
  udevadm-settle.patch.
* debian/control: Breaks versions of dmraid not
  patched to follow the same 'p' behavior.

[Colin Watson (cjwatson)]

Thanks, this looks much better now.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2011-02-02 10:21:15 +0000
+++ debian/changelog	2011-03-09 00:47:14 +0000
@@ -1,3 +1,20 @@
+parted (2.3-5ubuntu4) maverick; urgency=low
+
+  * Add fix-head-size-assertion.patch:
+    Fix probe_partition_for_geom() to fail gracefully rather
+    than abort with PED_ASSERT().  (LP: #545911)
+  * Add dm_p_separator.patch: Device mapper type should not
+    automatically mean add 'p' before the partition number.
+    Fall back to adding it only if the previous character is
+    a digit.  This complies with kpartx behavior and linux
+    behavior "since the dawn of time".
+  * Refresh dasd-sync.patch, dmraid.patch, tiny-disk-constraint.patch,
+    udevadm-settle.patch.
+  * debian/control: Breaks versions of dmraid not
+    patched to follow the same 'p' behavior.
+
+ -- Phillip Susi <[e-mail]>  Sat, 12 Feb 2011 16:37:05 -0500
+
 parted (2.3-5ubuntu3) natty; urgency=low
 
   * Update dmraid.patch to add the 'p' before the partition

=== modified file 'debian/control'
--- debian/control	2010-10-22 11:24:41 +0000
+++ debian/control	2011-03-09 00:47:14 +0000
@@ -79,6 +79,7 @@
 Depends: ${shlibs:Depends}, ${misc:Depends}
 Suggests: parted | nparted, libparted0-dev, libparted0-i18n (= ${source:Version})
 Conflicts: parted (<< 1.4.13+14pre1), libparted1 (<< 2.2), libparted2 (<< 2.2)
+Breaks: dmraid (<< 1.0.0.rc16-4.1ubuntu2)
 Replaces: libparted0 (<< 2.2-4), libparted1 (<< 2.2), libparted2 (<< 2.2), libparted1.4 (<< 1.4.24-2)
 Provides: libparted
 Description: The GNU Parted disk partitioning shared library

=== modified file 'debian/patches/dasd-sync.patch'
--- debian/patches/dasd-sync.patch	2010-12-19 12:57:43 +0000
+++ debian/patches/dasd-sync.patch	2011-03-09 00:47:14 +0000
@@ -18,7 +18,7 @@
  #ifdef ENABLE_DEVICE_MAPPER
  #include <libdevmapper.h>
  #endif
-@@ -2807,20 +2808,15 @@
+@@ -2809,20 +2810,15 @@
                  return _dm_reread_part_table (disk);
  #endif
          if (disk->dev->type != PED_DEVICE_FILE) {

=== added file 'debian/patches/dm_p_separator.patch'
--- debian/patches/dm_p_separator.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/dm_p_separator.patch	2011-03-09 00:47:14 +0000
@@ -0,0 +1,33 @@
+From: Phillip Susi <[e-mail]>
+Description: Device mapper type should not automatically
+ mean add 'p' before the partition number.  Fall back to
+ adding it only if the previous character is a digit.
+ This complies with kpartx behavior and linux behavior
+ "since the dawn of time".
+Forwarded: yes
+Last-Update: 2011-03-08
+
+Index: b/libparted/arch/linux.c
+===================================================================
+--- a/libparted/arch/linux.c	2011-03-08 10:43:36.726632247 -0500
++++ b/libparted/arch/linux.c	2011-03-08 10:46:30.274635151 -0500
+@@ -2198,7 +2198,6 @@
+         } else if (dev->type == PED_DEVICE_DAC960
+                         || dev->type == PED_DEVICE_CPQARRAY
+                         || dev->type == PED_DEVICE_ATARAID
+-                        || dev->type == PED_DEVICE_DM
+                         || isdigit (dev->path[path_len - 1]))
+                 snprintf (result, result_len, "%sp%d", dev->path, num);
+         else
+@@ -2716,7 +2715,10 @@
+ 
+         dev_name = dm_task_get_name (task);
+ 
+-        if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
++        if (isdigit (dev_name[strlen (dev_name) - 1])) {
++                if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
++                        goto err;
++        } else if (asprintf (&vol_name, "%s%d", dev_name, part->num) == -1)
+                 goto err;
+ 
+         /* Caution: dm_task_destroy frees dev_name.  */

=== modified file 'debian/patches/dmraid.patch'
--- debian/patches/dmraid.patch	2011-02-02 10:11:46 +0000
+++ debian/patches/dmraid.patch	2011-03-09 00:47:14 +0000
@@ -39,18 +39,7 @@
         }
         closedir (mapper_dir);
  
-@@ -2200,6 +2212,10 @@
-         } else if (!strncmp (dev->path, "/dev/loop", 9)) {
-                 /* Loop devices can only have one partition. */
-                 strcpy (result, dev->path);
-+#ifdef ENABLE_DEVICE_MAPPER
-+        } else if (dev->type == PED_DEVICE_DM && _is_dmraid_device (dev->path)) {
-+                snprintf (result, result_len, "%sp%d", dev->path, num);
-+#endif
-         } else if (dev->type == PED_DEVICE_DAC960
-                         || dev->type == PED_DEVICE_CPQARRAY
-                         || dev->type == PED_DEVICE_ATARAID
-@@ -2702,6 +2718,8 @@
+@@ -2701,6 +2717,8 @@
          char*           vol_name = NULL;
          const char*     dev_name = NULL;
          char*           params = NULL;
@@ -59,7 +48,7 @@
          LinuxSpecific*  arch_specific = LINUX_SPECIFIC (disk->dev);
  
          if (!_has_partitions(disk))
-@@ -2739,12 +2757,19 @@
+@@ -2741,12 +2759,19 @@
          dm_task_set_name (task, vol_name);
          dm_task_add_target (task, 0, part->geom.length,
                  "linear", params);
@@ -79,7 +68,7 @@
                  return 1;
          } else {
                  _dm_remove_map_name(vol_name);
-@@ -2785,6 +2810,85 @@
+@@ -2787,6 +2812,85 @@
          }
          return rc;
  }

=== added file 'debian/patches/fix-head-size-assertion.patch'
--- debian/patches/fix-head-size-assertion.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/fix-head-size-assertion.patch	2011-03-09 00:47:14 +0000
@@ -0,0 +1,74 @@
+From: Phillip Susi <[e-mail]>
+Subject: Fix assertion failure: head_size <= 63
+Description: probe_partition_for_geom() was originally written
+ using PED_ASSERT() to test for failures in computing the disk
+ geometry based on existing partition table entries.  That
+ macro has been modified to ignore the second argument ( which
+ was a return statement ), and instead abort the program.  This
+ patch rewrites those tests to not use PED_ASSERT() so the
+ function correctly returns when the computations fail, and the
+ program can recover gracefully and operate normally.
+Last-Update: 2011-03-08
+Bug-Ubuntu: http://launchpad.net/bugs/545911
+
+Index: b/libparted/labels/dos.c
+===================================================================
+--- a/libparted/labels/dos.c	2011-03-08 10:25:21.558631186 -0500
++++ b/libparted/labels/dos.c	2011-03-08 10:25:33.770633724 -0500
+@@ -646,8 +645,10 @@
+ 	if (cyl_size * denum != a_*H - A_*h)
+ 		return 0;
+ 
+-	PED_ASSERT (cyl_size > 0, return 0);
+- 	PED_ASSERT (cyl_size <= 255 * 63, return 0);
++	if (cyl_size <= 0)
++		return 0;
++	if (cyl_size > 255 * 63)
++		return 0;
+ 
+ 	if (h > 0)
+ 		head_size = ( a_ - c * cyl_size ) / h;
+@@ -658,18 +659,24 @@
+ 		PED_ASSERT (0, return 0);
+ 	}
+ 
+-	PED_ASSERT (head_size > 0, return 0);
+-	PED_ASSERT (head_size <= 63, return 0);
++	if (head_size <= 0)
++		return 0;
++	if (head_size > 63)
++		return 0;
+ 
+ 	cylinders = part->disk->dev->length / cyl_size;
+ 	heads = cyl_size / head_size;
+ 	sectors = head_size;
+ 
+-	PED_ASSERT (heads > 0, return 0);
+-	PED_ASSERT (heads < 256, return 0);
++	if (heads <= 0)
++		return 0;
++	if (heads > 255)
++		return 0;
+ 
+-	PED_ASSERT (sectors > 0, return 0);
+-	PED_ASSERT (sectors <= 63, return 0);
++	if (sectors <= 0)
++		return 0;
++	if (sectors > 63)
++		return 0;
+ 
+ 	/* Some broken OEM partitioning program(s) seem to have an out-by-one
+ 	 * error on the end of partitions.  We should offer to fix the
+@@ -678,8 +685,10 @@
+ 	if (((C + 1) * heads + H) * sectors + S == A)
+ 		C++;
+ 
+-	PED_ASSERT ((c * heads + h) * sectors + s == a, return 0);
+-	PED_ASSERT ((C * heads + H) * sectors + S == A, return 0);
++	if ((c * heads + h) * sectors + s != a)
++		return 0;
++	if ((C * heads + H) * sectors + S != A)
++		return 0;
+ 
+ 	bios_geom->cylinders = cylinders;
+ 	bios_geom->heads = heads;

=== modified file 'debian/patches/series'
--- debian/patches/series	2010-12-24 17:00:39 +0000
+++ debian/patches/series	2011-03-09 00:47:14 +0000
@@ -11,6 +11,8 @@
 freebsd-ufs.patch
 zfs.patch
 zero-length-devices.patch
+fix-head-size-assertion.patch
+dm_p_separator.patch
 
 # Backported
 sun-revert-disk-flag.patch

=== modified file 'debian/patches/tiny-disk-constraint.patch'
--- debian/patches/tiny-disk-constraint.patch	2010-11-19 21:42:56 +0000
+++ debian/patches/tiny-disk-constraint.patch	2011-03-09 00:47:14 +0000
@@ -9,7 +9,7 @@
 ===================================================================
 --- a/libparted/labels/dos.c
 +++ b/libparted/labels/dos.c
-@@ -1646,13 +1646,13 @@
+@@ -1656,13 +1656,13 @@
  			       		dev->length - min_geom->end))
  			return NULL;
  	} else {

=== modified file 'debian/patches/udevadm-settle.patch'
--- debian/patches/udevadm-settle.patch	2010-12-24 20:44:27 +0000
+++ debian/patches/udevadm-settle.patch	2011-03-09 00:47:14 +0000
@@ -19,7 +19,7 @@
  #include <ctype.h>
  #include <errno.h>
  #include <fcntl.h>
-@@ -2799,16 +2800,60 @@
+@@ -2801,16 +2802,60 @@
          return have_blkpg = kver >= KERNEL_VERSION (2,4,0) ? 1 : 0;
  }
  
@@ -82,7 +82,7 @@
  #endif
          if (disk->dev->type != PED_DEVICE_FILE) {
  
-@@ -2819,10 +2864,20 @@
+@@ -2821,10 +2866,20 @@
  		assert (_have_blkpg ());
  
  		if (!_disk_sync_part_table (disk))