X-Original-To: alpine-devel@lists.alpinelinux.org Delivered-To: alpine-devel@mail.alpinelinux.org Received: from ncopa-desktop.nor.wtbts.net (3.203.202.84.customer.cdi.no [84.202.203.3]) (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: nc@alpinelinux.org) by mail.alpinelinux.org (Postfix) with ESMTPSA id 1C2B5DC0FAA; Mon, 5 Mar 2012 13:13:00 +0000 (UTC) Date: Mon, 5 Mar 2012 14:12:54 +0100 From: Natanael Copa To: Jim Pryor Cc: alpine-devel@lists.alpinelinux.org Subject: Re: [alpine-devel] Understanding alpine-setup Message-ID: <20120305141254.484661d1@ncopa-desktop.nor.wtbts.net> In-Reply-To: <1330897651.26242.140661044650293@webmail.messagingengine.com> References: <1330852060.31716.140661044540177@webmail.messagingengine.com> <1330880212.26962.140661044634665@webmail.messagingengine.com> <1330897651.26242.140661044650293@webmail.messagingengine.com> X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.6; x86_64-unknown-linux-gnu) X-Mailinglist: alpine-devel Precedence: list List-Id: Alpine Development List-Unsubscribe: List-Post: List-Help: List-Subscribe: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Sun, 04 Mar 2012 16:47:31 -0500 Jim Pryor wrote: > > On Sun, Mar 4, 2012, at 11:56 AM, lists+alpine-devel@jimpryor.net > wrote: > > Reading setup-disk now... > > Ok, I have a variety of questions here. I don't really know a way to > do this efficiently. I'm going to include portions of > the /sbin/setup-disk file with numbered lines, and intersperse my > questions / observations. > > 6 MBR=${MBR:-"/usr/share/syslinux/mbr.bin"} > 7 ROOTFS=${ROOTFS:-ext4} > 8 BOOTFS=${BOOTFS:-ext4} > > Mightn't it also be useful to have a VARFS, instead of always just > using ext4? You are of course right about that. Fixed. Thanks! > > > 20 all_in_list() { > 21 local needle="$1" > 22 local i > 23 [ -z "$needle" ] && return 1 > > I see that the way this function is used later in the script requires > an empty $needle to fail; however, since this goes counter to what one > expects from the name of the function (if $needle is empty, all > elements of $needle are in the list), perhaps it's worth a comment > here so that it never gets accidentally broken in the future. or change the function name to "non_empty_all_in_list"? > > 54 # generate an fstab from a given mountpoint. Convert to > UUID if possible > 55 enumerate_fstab() { > 56 local mnt="$1" > 57 local fs_spec= fs_file= fs_vfstype= fs_mntops= > fs_freq= fs_passno= > 58 [ -z "$mnt" ] && return > 59 local escaped_mnt=$(echo $mnt | sed 's:/:\\/:g') > 60 awk "\$2 ~ /^$escaped_mnt/ {print > \$0}" /proc/mounts | \ > > Notice that $escaped_mnt is only anchored at the beginning, not at the > end. Mightn't this break if one of my mountpoints is a prefix of > another? I think you are right. I wonder if there are any practical cases where this might happen. Maybe if you mount your dir as /d/ (which would include /dev/) Yup. tested here. It ends up creating various /ev/* entries in fstab. Good catch! How about this fix: --- a/setup-disk.in +++ b/setup-disk.in @@ -57,8 +57,8 @@ enumerate_fstab() { local mnt="$1" local fs_spec= fs_file= fs_vfstype= fs_mntops= fs_freq= fs_passno= [ -z "$mnt" ] && return - local escaped_mnt=$(echo $mnt | sed 's:/:\\/:g') - awk "\$2 ~ /^$escaped_mnt/ {print \$0}" /proc/mounts | \ + local escaped_mnt=$(echo $mnt | sed -e 's:/*$::' -e 's:/:\\/:g') + awk "\$2 ~ /^$escaped_mnt(\/|\$)/ {print \$0}" /proc/mounts | \ sed "s:$mnt:/:g; s: :\t:g" | sed 's:/\+:/:g' | \ while read fs_spec fs_file fs_vfstype fs_mntops fs_freq fs_passno; do echo -e "$(uuid_or_device $fs_spec)\t${fs_file}\t${fs_vfstype}\t${fs_mntops} ${fs_freq} ${fs_passno}" > > > 76 # return true (0) if given device is lvm > 77 is_lvm() { > 78 lvs "$1" >/dev/null 2>&1 > 79 } > > Perhaps the comment could be updated to say specifically that the $1 > is expected to be a lvs, not a vg. This had me scratching my head for > a while, since supplying a bare vg name as $1 will also work here; > however the code that uses this function will always be supplying > putative lvs, not vgs. Do you have a better test that will return true on lvs but false on vgs? If not I suppose a comment will do. > 163 install_mounted_root() { > ... > 183 > 184 bootdev=$(find_mount_dev "$mnt"/boot) > 185 if [ -z "$bootdev" ]; then > 186 bootdev=$rootdev > 187 else > 188 mnt_boot="$mnt"/boot > 189 bootdev=$(find_mount_dev "$mnt_boot") > > Isn't this line redundant? bootdev is already what we want it to be on > this codepath, no? Indeed. Also, it is slightly confusing that we depend on the earlier initialized mnt_boot. Lets move it to same if .. else .. clause to make it clearer whats going on. > > > 194 if [ -e "/sys/block/${bootdev#/dev/}/md" ]; then > 195 raidopt="--raid" > 196 fi > 197 > 198 for dev in $rootdev $pvs; do > 199 [ -e "/sys/block/${dev#/dev/}/md" ] || > continue > 200 > 201 local md=${dev#/dev/} > 202 initfs_features="$(echo $initfs_features | > sed 's/raid//') raid" > 203 local level=$(cat /sys/block/$md/md/level) > 204 case "$level" in > 205 raid1) raidmod="$raidmod,$level";; > 206 raid[456]) > raidmod="$raidmod,raid456";; > 207 esac > 208 done > > Might be nice to add a comment explaining that $raidopt records the > raid-status of the bootdev, and is used only if is_xen, in the > invocation of extlinux, It is actually the opposite. The $raidopt is only used if not xen. > whereas raidmod records the raid-status of the > rootvol[s], and is used in the /etc/mkinitfs and > /etc/update-extlinux.conf configuration. This can be extracted by > grepping the source, but it would be friendly to just explain it where > these variables are initialized. Also, if some of the behavior > described here is unintended / leftover cruft, this is a good chance > to fix it. :-) Could any xen user exmplain why we don't want run extlinux when xen is detected? Do we still want this behaviour? > 201 local md=${dev#/dev/} > 202 initfs_features="$(echo $initfs_features | > sed 's/raid//') raid" > ... > 211 case $rootdev in > 212 /dev/cciss/*) > 213 initfs_features="$(echo > $initfs_features | sed 's/raid//') raid" > 214 ;; > > Note to myself: Why are echo... | sed... necessary here? Wouldn't > shell manipulation suffice? Answer: BusyBox ash does let you use > ${FOO//orig/repl} constructs, but we should steer clear of non-POSIX > sh capabilities, and this isn't one. However, if the desired > replacements here were anchored (^raid or raid$), then shell > manipulation would suffice. I think the shell manipulation would suffice in this case. The initfs_features is a local var which we have full control over and the code only makes sure we don't add the "raid" multiple times. Looks like we should do the same with raidmod btw. Seems like there is a chance that raidmod gets added multiple times if the root filesystem is on a lvm with multiple raids as pvs. > > 286 is_xen || extlinux $raidopt --install "$mnt"/boot > > Why do we use extlinux here and syslinux everywhere else? At one > point I understood the difference between them, but now I've > forgotten. syslinux: works only only on FAT. for booting CF or USB and run from tmpfs. isolinux: for booting from cdrom extlinux: is a more general prupose boot loader (like lilo and grub). for booting on a 'sys' installed disk. http://www.syslinux.org/wiki/index.php/EXTLINUX > > > 289 unmount_partitions() { > 290 local mnt="$1" > 291 > 292 # unmount the partitions > 293 umount $(awk '{print $2}' /proc/mounts | grep > ^"$mnt" | sort -r) > 294 } > > Notice that as on line 60, the regexp pattern here is only anchored at > the beginning. Mightn't this break? Shouldn't we anchor it on both > sides? Yes. I think you are right. > > > 411 # setup disk dev in $1 for LVM usage. > 412 # usage: setup_partitions size1,type1 > [size2,type2 ...] > 413 setup_partitions() { > > That comment is misleading; this function is also called on codepaths > that don't use LVM. Fixed. Thanks! > > 436 ) | sfdisk -q -L -uM $diskdev >>/tmp/sfdisk.out || > return 1 > > Who uses /tmp/sfdisk.out? It doesn't show up when I grep all the other > setup-* scripts. I would guess it was for debugging purposes. Probably never used. > > 454 # set up boot device. We only use raid1 for boot devices if > any raid > 455 setup_boot_dev() { > > That comment seems to be truncated or otherwise ungrammatical. Fixed. Thanks! > > > > 492 setup_non_boot_raid_dev() { > ... > 497 local numdevs=$# > > This seems to be cruft; so far as I can see this variable is never > again referenced. Yup. You are right. I have removed this. Thanks! > > 563 # set up /var on given device > 564 setup_var() { > 565 local var_dev="$1" > 566 local varfs=ext4 > > See comment to line 8 about possibly introducing a VARFS env variable. Fixed. Thanks! > > 596 data_only_disk_install_lvm() { > ... > 601 local raid_part_type="fd" > ... > 608 if [ "$USE_RAID" ]; then > 609 part_type=$raid_part_type > 610 stop_all_raid > 611 fi > > Is raid_part_type necessary? It doesn't seem to used anywhere else, > and at several other parallel places in this same script, we just say > part_type="fd". See also comment to line 684, below. I think i did that for remembering that "fd" is the partition type for raid. But you are right of course. Fixed. Thanks! > 661 # setup > 662 setup_root() { > 663 local root_dev="$1" boot_dev="$2" > 664 mkfs.$ROOTFS -q "$root_dev" > 665 mkdir -p /mnt > 666 mount -t $ROOTFS $root_dev /mnt || return 1 > 667 if [ -n "$boot_dev" ]; then > 668 mkdir -p /mnt/boot > 669 mount -t $BOOTFS $boot_dev /mnt/boot || > return 1 > 670 fi > 671 > 672 setup_mdadm_conf > 673 install_mounted_root /mnt || return 1 > 674 unmount_partitions /mnt > 675 swapoff -a > 676 > 677 echo "" > 678 echo "Installation is complete. Please reboot." > 679 } > > setup-bootable uses ${MNT:-/mnt}, instead of just mandating /mnt. > Should we do the same here? Good idea. I wonder if we should use $SYSROOT instead of $MNT though. > 681 native_disk_install_lvm() { > ... > 684 local raid_part_type="fd" > > This variable doesn't seem to be anywhere referenced. Fixed. Thanks! > > 690 init_progs syslinux || return 1 > ... > 721 init_progs syslinux || return 1 > > syslinux is already included in the packages init_progs installs by > default. Fixed. Thanks! > > 817 usage() { > 818 cat <<__EOF__ > 819 usage: setup-disk [-hqr] [-k kernelflavor] [-m MODE] [-o > apkovl] [-s SWAPSIZE] > 820 [MOUNTPOINT | DISKDEV...] > > Shouldn't we s/-hqr/-hqrLv/ ? See below. Yes. > ... > 831 options: > 832 -h Show this help > 833 -m Use disk for MODE without asking, where MODE is either > 'data' or 'root' > > Shouldn't we s/root/sys/ ? Yes. > Perhaps also permit specifying "-m none"? I think in that case you wouldn't call setup-disk in first place. > Alternatively, see comment to line 895, below. > ... > 836 -L Use LVM to manage partitions > ... > 839 -s Use SWAPSIZE MB instead of $SWAP_SIZE MB for swap > (Use 0 to disable swap) > > It looks like, when usage() would normally be run, $SWAP_SIZE won't > yet have been set? You are right. I'll change that to "instead of autodetecting..." > > 840 -v Be more verbose about what is happening > 841 __EOF__ > > > > 895 if [ $# -gt 0 ]; then > > Mightn't it be useful to permit, and check whether, $* = "none"? I think we wouldn't run setup-disk at all in that case. > 896 # check that they are > 897 for i in "$@"; do > 898 j=$(readlink -f "$i" | sed 's:^/dev/::; > s:/:!:g') > 899 if ! [ -e "/sys/block/$j/device" ]; then > 900 echo "$i is not a suitable for > partitioning" > 901 exit 1 > 902 fi > 903 diskdevs="$diskdevs /dev/$j" > 904 done > 905 else > 906 ask_disk "Which disk(s) would you like to use? (or > '?' for help or 'none')" \ > 907 diskselect_help $disks > 908 if [ "$answer" != none ]; then > 909 for i in $answer; do > 910 diskdevs="$diskdevs /dev/$i" > 911 done > 912 else > 913 DISK_MODE="none" > 914 fi > 915 fi > > Thanks a lot for your review of my code and for very useful feedback :) I have done most of the changes in git: http://git.alpinelinux.org/cgit/alpine-conf/log/ Now it needs some testing. There have been various nasty corner cases along the way. -nc --- Unsubscribe: alpine-devel+unsubscribe@lists.alpinelinux.org Help: alpine-devel+help@lists.alpinelinux.org ---