X-Original-To: alpine-devel@lists.alpinelinux.org Delivered-To: alpine-devel@mail.alpinelinux.org Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.alpinelinux.org (Postfix) with ESMTPS id 1E9DADC19A2 for ; Sun, 4 Mar 2012 21:47:31 +0000 (UTC) Received: from compute3.internal (compute3.nyi.mail.srv.osa [10.202.2.43]) by gateway1.nyi.mail.srv.osa (Postfix) with ESMTP id 806C821324 for ; Sun, 4 Mar 2012 16:47:31 -0500 (EST) Received: from web4.nyi.mail.srv.osa ([10.202.2.214]) by compute3.internal (MEProxy); Sun, 04 Mar 2012 16:47:31 -0500 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=message-id:from:to:mime-version :content-transfer-encoding:content-type:in-reply-to:references :subject:date; s=smtpout; bh=E5ajLNNytCuEy5et8alM/74mLxc=; b=FER b3wBB5+1naHJohoDBtY1FpSmyi6EuqQ2bbWcC2lESJpWLFodc7d9F699Ge5dd56u I72cWgFEfydbySj7YqezvmmVaYhmgbKGU6vIuIEUFrccXcN2Ynmfmg9CDDWTNpKq QRGmd7bk1brobimdHsB9t93ucgchL7aSJom0sFkI= Received: by web4.nyi.mail.srv.osa (Postfix, from userid 99) id 3814F3C1EEF; Sun, 4 Mar 2012 16:47:31 -0500 (EST) Message-Id: <1330897651.26242.140661044650293@webmail.messagingengine.com> X-Sasl-Enc: gnS9iR4RPYG7TsNlGfSNyNutekndOVQ9TeWaJlftcTGQ 1330897651 From: Jim Pryor To: alpine-devel@lists.alpinelinux.org X-Mailinglist: alpine-devel Precedence: list List-Id: Alpine Development List-Unsubscribe: List-Post: List-Help: List-Subscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii" X-Mailer: MessagingEngine.com Webmail Interface In-Reply-To: <1330880212.26962.140661044634665@webmail.messagingengine.com> References: <1330852060.31716.140661044540177@webmail.messagingengine.com> <1330880212.26962.140661044634665@webmail.messagingengine.com> Subject: Re: [alpine-devel] Understanding alpine-setup Date: Sun, 04 Mar 2012 16:47:31 -0500 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? 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. 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? 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. 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? 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, 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. :-) 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. 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. 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? 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. 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. 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. 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. 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. 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. 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? 681 native_disk_install_lvm() { ... 684 local raid_part_type="fd" This variable doesn't seem to be anywhere referenced. 690 init_progs syslinux || return 1 ... 721 init_progs syslinux || return 1 syslinux is already included in the packages init_progs installs by default. 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. ... 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/ ? Perhaps also permit specifying "-m none"? 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? 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"? 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 -- Jim Pryor jim@jimpryor.net --- Unsubscribe: alpine-devel+unsubscribe@lists.alpinelinux.org Help: alpine-devel+help@lists.alpinelinux.org ---