Bug#738922: parted3 preparation
On Thu, Feb 13, 2014 at 04:53:16PM -0500, Phillip Susi wrote:
> In preparation for the parted3 transition, this patch fixes format_swap
> to go straight to using mkswap instead of trying to use parted_server
> first. It also removes ext2 support, which will be picked up by
> partman-ext3 and depends on dosfstools-udeb for fsck/mkfs. Finally, it
> removes check_swap, since there really is no such thing as fscking swap.
I think it would be very helpful to split up the logical chunks of this.
Moving ext2 support means that we need to make sure that
partman-basicfilesystems and partman-ext3 land in unstable and testing
at the same time, and derived distributions need to do that too. Given
the general lack of tool support for this sort of lockstep change in
udebs, I'm uncomfortable with bundling it into this change. Was there a
good reason for that or did you just think it was tidier? If the
latter, I think it would in fact be better avoided.
In any case, the presence of this change bundled in with everything else
makes the rest of it much harder to read.
> diff -Nru partman-basicfilesystems-90/check.d/check_basicfilesystems partman-basicfilesystems-91/check.d/check_basicfilesystems
> --- partman-basicfilesystems-90/check.d/check_basicfilesystems 2011-05-03 21:00:32.000000000 -0400
> +++ partman-basicfilesystems-91/check.d/check_basicfilesystems 2014-02-13 16:30:28.000000000 -0500
> @@ -30,9 +30,14 @@
> db_subst $template PARTITION "$num"
> db_subst $template DEVICE $(humandev $(cat device))
> name_progress_bar $template
> - open_dialog CHECK_FILE_SYSTEM $id
> - read_line status
> - close_dialog
> + if log-output -t partman --pass-stdout \
> + dosfsck $device >/dev/null; then
> + status=OK
> + else
> + status=failed
> + fi
> + db_progress STOP
> +
> if [ "$status" != good ]; then
> db_subst partman-basicfilesystems/check_failed TYPE "$filesystem"
> db_subst partman-basicfilesystems/check_failed PARTITION "$num"
The name_progress_bar call now has no effect and should be removed.
Contrariwise, the effect of this change is that no progress bar is
displayed while dosfsck is running. This is the sort of thing we need
to be careful about throughout this work. Given that we don't have
useful steps, I would suggest replacing the name_progress_bar call with
something like:
db_progress START 0 1 partman/text/please_wait
db_progress INFO $template
[log-output dosfsck, etc.]
db_progress STOP
I also don't think you can possibly have tested this, because you set
status=OK on success, but then the check below is [ "$status" != good ],
so the effect of this change will be that partman-basicfilesystems
always displays an error dialog. Unlike CREATE_FILE_SYSTEM,
parted_server CHECK_FILE_SYSTEM sets status to good or bad; I suggest
following that.
> diff -Nru partman-basicfilesystems-90/check.d/check_swap partman-basicfilesystems-91/check.d/check_swap
> --- partman-basicfilesystems-90/check.d/check_swap 2011-01-18 23:57:21.000000000 -0500
> +++ partman-basicfilesystems-91/check.d/check_swap 1969-12-31 19:00:00.000000000 -0500
> @@ -1,58 +0,0 @@
> -#!/bin/sh
> -
> -. /lib/partman/lib/base.sh
> -
> -swap=false
> -
> -for dev in $DEVICES/*; do
> - [ -d "$dev" ] || continue
> - cd $dev
> - partitions=
> - open_dialog PARTITIONS
> - while { read_line num id size type fs path name; [ "$id" ]; }; do
> - [ "$fs" != free ] || continue
> - partitions="$partitions $id,$num"
> - done
> - close_dialog
> -
> - for part in $partitions; do
> - id=${part%,*}
> - num=${part#*,}
> - [ -f $id/method ] || continue
> - method=$(cat $id/method)
> - if [ "$method" = swap ]; then
> - swap=:
> - fi
> - [ ! -f $id/format ] || continue
> - if [ "$method" = swap ]; then
> - log "Check the swap space in $dev/$id"
> - template=partman-basicfilesystems/progress_swap_checking
> - db_subst $template PARTITION "$num"
> - db_subst $template DEVICE $(humandev $(cat device))
> - name_progress_bar $template
> - open_dialog CHECK_FILE_SYSTEM $id
> - read_line status
> - close_dialog
> - if [ "$status" != good ]; then
> - db_subst partman-basicfilesystems/swap_check_failed PARTITION "$num"
> - db_subst partman-basicfilesystems/swap_check_failed DEVICE $(humandev $(cat device))
> - db_set partman-basicfilesystems/swap_check_failed true
> - db_input critical partman-basicfilesystems/swap_check_failed || true
> - db_go || true
> - db_get partman-basicfilesystems/swap_check_failed
> - if [ "$RET" = true ]; then
> - exit 1
> - fi
> - fi
> - fi
> - done
> -done
> -
> -if ! $swap; then
> - db_input critical partman-basicfilesystems/no_swap || true
> - db_go || true
> - db_get partman-basicfilesystems/no_swap
> - if [ "$RET" = true ]; then
> - exit 1
> - fi
> -fi
While it may be reasonable to remove the CHECK_FILE_SYSTEM bits, it
isn't reasonable to remove the check that you have a swap partition at
all. Please put this file back and instead just delete the parts from
'[ ! -f $id/format ] || continue' to the end of the enclosing for loop.
> diff -Nru partman-basicfilesystems-90/commit.d/format_basicfilesystems partman-basicfilesystems-91/commit.d/format_basicfilesystems
> --- partman-basicfilesystems-90/commit.d/format_basicfilesystems 2011-01-18 23:57:21.000000000 -0500
> +++ partman-basicfilesystems-91/commit.d/format_basicfilesystems 2014-02-13 14:59:35.000000000 -0500
[...]
> fat16|fat32)
> name_progress_bar $template
> - open_dialog CREATE_FILE_SYSTEM $id $filesystem
> - read_line status
> - close_dialog
> - sync
> + if log-output -t partman --pass-stdout \
> + mkfs.$filesystem $device >/dev/null; then
> + status=OK
> + else
> + status=failed
> + fi
> + db_progress STOP
> ;;
> esac
>
Again, you need to actually create a progress bar here.
name_progress_bar doesn't do so - all it does is set things up so that
the exception handler knows what to do on the next timer event, which
now won't arrive at all here - and we shouldn't have unbalanced
db_progress START/STOP calls. You could use commit.d/format_swap for
comparison here, since it's done reasonably enough there.
I think it would be a good idea to keep the sync call in place, at least
in the event that mkfs succeeds.
> -Template: partman-basicfilesystems/text/noatime
> -Type: text
> -# :sl2:
> -# Note to translators: Please keep your translations of this string below
> -# a 65 columns limit (which means 65 characters in single-byte languages)
> -_Description: noatime - do not update inode access times at each access
Even aside from my comments about moving ext2 support, you must not
remove all these mount option templates. select_mountoptions always
picks up the templates from partman-basicfilesystems/text/$op, and these
mount options are made available for various different file systems.
Really, I don't think you should need to make any template changes in
partman-basicfilesystems as part of preparing for parted 3. But this in
particular would have broken quite a lot of things.
Could you fix these problems and post a corrected version? It should be
much shorter.
Thanks,
--
Colin Watson [cjwatson@debian.org]
Reply to: