[arch-projects] [initscripts][PATCH 0/7] Cleanup and new feature
Hi all, This fixes a couple of items that have been nagging at me, the largest of which being that we parse environment files such as /etc/locale.conf. This is pretty bad, imo, and we shouldn't be doing it. Benefits of not doing it are laid out in the patch that implements it. Tom, I'm not sure when you planned on doing a release, but hopefully some of these are small enough that you won't mind merging them. d Dave Reisner (7): avoid unnecessary escaping of newlines fix non-uniform indentation functions: remove redundant 'return $?' functions: properly quote ${mounts[@]} functions: declare locale vars in an array for reuse functions: implement a environment file parser PKGBUILD: use %Y%m%d format instead of %s PKGBUILD | 2 +- functions | 70 ++++++++++++++++++++++++++++++++++++++++++++++++---------- rc.shutdown | 2 +- rc.sysinit | 16 ++++++------ 4 files changed, 68 insertions(+), 22 deletions(-) -- 1.7.7.2
The bash parser assumes that an expression continues when || is at the end of a line. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- rc.sysinit | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/rc.sysinit b/rc.sysinit index 1c74bd9..eb66935 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -15,14 +15,14 @@ printsep mountpoint -q /proc || mount -n -t proc proc /proc -o nosuid,noexec,nodev mountpoint -q /sys || mount -n -t sysfs sys /sys -o nosuid,noexec,nodev mountpoint -q /run || mount -n -t tmpfs run /run -o mode=0755,size=10M,nosuid,nodev -mountpoint -q /dev || mount -n -t devtmpfs udev /dev -o mode=0755,size=10M,nosuid \ - || mount -n -t tmpfs udev /dev -o mode=0755,size=10M,nosuid +mountpoint -q /dev || mount -n -t devtmpfs udev /dev -o mode=0755,size=10M,nosuid || + mount -n -t tmpfs udev /dev -o mode=0755,size=10M,nosuid mkdir -p -m 1777 /run/lock mkdir -p /dev/{pts,shm} -mountpoint -q /dev/pts || mount -n /dev/pts &>/dev/null \ - || mount -n -t devpts devpts /dev/pts -o mode=0620,gid=5,nosuid,noexec -mountpoint -q /dev/shm || mount -n /dev/shm &>/dev/null \ - || mount -n -t tmpfs shm /dev/shm -o mode=1777,nosuid,nodev +mountpoint -q /dev/pts || mount -n /dev/pts &>/dev/null || + mount -n -t devpts devpts /dev/pts -o mode=0620,gid=5,nosuid,noexec +mountpoint -q /dev/shm || mount -n /dev/shm &>/dev/null || + mount -n -t tmpfs shm /dev/shm -o mode=1777,nosuid,nodev # remount root ro to allow for fsck later on, we remount now to # make sure nothing can open files rw on root which would block a remount -- 1.7.7.2
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- functions | 2 +- rc.shutdown | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/functions b/functions index 81328df..471c7d1 100644 --- a/functions +++ b/functions @@ -370,7 +370,7 @@ activate_vgs() { # Newer packages will only have /sbin/cryptsetup and no static binary # This ensures maximal compatibility with the old and new layout for CS in /sbin/cryptsetup /usr/sbin/cryptsetup \ - /sbin/cryptsetup.static ''; do + /sbin/cryptsetup.static ''; do [[ -x $CS ]] && break done diff --git a/rc.shutdown b/rc.shutdown index 86ff799..30708ce 100755 --- a/rc.shutdown +++ b/rc.shutdown @@ -121,7 +121,7 @@ if [[ -x /run/initramfs/shutdown ]]; then else status "Remounting Root Filesystem Read-only" \ - mount -n -o remount,ro / + mount -n -o remount,ro / # Power off or reboot printsep -- 1.7.7.2
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- functions | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/functions b/functions index 471c7d1..d388260 100644 --- a/functions +++ b/functions @@ -418,7 +418,6 @@ NETFS="nfs,nfs4,smbfs,cifs,codafs,ncpfs,shfs,fuse,fuseblk,glusterfs,davfs,fuse.g # Check local filesystems fsck_all() { fsck -A -T -C"$FSCK_FD" -a -t "no${NETFS//,/,no},noopts=_netdev" $FORCEFSCK - return $? } # Single-user login and/or automatic reboot after fsck (if needed) -- 1.7.7.2
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- This one is fairly important... functions | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/functions b/functions index d388260..7de58c1 100644 --- a/functions +++ b/functions @@ -485,7 +485,7 @@ umount_all() { mounts+=("$target") done < <(findmnt -runRo TARGET,FSTYPE,OPTIONS / | tac) - umount -r ${mounts[@]} + umount -r "${mounts[@]}" } -- 1.7.7.2
This also fixes a bug that unintentionally sets LOCALE instead of just defaulting to a value when its empty. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- functions | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-) diff --git a/functions b/functions index 7de58c1..ce664ed 100644 --- a/functions +++ b/functions @@ -5,6 +5,10 @@ # sanitize PATH (will be overridden later when /etc/profile is sourced, but is useful for UDev) export PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" +localevars=(LANG LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY + LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE + LC_MEASUREMENT LC_IDENTIFICATION LC_ALL) + if [[ $1 == "start" ]]; then if [[ $STARTING ]]; then echo "A daemon is starting another daemon, this is unlikely to work as intended." @@ -64,17 +68,14 @@ unset TERM_COLORS unset TZ # sanitize the locale settins -unset LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL +unset "${localevars[@]}" + if [[ $DAEMON_LOCALE = [yY][eE][sS] ]]; then - LANG="${LOCALE:=C}" - if [ -r /etc/locale.conf ]; then + LANG=${LOCALE:-C} + if [[ -r /etc/locale.conf ]]; then . /etc/locale.conf fi - export LANG LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL + export "${localevars[@]}" else export LANG=C fi -- 1.7.7.2
This adds a 'parse_envfile' function that reads files such as /etc/locale.conf and /etc/vconsole.conf without sourcing them as bash logic. Several benefits are realized from this: - Impossible to execute arbitrary code - Bad syntax won't prevent the entire file from being read - Possible to limit what variables are allowed Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- functions | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- rc.sysinit | 4 ++-- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/functions b/functions index ce664ed..387f2af 100644 --- a/functions +++ b/functions @@ -9,6 +9,8 @@ localevars=(LANG LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT LC_IDENTIFICATION LC_ALL) +vconsolevars=(KEYMAP KEYMAP_TOGGLE FONT FONT_MAP FONT_UNIMAP) + if [[ $1 == "start" ]]; then if [[ $STARTING ]]; then echo "A daemon is starting another daemon, this is unlikely to work as intended." @@ -70,12 +72,56 @@ unset TZ # sanitize the locale settins unset "${localevars[@]}" +parse_envfile() { + local file=$1 validkeys=${@:2} ret=0 lineno=0 key= val= + + if [[ -z $file ]]; then + printf "error: no environment file specified\n" + return 1 + fi + + if [[ ! -f $file ]]; then + printf "error: cannot parse \`%s': No such file or directory\n" "$file" + return 1 + fi + + if [[ ! -r $file ]]; then + printf "error: cannot read \`%s': Permission denied\n" "$file" + return 1 + fi + + while IFS='=' read -r key val; do + (( ++lineno )) + + # trim whitespace, avoiding usage of a tempfile + key=$(echo "$1" | { read -r val; echo "$val"; }) + val=$(echo "$1" | { read -r val; echo "$val"; }) + + [[ $key ]] || continue + + if [[ -z $val ]]; then + printf "error: found key \`%s' without value on line %s of %s\n" \ + "$key" "$lineno" "$file" + (( ++ret )) + continue + fi + + # ignore invalid keys if we have a list of valid ones + if (( ${#validkeys[*]} )); then + in_array "$key" "${validkeys[@]}" || continue + fi + + export "$key=$val" || (( ++ret )) + done <"$file" + + return $ret +} + if [[ $DAEMON_LOCALE = [yY][eE][sS] ]]; then LANG=${LOCALE:-C} if [[ -r /etc/locale.conf ]]; then - . /etc/locale.conf + parse_envfile /etc/locale.conf "${localevars[@]}" fi - export "${localevars[@]}" else export LANG=C fi diff --git a/rc.sysinit b/rc.sysinit index eb66935..654a409 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -237,7 +237,7 @@ if [[ $HOSTNAME ]]; then fi if [[ -s /etc/locale.conf ]]; then - . /etc/locale.conf + parse_envfile /etc/locale.conf "LANG" [[ $LANG ]] && LOCALE=$LANG fi if [[ ${LOCALE,,} =~ utf ]]; then @@ -263,7 +263,7 @@ else fi if [[ -s /etc/vconsole.conf ]]; then - . /etc/vconsole.conf + parse_envfile /etc/vconsole.conf "${vconsolevars[@]}" [[ $FONT ]] && CONSOLEFONT=$FONT [[ $FONT_MAP ]] && CONSOLEMAP=$FONT_MAP fi -- 1.7.7.2
On Mon, Nov 7, 2011 at 11:27 AM, Dave Reisner <d@falconindy.com> wrote:
ds a 'parse_envfile' function that reads files such as /etc/locale.conf and /etc/vconsole.conf without sourcing them as bash logic. Several benefits are realized from this:
- Impossible to execute arbitrary code - Bad syntax won't prevent the entire file from being read - Possible to limit what variables are allowed
Awesome! -t
Using seconds from epoch will cause 'makepkg -i' to fail. Lame. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- PKGBUILD | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/PKGBUILD b/PKGBUILD index deddbd5..2667db0 100644 --- a/PKGBUILD +++ b/PKGBUILD @@ -1,5 +1,5 @@ pkgname=initscripts-git -pkgver=$(date +%s) +pkgver=$(date +%Y%m%d) pkgrel=$(git log -1 --pretty=format:%h) pkgdesc="System initialization/bootup scripts" arch=('any') -- 1.7.7.2
On Mon, Nov 7, 2011 at 11:27 AM, Dave Reisner <d@falconindy.com> wrote:
Hi all,
This fixes a couple of items that have been nagging at me, the largest of which being that we parse environment files such as /etc/locale.conf. This is pretty bad, imo, and we shouldn't be doing it. Benefits of not doing it are laid out in the patch that implements it.
Tom, I'm not sure when you planned on doing a release, but hopefully some of these are small enough that you won't mind merging them.
I'll merge them all and delay the release. The reason being that your big patch (which I love) correctly restricts what can be in vconsole.conf and locale.conf. Since vconsole.conf is new and locale.conf is almost new, I think it makes sense to include this as soon as possible. Cheers, Tom
participants (2)
-
Dave Reisner
-
Tom Gundersen