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 <[EMAIL PROTECTED]> To: [EMAIL PROTECTED] 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 <[EMAIL PROTECTED]> Tue, 16 Jan 2007 20:48:35 +0100 + partman-lvm (48) unstable; urgency=low [ David Härdeman ] -------------------------------------------------------
pgpfmVV2BRvsU.pgp
Description: PGP signature
