Ubuntu
parted package

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

  1. Natty (11.04)
  2. fix-dmraid
  3. Merge into natty
Proposed by Phillip Susi
Status: Merged
Merged at revision: 89
Proposed branch: lp:~psusi/ubuntu/natty/parted/fix-dmraid
Merge into: lp:ubuntu/natty/parted
Diff against target: 256 lines (+134/-18)
9 files modified
debian/changelog (+17/-0)
debian/control (+1/-0)
debian/patches/dasd-sync.patch (+1/-1)
debian/patches/dm_p_separator.patch (+33/-0)
debian/patches/dmraid.patch (+3/-14)
debian/patches/fix-head-size-assertion.patch (+74/-0)
debian/patches/series (+2/-0)
debian/patches/tiny-disk-constraint.patch (+1/-1)
debian/patches/udevadm-settle.patch (+2/-2)
To merge this branch: bzr merge lp:~psusi/ubuntu/natty/parted/fix-dmraid
Reviewer Review Type Date Requested Status
Colin Watson Approve
Phillip Susi (community) Needs Resubmitting
Review via email: mp+52285@code.launchpad.net

Description of the change

  * 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

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

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 updated
89. By Phillip Susi

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

Revision history for this message
Phillip Susi (psusi) wrote :

How about dem apples?

review: Needs Resubmitting
lp:~psusi/ubuntu/natty/parted/fix-dmraid updated
90. By Phillip Susi

* 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.

Revision history for this message
Colin Watson (cjwatson) wrote :

