[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: partman extended partition bug



On Wednesday 16 January 2008, Grégory Oestreicher wrote:
> Le mercredi 16 janvier 2008 08:29, Frans Pop a écrit :
> > I see roughly what you mean, but have some trouble translating it to a
> > specific situation. With which recipe does this happen (or: how exactly
> > can I reproduce this bug)?
>
> Actually there's no recipe that should be impacted by this bug. I noticed
> it because I am modifying partman-auto-lvm to support multiple disks
> during automated installations, and in the recipe I wrote to test my code
> there's a way to declare a PV on a physical device. My code failed after
> the SVN update, and I finallly traced it back to this cause (the code is
> using $devfspv_devices to track LVM PVs, and for disks other than the
> preseeded "first" one it failed).
>
> It's not strictly speaking a bug in partman-auto (that's why I put it in
> quotes), but as I had to patch to continue my tests I thought it may be
> useful for the future.

OK. I thought something like that was the case as the partman-auto-lvm code 
currently always creates the "envelope" partition as a logical partition.
See auto_lvm_prepare() in partman-auto-lvm/lib/auto-lvm.sh:
    scheme="$normalscheme${NL}100 1000 1000000000 ext3 method{ $method }"

However, I still like the idea behind your patch because it is a more 
general and cleaner implementation. And I have wondered in the past why we 
use a logical partition at all when we create only a root and lvm 
partition. So earlier today I came up with the following (tested) patches 
that include your cleanup (with minor modifications) and creates a primary 
lvm partition when possible.
As you can see, an additional change was needed in partman-auto-lvm to make 
using a primary partition really work (initialization of devfspv_devices 
happened after use in create_primary_partitions).

Thanks again,
FJP

P.S. I'll reply soon to your proposed patch for using multiple disks.

Index: partman-auto-lvm/debian/control
===================================================================
--- partman-auto-lvm/debian/control	(revision 50896)
+++ partman-auto-lvm/debian/control	(working copy)
@@ -9,5 +9,5 @@
 Package: partman-auto-lvm
 XC-Package-Type: udeb
 Architecture: all
-Depends: ${misc:Depends}, partman-base (>= 114), partman-auto (>= 73), partman-lvm (>= 58), di-utils (>= 1.15), lvm2-udeb
+Depends: ${misc:Depends}, partman-base (>= 114), partman-auto (>= 74), partman-lvm (>= 58), di-utils (>= 1.15)
 Description: Automatically partition storage devices using LVM
Index: partman-auto-lvm/debian/changelog
===================================================================
--- partman-auto-lvm/debian/changelog	(revision 50896)
+++ partman-auto-lvm/debian/changelog	(working copy)
@@ -1,3 +1,11 @@
+partman-auto-lvm (26) UNRELEASED; urgency=low
+
+  * Create envelope partition on primary partition if possible.
+    Requires partman-auto (>= 74).
+  * Remove dependency on lvm2-udeb as partman-lvm already depends on it.
+
+ -- Frans Pop <fjp@debian.org>  Wed, 16 Jan 2008 09:44:44 +0100
+
 partman-auto-lvm (25) unstable; urgency=low
 
   [ Frans Pop ]
Index: partman-auto-lvm/lib/auto-lvm.sh
===================================================================
--- partman-auto-lvm/lib/auto-lvm.sh	(revision 50896)
+++ partman-auto-lvm/lib/auto-lvm.sh	(working copy)
@@ -89,20 +89,20 @@
 	esac
 
 	# Creating envelope
-	scheme="$normalscheme${NL}100 1000 1000000000 ext3 method{ $method }"
+	scheme="$normalscheme${NL}100 1000 1000000000 ext3 \$primary{ } method{ $method }"
 
 	expand_scheme
 
 	clean_method
 
-	create_primary_partitions
-
 	# This variable will be used to store the partitions that will be LVM
 	# by create_partitions; zero it to be sure it's not cluttered.
 	# It will be used later to provide real paths to partitions to LVM.
 	# (still one atm)
 	devfspv_devices=''
 
+	create_primary_partitions
+
 	create_partitions
 
 	if ! confirm_changes partman-lvm; then
Index: partman-auto/debian/changelog
===================================================================
--- partman-auto/debian/changelog	(revision 50896)
+++ partman-auto/debian/changelog	(working copy)
@@ -3,8 +3,10 @@
   * Don't create the last partition as a logical partition when all partitions
     are defined as primary and there are sufficient primary partitions
     available. Closes: #413505.
+  * Make it possible to use a primary partition for LVM. Thanks to Grégory
+    Oestreicher for spotting the issue and providing an initial patch.
 
- -- Frans Pop <fjp@debian.org>  Tue, 15 Jan 2008 10:47:03 +0100
+ -- Frans Pop <fjp@debian.org>  Wed, 16 Jan 2008 09:39:12 +0100
 
 partman-auto (73) unstable; urgency=low
 
Index: partman-auto/lib/auto-shared.sh
===================================================================
--- partman-auto/lib/auto-shared.sh	(revision 50896)
+++ partman-auto/lib/auto-shared.sh	(working copy)
@@ -22,6 +22,23 @@
 	close_dialog
 }
 
+# Mark a partition as LVM and add it to vgpath
+mark_partition_as_lvm() {
+	local id
+	id=$1
+	shift
+
+	devfspv_devices="$devfspv_devices $path"
+	open_dialog GET_FLAGS $id
+	flags=$(read_paragraph)
+	close_dialog
+	open_dialog SET_FLAGS $id
+	write_line "$flags"
+	write_line lvm
+	write_line NO_MORE
+	close_dialog
+}
+
 create_primary_partitions() {
 	cd $dev
 
@@ -72,6 +89,9 @@
 			fi
 		fi
 		shift; shift; shift; shift
+		if echo "$*" | grep -q "method{ lvm }"; then
+			mark_partition_as_lvm $id $*
+		fi
 		setup_partition $id $*
 		primary=''
 		scheme="$scheme_rest"
@@ -117,20 +137,10 @@
 		db_progress STOP
 		autopartitioning_failed
 	fi
-
-	# Mark the partition LVM only if it is actually LVM and add it to vgpath
+	shift; shift; shift; shift
 	if echo "$*" | grep -q "method{ lvm }"; then
-		devfspv_devices="$devfspv_devices $path"
-		open_dialog GET_FLAGS $id
-		flags=$(read_paragraph)
-		close_dialog
-		open_dialog SET_FLAGS $id
-		write_line "$flags"
-		write_line lvm
-		write_line NO_MORE
-		close_dialog
+		mark_partition_as_lvm $id $*
 	fi
-	shift; shift; shift; shift
 	setup_partition $id $*
 	free_space=$(partition_after $id)'
 }

Attachment: signature.asc
Description: This is a digitally signed message part.


Reply to: