Code review comment for lp:~psusi/ubuntu/natty/parted/fix-dmraid

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