Thanks, this looks much better now.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1parted (2.3-5ubuntu4) maverick; urgency=low
2
3 * Add fix-head-size-assertion.patch:
4 Fix probe_partition_for_geom() to fail gracefully rather
5 than abort with PED_ASSERT(). (LP: #545911)
6 * Add dm_p_separator.patch: Device mapper type should not
7 automatically mean add 'p' before the partition number.
8 Fall back to adding it only if the previous character is
9 a digit. This complies with kpartx behavior and linux
10 behavior "since the dawn of time".
11 * Refresh dasd-sync.patch, dmraid.patch, tiny-disk-constraint.patch,
12 udevadm-settle.patch.
13 * debian/control: Breaks versions of dmraid not
14 patched to follow the same 'p' behavior.
15
16 -- Phillip Susi <psusi@cfl.rr.com> Sat, 12 Feb 2011 16:37:05 -0500
17
1parted (2.3-5ubuntu3) natty; urgency=low18parted (2.3-5ubuntu3) natty; urgency=low
219
3 * Update dmraid.patch to add the 'p' before the partition20 * Update dmraid.patch to add the 'p' before the partition
421
=== 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 @@
79Depends: ${shlibs:Depends}, ${misc:Depends}79Depends: ${shlibs:Depends}, ${misc:Depends}
80Suggests: parted | nparted, libparted0-dev, libparted0-i18n (= ${source:Version})80Suggests: parted | nparted, libparted0-dev, libparted0-i18n (= ${source:Version})
81Conflicts: parted (<< 1.4.13+14pre1), libparted1 (<< 2.2), libparted2 (<< 2.2)81Conflicts: parted (<< 1.4.13+14pre1), libparted1 (<< 2.2), libparted2 (<< 2.2)
82Breaks: dmraid (<< 1.0.0.rc16-4.1ubuntu2)
82Replaces: libparted0 (<< 2.2-4), libparted1 (<< 2.2), libparted2 (<< 2.2), libparted1.4 (<< 1.4.24-2)83Replaces: libparted0 (<< 2.2-4), libparted1 (<< 2.2), libparted2 (<< 2.2), libparted1.4 (<< 1.4.24-2)
83Provides: libparted84Provides: libparted
84Description: The GNU Parted disk partitioning shared library85Description: The GNU Parted disk partitioning shared library
8586
=== 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 @@
18 #ifdef ENABLE_DEVICE_MAPPER18 #ifdef ENABLE_DEVICE_MAPPER
19 #include <libdevmapper.h>19 #include <libdevmapper.h>
20 #endif20 #endif
21@@ -2807,20 +2808,15 @@21@@ -2809,20 +2810,15 @@
22 return _dm_reread_part_table (disk);22 return _dm_reread_part_table (disk);
23 #endif23 #endif
24 if (disk->dev->type != PED_DEVICE_FILE) {24 if (disk->dev->type != PED_DEVICE_FILE) {
2525
=== 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 @@
1From: Phillip Susi <psusi@cfl.rr.com>
2Description: Device mapper type should not automatically
3 mean add 'p' before the partition number. Fall back to
4 adding it only if the previous character is a digit.
5 This complies with kpartx behavior and linux behavior
6 "since the dawn of time".
7Forwarded: yes
8Last-Update: 2011-03-08
9
10Index: b/libparted/arch/linux.c
11===================================================================
12--- a/libparted/arch/linux.c 2011-03-08 10:43:36.726632247 -0500
13+++ b/libparted/arch/linux.c 2011-03-08 10:46:30.274635151 -0500
14@@ -2198,7 +2198,6 @@
15 } else if (dev->type == PED_DEVICE_DAC960
16 || dev->type == PED_DEVICE_CPQARRAY
17 || dev->type == PED_DEVICE_ATARAID
18- || dev->type == PED_DEVICE_DM
19 || isdigit (dev->path[path_len - 1]))
20 snprintf (result, result_len, "%sp%d", dev->path, num);
21 else
22@@ -2716,7 +2715,10 @@
23
24 dev_name = dm_task_get_name (task);
25
26- if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
27+ if (isdigit (dev_name[strlen (dev_name) - 1])) {
28+ if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
29+ goto err;
30+ } else if (asprintf (&vol_name, "%s%d", dev_name, part->num) == -1)
31 goto err;
32
33 /* Caution: dm_task_destroy frees dev_name. */
034
=== 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 @@
39 }39 }
40 closedir (mapper_dir);40 closedir (mapper_dir);
41 41
42@@ -2200,6 +2212,10 @@42@@ -2701,6 +2717,8 @@
43 } else if (!strncmp (dev->path, "/dev/loop", 9)) {
44 /* Loop devices can only have one partition. */
45 strcpy (result, dev->path);
46+#ifdef ENABLE_DEVICE_MAPPER
47+ } else if (dev->type == PED_DEVICE_DM && _is_dmraid_device (dev->path)) {
48+ snprintf (result, result_len, "%sp%d", dev->path, num);
49+#endif
50 } else if (dev->type == PED_DEVICE_DAC960
51 || dev->type == PED_DEVICE_CPQARRAY
52 || dev->type == PED_DEVICE_ATARAID
53@@ -2702,6 +2718,8 @@
54 char* vol_name = NULL;43 char* vol_name = NULL;
55 const char* dev_name = NULL;44 const char* dev_name = NULL;
56 char* params = NULL;45 char* params = NULL;
@@ -59,7 +48,7 @@
59 LinuxSpecific* arch_specific = LINUX_SPECIFIC (disk->dev);48 LinuxSpecific* arch_specific = LINUX_SPECIFIC (disk->dev);
60 49
61 if (!_has_partitions(disk))50 if (!_has_partitions(disk))
62@@ -2739,12 +2757,19 @@51@@ -2741,12 +2759,19 @@
63 dm_task_set_name (task, vol_name);52 dm_task_set_name (task, vol_name);
64 dm_task_add_target (task, 0, part->geom.length,53 dm_task_add_target (task, 0, part->geom.length,
65 "linear", params);54 "linear", params);
@@ -79,7 +68,7 @@
79 return 1;68 return 1;
80 } else {69 } else {
81 _dm_remove_map_name(vol_name);70 _dm_remove_map_name(vol_name);
82@@ -2785,6 +2810,85 @@71@@ -2787,6 +2812,85 @@
83 }72 }
84 return rc;73 return rc;
85 }74 }
8675
=== 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 @@
1From: Phillip Susi <psusi@cfl.rr.com>
2Subject: Fix assertion failure: head_size <= 63
3Description: probe_partition_for_geom() was originally written
4 using PED_ASSERT() to test for failures in computing the disk
5 geometry based on existing partition table entries. That
6 macro has been modified to ignore the second argument ( which
7 was a return statement ), and instead abort the program. This
8 patch rewrites those tests to not use PED_ASSERT() so the
9 function correctly returns when the computations fail, and the
10 program can recover gracefully and operate normally.
11Last-Update: 2011-03-08
12Bug-Ubuntu: http://launchpad.net/bugs/545911
13
14Index: b/libparted/labels/dos.c
15===================================================================
16--- a/libparted/labels/dos.c 2011-03-08 10:25:21.558631186 -0500
17+++ b/libparted/labels/dos.c 2011-03-08 10:25:33.770633724 -0500
18@@ -646,8 +645,10 @@
19 if (cyl_size * denum != a_*H - A_*h)
20 return 0;
21
22- PED_ASSERT (cyl_size > 0, return 0);
23- PED_ASSERT (cyl_size <= 255 * 63, return 0);
24+ if (cyl_size <= 0)
25+ return 0;
26+ if (cyl_size > 255 * 63)
27+ return 0;
28
29 if (h > 0)
30 head_size = ( a_ - c * cyl_size ) / h;
31@@ -658,18 +659,24 @@
32 PED_ASSERT (0, return 0);
33 }
34
35- PED_ASSERT (head_size > 0, return 0);
36- PED_ASSERT (head_size <= 63, return 0);
37+ if (head_size <= 0)
38+ return 0;
39+ if (head_size > 63)
40+ return 0;
41
42 cylinders = part->disk->dev->length / cyl_size;
43 heads = cyl_size / head_size;
44 sectors = head_size;
45
46- PED_ASSERT (heads > 0, return 0);
47- PED_ASSERT (heads < 256, return 0);
48+ if (heads <= 0)
49+ return 0;
50+ if (heads > 255)
51+ return 0;
52
53- PED_ASSERT (sectors > 0, return 0);
54- PED_ASSERT (sectors <= 63, return 0);
55+ if (sectors <= 0)
56+ return 0;
57+ if (sectors > 63)
58+ return 0;
59
60 /* Some broken OEM partitioning program(s) seem to have an out-by-one
61 * error on the end of partitions. We should offer to fix the
62@@ -678,8 +685,10 @@
63 if (((C + 1) * heads + H) * sectors + S == A)
64 C++;
65
66- PED_ASSERT ((c * heads + h) * sectors + s == a, return 0);
67- PED_ASSERT ((C * heads + H) * sectors + S == A, return 0);
68+ if ((c * heads + h) * sectors + s != a)
69+ return 0;
70+ if ((C * heads + H) * sectors + S != A)
71+ return 0;
72
73 bios_geom->cylinders = cylinders;
74 bios_geom->heads = heads;
075
=== 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 @@
11freebsd-ufs.patch11freebsd-ufs.patch
12zfs.patch12zfs.patch
13zero-length-devices.patch13zero-length-devices.patch
14fix-head-size-assertion.patch
15dm_p_separator.patch
1416
15# Backported17# Backported
16sun-revert-disk-flag.patch18sun-revert-disk-flag.patch
1719
=== 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 @@
9===================================================================9===================================================================
10--- a/libparted/labels/dos.c10--- a/libparted/labels/dos.c
11+++ b/libparted/labels/dos.c11+++ b/libparted/labels/dos.c
12@@ -1646,13 +1646,13 @@12@@ -1656,13 +1656,13 @@
13 dev->length - min_geom->end))13 dev->length - min_geom->end))
14 return NULL;14 return NULL;
15 } else {15 } else {
1616
=== 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 @@
19 #include <ctype.h>19 #include <ctype.h>
20 #include <errno.h>20 #include <errno.h>
21 #include <fcntl.h>21 #include <fcntl.h>
22@@ -2799,16 +2800,60 @@22@@ -2801,16 +2802,60 @@
23 return have_blkpg = kver >= KERNEL_VERSION (2,4,0) ? 1 : 0;23 return have_blkpg = kver >= KERNEL_VERSION (2,4,0) ? 1 : 0;
24 }24 }
25 25
@@ -82,7 +82,7 @@
82 #endif82 #endif
83 if (disk->dev->type != PED_DEVICE_FILE) {83 if (disk->dev->type != PED_DEVICE_FILE) {
84 84
85@@ -2819,10 +2864,20 @@85@@ -2821,10 +2866,20 @@
86 assert (_have_blkpg ());86 assert (_have_blkpg ());
87 87
88 if (!_disk_sync_part_table (disk))88 if (!_disk_sync_part_table (disk))

Subscribers

People subscribed via source and target branches

to all changes: