Fwd: r44255 - in trunk/packages/partman/partman-lvm: choose_partition/lvm debian

To: debian-boot@lists.debian.org
Subject: Fwd: r44255 - in trunk/packages/partman/partman-lvm: choose_partition/lvm debian
From: Frans Pop <aragorn@tiscali.nl>
Date: Tue, 16 Jan 2007 21:55:16 +0100
Message-id: <[🔎] 200701162155.17438.aragorn@tiscali.nl>
I'd appreciate a careful review of this patch as it is late in the game
and not all parts of the code are easy to test. I'd hate to see
regressions because of some stupid mistake.

Although the existing code is not necessarily wrong, it does do a few
things in a way that, AFAIK, are not really in line with the intentions
of the debconf protocol, like setting the default for (multi)select
questions to non-existing values and depending on them to stay that way
to detect a <GoBack>.

I've tried to be as consistent as possible in making my changes.
See http://bugs.debian.org/407039 for the background on this patch.

David: are you aware of any other scripts that could have similar issues?

Cheers,
FJP

----------  Forwarded Message  ----------

Subject: r44255 - in trunk/packages/partman/partman-lvm: choose_partition/lvm debian
Date: Tuesday 16 January 2007 20:55
From: Frans Pop <fjp@alioth.debian.org>
To: debian-installer_cvs@packages.qa.debian.org

Author: fjp
Date: Tue Jan 16 20:55:16 2007
New Revision: 44255

Modified:
   trunk/packages/partman/partman-lvm/choose_partition/lvm/do_option
   trunk/packages/partman/partman-lvm/debian/changelog
Log:
* Properly check for <Go Back> during selection of LV or VG for deletion.
  Thanks to Joel Johnson for spotting this issue. Closes: #407039.
* General review of debconf handling in main LVM script:
  - Don't set notes or error templates to "false"; it's meaningless.
  - Don't set (multi)select templates to non-existent "false" value as
    that won't work as was probably expected.
  - More consistent handling of db_go.


Modified:
trunk/packages/partman/partman-lvm/choose_partition/lvm/do_option
==============================================================================
--- trunk/packages/partman/partman-lvm/choose_partition/lvm/do_option	(original)
+++ trunk/packages/partman/partman-lvm/choose_partition/lvm/do_option	Tue Jan 16 20:55:16 2007
@@ -16,7 +16,6 @@

 	# make sure that lvm is available
 	if ! grep -q "[0-9] device-mapper$" /proc/misc ; then
-		db_set partman-lvm/nolvm "false"
 		db_input critical partman-lvm/nolvm
 		db_go
 		exit 0
@@ -47,9 +46,8 @@
 	for pv in $(pv_list); do
 		if ! pv_create "$pv"; then
 			db_subst partman-lvm/pvcreate_error PV "$pv"
-			db_set partman-lvm/nolvm "false"
-			db_input critical partman-lvm/nolvm
-			db_go
+			db_input critical partman-lvm/pvcreate_error
+			db_go || true
 			exit 0
 		fi
 	done
@@ -61,7 +59,7 @@
 			db_subst partman-lvm/activevg COUNT $count
 			db_set partman-lvm/activevg "false"
 			db_input critical partman-lvm/activevg
-			db_go
+			db_go || true
 			db_get partman-lvm/activevg
 			[ "$RET" = "true" ] && log-output -t partman-lvm vgchange -a y
 			# TODO: We need to update the devices that LVM just claimed
@@ -74,10 +72,9 @@
 do_display() {
 	lvm_get_config
 	db_subst partman-lvm/displayall CURRENT_CONFIG "$RET"
-	db_set partman-lvm/displayall "false"
 	db_input critical partman-lvm/displayall
 	db_capb
-	db_go
+	db_go || true
 	db_capb backup
 	db_get partman-lvm/displayall
 }
@@ -97,63 +94,54 @@
 		fi
 	done
 	if [ -z "$pvs" ]; then
-		db_set partman-lvm/nopartitions "false"
 		db_input critical partman-lvm/nopartitions
-		db_go
+		db_go || true
 		return
 	fi

 	# Prompt for VG name
 	db_set partman-lvm/vgcreate_name ""
 	db_input critical partman-lvm/vgcreate_name
-	db_go
-	[ $? -eq 30 ] && return
+	db_go || return
 	db_get partman-lvm/vgcreate_name
 	vg="$RET"

 	# Check VG name
 	if [ -z "$vg" ]; then
-		db_set partman-lvm/vgcreate_nonamegiven "false"
 		db_input critical partman-lvm/vgcreate_nonamegiven
-		db_go
+		db_go || true
 		return
 	fi

 	if ! vg_name_ok "$vg"; then
-		db_set partman-lvm/badnamegiven "false"
 		db_input critical partman-lvm/badnamegiven
-		db_go
+		db_go || true
 		return
 	fi

 	# Check whether the VG name is already in use
 	if vgs "$vg" > /dev/null 2>&1; then
-		db_set partman-lvm/vgcreate_nameused "false"
 		db_input critical partman-lvm/vgcreate_nameused
-		db_go
+		db_go || true
 		return
 	fi

 	# Check whether the VG name overlaps with an existing device
 	if [ -e "/dev/$vg" ]; then
-		db_set partman-lvm/vgcreate_devnameused "false"
 		db_input critical partman-lvm/vgcreate_devnameused
-		db_go
+		db_go || true
 		return
 	fi

 	# Choose the PVs to use
 	db_subst partman-lvm/vgcreate_parts PARTITIONS "$pvs"
-	db_set partman-lvm/vgcreate_parts "false"
+	db_reset partman-lvm/vgcreate_parts
 	db_input critical partman-lvm/vgcreate_parts
-	db_go
+	db_go || return
 	db_get partman-lvm/vgcreate_parts
-	if [ "$RET" = "false" ]; then
-		return
-	elif [ -z "$RET" ]; then
-		db_set partman-lvm/vgcreate_nosel "false"
+	if [ -z "$RET" ]; then
 		db_input critical partman-lvm/vgcreate_nosel
-		db_go
+		db_go || true
 		return
 	fi
 	pvs=$(echo "$RET" | sed -e 's/ *([^)]*) *//g')
@@ -161,9 +149,8 @@

 	if ! vg_create "$vg" $pvs; then
 		db_subst partman-lvm/vgcreate_error VG "$vg"
-		db_set partman-lvm/vgcreate_error "false"
 		db_input critical partman-lvm/vgcreate_error
-		db_go
+		db_go || true
 	else
 		db_subst partman-lvm/text/in_use VG "$vg"
 		db_metaget partman-lvm/text/in_use description
@@ -189,34 +176,31 @@
 		fi
 	done
 	if [ -z "$vgs" ]; then
-		db_set partman-lvm/vgdelete_novg "false"
 		db_input critical partman-lvm/vgdelete_novg
-		db_go
+		db_go || true
 		return
 	fi

 	# Prompt for VG to delete
 	db_subst partman-lvm/vgdelete_names GROUPS "$vgs"
-	db_set partman-lvm/vgdelete_names "false"
+	db_reset partman-lvm/vgdelete_names
 	db_input critical partman-lvm/vgdelete_names
-	db_go
+	db_go || return
 	db_get partman-lvm/vgdelete_names
-	[ "$RET" = "false" ] && return
 	vg=$(echo "$RET" | sed -e 's/[[:space:]]*(.*//')

 	# Confirm
 	db_subst partman-lvm/vgdelete_confirm VG $vg
 	db_set partman-lvm/vgdelete_confirm "false"
 	db_input critical partman-lvm/vgdelete_confirm
-	db_go
+	db_go || true
 	db_get partman-lvm/vgdelete_confirm
 	[ "$RET" = "true" ] || return

 	pvs=$(vg_list_pvs "$vg")
 	if ! vg_delete "$vg"; then
-		db_set partman-lvm/vgdelete_error "false"
 		db_input critical partman-lvm/vgdelete_error
-		db_go
+		db_go || true
 	else
 		for pv in $pvs; do
 			partman_unlock_unit "$pv"
@@ -240,9 +224,8 @@
 		fi
 	done
 	if [ -z "$pvs" ]; then
-		db_set partman-lvm/nopartitions "false"
 		db_input critical partman-lvm/nopartitions
-		db_go
+		db_go || true
 		return
 	fi

@@ -258,33 +241,28 @@
 		fi
 	done
 	if [ -z "$vgs" ]; then
-		db_set partman-lvm/vgextend_novg "false"
 		db_input critical partman-lvm/vgextend_novg
-		db_go
+		db_go || true
 		return
 	fi

 	# Prompt for VG to extend
 	db_subst partman-lvm/vgextend_names GROUPS "$vgs"
-	db_set partman-lvm/vgextend_names "false"
+	db_reset partman-lvm/vgextend_names
 	db_input critical partman-lvm/vgextend_names
-	db_go
+	db_go || return
 	db_get partman-lvm/vgextend_names
-	[ "$RET" = "false" ] && return
 	vg=$(echo "$RET" | sed -e 's/[[:space:]]*(.*//')

 	# Prompt for PVs to use
 	db_subst partman-lvm/vgextend_parts PARTITIONS "$pvs"
-	db_set partman-lvm/vgextend_parts "false"
+	db_reset partman-lvm/vgextend_parts
 	db_input critical partman-lvm/vgextend_parts
-	db_go
+	db_go || return
 	db_get partman-lvm/vgextend_parts
 	if [ -z "$RET" ]; then
-		db_set partman-lvm/vgextend_nosel "false"
 		db_input critical partman-lvm/vgextend_nosel
-		db_go
-		return
-	elif [ "$RET" = "false" ]; then
+		db_go || true
 		return
 	fi
 	pvs=$(echo "$RET" | sed -e 's/ *([^)]*) *//g')
@@ -294,9 +272,8 @@
 		if ! vg_extend "$vg" "$pv"; then
 			db_subst partman-lvm/vgextend_error PARTITION $pv
 			db_subst partman-lvm/vgextend_error VG $vg
-			db_set partman-lvm/vgextend_error "false"
 			db_input critical partman-lvm/vgextend_error
-			db_go
+			db_go || true
 			return
 		else
 			db_subst partman-lvm/text/in_use VG "$vg"
@@ -323,19 +300,17 @@
 		fi
 	done
 	if [ -z "$vgs" ]; then
-		db_set partman-lvm/vgreduce_novg "false"
 		db_input critical partman-lvm/vgreduce_novg
-		db_go
+		db_go || true
 		return
 	fi

 	# Prompt for VG to reduce
 	db_subst partman-lvm/vgreduce_names GROUPS "$vgs"
-	db_set partman-lvm/vgreduce_names "false"
+	db_reset partman-lvm/vgreduce_names
 	db_input critical partman-lvm/vgreduce_names
-	db_go
+	db_go || return
 	db_get partman-lvm/vgreduce_names
-	[ "$RET" = "false" ] && return
 	vg=$(echo "$RET" | sed -e 's/[[:space:]]*(.*//')

 	# Prompt for PV to remove
@@ -352,16 +327,13 @@

 	# Prompt for PVs to use
 	db_subst partman-lvm/vgreduce_parts PARTITIONS "$pvs"
-	db_set partman-lvm/vgreduce_parts "false"
+	db_reset partman-lvm/vgreduce_parts
 	db_input critical partman-lvm/vgreduce_parts
-	db_go
+	db_go || return
 	db_get partman-lvm/vgreduce_parts
 	if [ -z "$RET" ]; then
-		db_set partman-lvm/vgreduce_nosel "false"
 		db_input critical partman-lvm/vgreduce_nosel
-		db_go
-		return
-	elif [ "$RET" = "false" ]; then
+		db_go || true
 		return
 	fi
 	pvs=$(echo "$RET" | sed -e 's/ *([^)]*) *//g')
@@ -372,9 +344,8 @@
 	vg_get_info "$vg"
 	if [ "$count" -eq "$PVS" ]; then
 		if ! vg_delete "$vg"; then
-			db_set partman-lvm/vgdelete_error "false"
 			db_input critical partman-lvm/vgdelete_error
-			db_go
+			db_go || true
 		fi
 		return
 	fi
@@ -384,9 +355,8 @@
 		if ! vg_reduce "$vg" "$pv"; then
 			db_subst partman-lvm/vgreduce_error VG "$vg"
 			db_subst partman-lvm/vgreduce_error PARTITION "$pv"
-			db_set partman-lvm/vgreduce_error "false"
 			db_input critical partman-lvm/vgreduce_error
-			db_go
+			db_go || true
 			return
 		else
 			partman_unlock_unit "$pv"
@@ -410,51 +380,45 @@
 	done

 	if [ -z "$vgs" ]; then
-		db_set partman-lvm/lvcreate_nofreevg "false"
 		db_input critical partman-lvm/lvcreate_nofreevg
-		db_go
+		db_go || true
 		return
 	fi

 	# Prompt for VG to use
 	db_subst partman-lvm/lvcreate_vgnames GROUPS "$vgs"
-	db_set partman-lvm/lvcreate_vgnames "false"
+	db_reset partman-lvm/lvcreate_vgnames
 	db_input critical partman-lvm/lvcreate_vgnames
-	db_go
+	db_go || return
 	db_get partman-lvm/lvcreate_vgnames
-	[ "$RET" = "false" ] && return
 	vg=$(echo "$RET" | sed -e 's/[[:space:]]*(.*//')

 	# Prompt for name to give the new lv
 	db_set partman-lvm/lvcreate_name ""
 	db_input critical partman-lvm/lvcreate_name
-	db_go
-	[ $? -eq 30 ] && return
+	db_go || return
 	db_get partman-lvm/lvcreate_name
 	lv="$RET"

 	# Check LV name
 	if [ -z "$lv" ]; then
-		db_set partman-lvm/lvcreate_nonamegiven "false"
 		db_input critical partman-lvm/lvcreate_nonamegiven
-		db_go
+		db_go || true
 		return
 	fi

 	if ! lv_name_ok "$lv"; then
-		db_set partman-lvm/badnamegiven "false"
 		db_input critical partman-lvm/badnamegiven
-		db_go
+		db_go || true
 		return
 	fi

 	# Make sure the name isn't already in use
 	if lvs "/dev/$vg/$lv" > /dev/null 2>&1; then
-		db_subst partman-lvm/lvcreate_exists LV "$lv"
+		db_subst partman-lvm/lvcreate_exists LV $lv
 		db_subst partman-lvm/lvcreate_exists VG $vg
-		db_set partman-lvm/lvcreate_exists "false"
 		db_input critical partman-lvm/lvcreate_exists
-		db_go
+		db_go || true
 		return
 	fi

@@ -464,10 +428,9 @@
 	db_set partman-lvm/lvcreate_size "${max_size}B"
 	db_fset partman-lvm/lvcreate_size seen false
 	db_input critical partman-lvm/lvcreate_size
-	db_go
-	[ $? -eq 30 ] && return
+	db_go || return
 	db_get partman-lvm/lvcreate_size
-	[ -z "$RET" ] && return
+	[ -n "$RET" ] || return
 	size=$(lvm_size_from_human "$RET")

 	# If the maximum free space should be used for the new LV, use the
@@ -482,9 +445,8 @@
 		db_subst partman-lvm/lvcreate_error VG $vg
 		db_subst partman-lvm/lvcreate_error LV $lv
 		db_subst partman-lvm/lvcreate_error SIZE $RET
-		db_set partman-lvm/lvcreate_error "false"
 		db_input critical partman-lvm/lvcreate_error
-		db_go
+		db_go || true
 		return
 	fi
 }
@@ -508,18 +470,16 @@
 	done

 	if [ -z "$lvs" ]; then
-		db_set partman-lvm/lvdelete_nolv "false"
 		db_input critical partman-lvm/lvdelete_nolv
-		db_go
+		db_go || true
 		return
 	fi

 	db_subst partman-lvm/lvdelete_lvnames LVS "$lvs"
-	db_set partman-lvm/lvdelete_lvnames "false"
+	db_reset partman-lvm/lvdelete_lvnames
 	db_input critical partman-lvm/lvdelete_lvnames
-	db_go
+	db_go || return
 	db_get partman-lvm/lvdelete_lvnames
-	[ "$RET" = "false" ] && return
 	lv=$(echo "$RET" | cut -d' ' -f1)
 	vg=$(echo "$RET" | sed -e 's/.*(\(.*\))/\1/')
 	vg=${vg##* }
@@ -527,9 +487,8 @@
 	if ! lv_delete "$vg" "$lv"; then
 		db_subst partman-lvm/lvdelete_error VG $vg
 		db_subst partman-lvm/lvdelete_error LV $lv
-		db_set partman-lvm/lvdelete_error "false"
 		db_input critical partman-lvm/lvdelete_error
-		db_go
+		db_go || true
 		return
 	fi
 }
@@ -624,12 +583,9 @@
 	db_subst partman-lvm/mainmenu USED_PVS "$used_pvs"
 	db_subst partman-lvm/mainmenu VGS "$vgs"
 	db_subst partman-lvm/mainmenu LVS "$lvs"
-	db_set partman-lvm/mainmenu "false"
+	db_reset partman-lvm/mainmenu
 	db_input critical partman-lvm/mainmenu
-	db_go
-	if [ $? -eq 30 ]; then
-		break
-	fi
+	db_go || break

 	db_get partman-lvm/mainmenu
 	option="$RET"

Modified: trunk/packages/partman/partman-lvm/debian/changelog
==============================================================================
--- trunk/packages/partman/partman-lvm/debian/changelog	(original)
+++ trunk/packages/partman/partman-lvm/debian/changelog	Tue Jan 16 20:55:16 2007
@@ -1,3 +1,11 @@
+partman-lvm (49) UNRELEASED; urgency=low
+
+  * Properly check for <Go Back> during selection of LV or VG for deletion.
+    Thanks to Joel Johnson for spotting this issue. Closes: #407039.
+  * General review of debconf handling in main LVM script.
+
+ -- Frans Pop <fjp@debian.org>  Tue, 16 Jan 2007 20:48:35 +0100
+
 partman-lvm (48) unstable; urgency=low

   [ David Härdeman ]

-------------------------------------------------------
Attachment: pgpkWsrOUEb8V.pgp
Description: PGP signature
Reply to:
Follow-Ups:
- Re: Fwd: r44255 - in trunk/packages/partman/partman-lvm: choose_partition/lvm debian
  - From: David Härdeman <david@hardeman.nu>
Prev by Date: Bug#407169: marked as done (installation-reports)
Next by Date: Processed: Re: Bug#407039: LVM Volume deleted from graphical installer
Previous by thread: Re: Une envie de chocolat ? Un atelier chocolat offert avec votre premiere commande
Next by thread: Re: Fwd: r44255 - in trunk/packages/partman/partman-lvm: choose_partition/lvm debian
Index(es):
- Date
- Thread