Patch for kickseed - properly log unsupported command options
Hey all,
I'd like to propose the following patch to kickseed. While the patch looks like a lot, it is really the same change applied to each command handler (e.g. handlers/auth.sh, handlers/post.sh, handlers/partition.sh). The problem is that none of the kickseed handlers correctly log messages about unsupported handler options.
All the handlers currently parse options like so:
eval set -- "$(getopt -o '' -l interpreter: -- "$@")" || { warn_getopt %pre; return; }
As mentioned in some bash docs (/usr/share/doc/util-linux-ng-2.17.2/getopt-parse.bash), this will *never* call warn_getopt because eval set swallows the return code. This is true of ash as well.
Because of this, almost every bug report I've seen mentions something along the lines of "getopt printed an error". This is getout outputting to stderr, but this only shows briefly -- there is nothing printed into any log. Proper behavior would be for an unknown option to be printed to stderr (as it is now) and for a log message to be generated (which is what this patch adds).
FWIW, this "swallow" behavior was likely designed intentionally to disable automatic exit (errexit) whenever getopt found an unexpected argument, due to the fact that kickseed supports so few options and therefore this "error" happens on almost every kickstart file. However, it looks like the code was intended to print to both the console and to the log file, and in that respect it's failing.
The patch adds a new function parse_options to kickseed.sh that briefly disables errexit, runs getopt, reenables errexit, and then prints a warning message to the log if there was an error. As ash doesn't support ERR trapping, I capture the stderr output and print a log message if it's not empty. To do this, I have to run getopt twice, but that's a small price to pay for good logging. This doesn't affect the current console behavior at all - a warning is still printed to the tty as the install is running. I've updated each handler, they now check options as so:
eval set -- "$(parse_options %pre interpreter: "$@")"
The patch result is that /var/log/installer/syslog now contains entries such as this for unknown options:
Jun 28 23:49:04 kickseed: Failed to parse some %pre options
Jun 28 23:49:04 kickseed: getopt: unrecognized option '--log'
This makes it much easier to debug an installation that didn't go quite as expected. Feedback and input welcome, if this gets merged in I'm happy to submit my next series of changes.
Best,
Hamilton
diff --git handlers/auth.sh handlers/auth.sh
index 530ef71..0e8ce33 100644
--- handlers/auth.sh
+++ handlers/auth.sh
@@ -5,7 +5,7 @@ auth_handler () {
# --enableldaptls, --enablekrb5, --krb5realm=, --krb5kdc=,
# --krb5adminserver=, --enablehesiod, --hesiodlhs,
# --hesiodrhs, --enablesmbauth, --smbservers=, --smbworkgroup=
- eval set -- "$(getopt -o '' -l enablemd5,enablenis,nisdomain:,nisserver:,useshadow,enableshadow,enablecache -- "$@")" || { warn_getopt auth; return; }
+ eval set -- "$(parse_options auth enablemd5,enablenis,nisdomain:,nisserver:,useshadow,enableshadow,enablecache "$@")"
while :; do
case $1 in
--enablemd5)
diff --git handlers/bootloader.sh handlers/bootloader.sh
index 33b51d5..dd74b55 100644
--- handlers/bootloader.sh
+++ handlers/bootloader.sh
@@ -4,7 +4,7 @@ bootloader_handler_common () {
useLilo="$1"
shift
# TODO --linear, --nolinear, --lba32
- eval set -- "$(getopt -o '' -l location:,password:,md5pass:,useLilo,upgrade -- "$@")" || { warn_getopt bootloader; return; }
+ eval set -- "$(parse_options bootloader location:,password:,md5pass:,useLilo,upgrade "$@")"
while :; do
case $1 in
--location)
diff --git handlers/clearpart.sh handlers/clearpart.sh
index c2b17d9..f6f5ebb 100644
--- handlers/clearpart.sh
+++ handlers/clearpart.sh
@@ -4,7 +4,7 @@ clearpart_method=
clearpart_need_method=
clearpart_handler () {
- eval set -- "$(getopt -o '' -l linux,all,drives:,initlabel -- "$@")" || { warn_getopt clearpart; return; }
+ eval set -- "$(parse_options clearpart linux,all,drives:,initlabel "$@")"
while :; do
case $1 in
--linux)
diff --git handlers/device.sh handlers/device.sh
index 49e595c..c9265e5 100644
--- handlers/device.sh
+++ handlers/device.sh
@@ -2,7 +2,7 @@
device_handler () {
opts=
- eval set -- "$(getopt -o '' -l opts: -- "$@")" || { warn_getopt device; return; }
+ eval set -- "$(parse_options device opts: "$@")"
while :; do
case $1 in
--opts)
diff --git handlers/firewall.sh handlers/firewall.sh
index c02f427..908bed1 100644
--- handlers/firewall.sh
+++ handlers/firewall.sh
@@ -1,7 +1,7 @@
#! /bin/sh
firewall_handler () {
- eval set -- "$(getopt -o '' -l high,medium,enabled,enable,disabled,disable,trust:,dhcp,ssh,telnet,smtp,http,ftp,port: -- "$@")" || { warn_getopt firewall; return; }
+ eval set -- "$(parse_options firewall high,medium,enabled,enable,disabled,disable,trust:,dhcp,ssh,telnet,smtp,http,ftp,port: "$@")"
while :; do
case $1 in
--high|--medium|--enabled|--enable|--dhcp|--ssh|--telnet|--smtp|--http|--ftp)
diff --git handlers/langsupport.sh handlers/langsupport.sh
index 192a119..63f6272 100644
--- handlers/langsupport.sh
+++ handlers/langsupport.sh
@@ -3,7 +3,7 @@
langsupport_default=
langsupport_handler () {
- eval set -- "$(getopt -o '' -l default: -- "$@")" || { warn_getopt langsupport; return; }
+ eval set -- "$(parse_options langsupport default: "$@")"
while :; do
case $1 in
--default)
diff --git handlers/logvol.sh handlers/logvol.sh
index 6c1ac7e..4c586f2 100644
--- handlers/logvol.sh
+++ handlers/logvol.sh
@@ -13,7 +13,7 @@ logvol_handler () {
fstype=
# TODO --percent=, --bytes-per-inode=, --fsoptions=
- eval set -- "$(getopt -o '' -l vgname:,recommended,size:,grow,maxsize:,name:,noformat,fstype: -- "$@")" || { warn_getopt logvol; return; }
+ eval set -- "$(parse_options logvol vgname:,recommended,size:,grow,maxsize:,name:,noformat,fstype: "$@")"
while :; do
case $1 in
--vgname)
diff --git handlers/mouse.sh handlers/mouse.sh
index ea1fd9d..0fab4fb 100644
--- handlers/mouse.sh
+++ handlers/mouse.sh
@@ -6,7 +6,7 @@ mouse_handler () {
return
fi
- eval set -- "$(getopt -o '' -l device:,emulthree -- "$@")" || { warn_getopt mouse; return; }
+ eval set -- "$(parse_options mouse device:,emulthree "$@")"
while :; do
case $1 in
--device)
diff --git handlers/network.sh handlers/network.sh
index b50ba40..dda1b19 100644
--- handlers/network.sh
+++ handlers/network.sh
@@ -5,7 +5,7 @@ network_handler () {
got_gateway=
got_nameservers=
got_netmask=
- eval set -- "$(getopt -o '' -l bootproto:,device:,ip:,gateway:,nameserver:,nodns,netmask:,hostname: -- "$@")" || { warn_getopt network; return; }
+ eval set -- "$(parse_options network bootproto:,device:,ip:,gateway:,nameserver:,nodns,netmask:,hostname: "$@")"
while :; do
case $1 in
--bootproto)
diff --git handlers/partition.sh handlers/partition.sh
index b3da6f4..803a6c1 100644
--- handlers/partition.sh
+++ handlers/partition.sh
@@ -21,7 +21,7 @@ partition_handler () {
asprimary=
fstype=
- eval set -- "$(getopt -o '' -l recommended,size:,grow,maxsize:,noformat,onpart:,usepart:,ondisk:,ondrive:,asprimary,fstype:,start:,end: -- "$@")" || { warn_getopt partition; return; }
+ eval set -- "$(parse_options partition recommended,size:,grow,maxsize:,noformat,onpart:,usepart:,ondisk:,ondrive:,asprimary,fstype:,start:,end: "$@")"
while :; do
case $1 in
--recommended)
diff --git handlers/post.sh handlers/post.sh
index a6e637f..d022f43 100644
--- handlers/post.sh
+++ handlers/post.sh
@@ -5,7 +5,7 @@ post_handler_section () {
post_chroot=1
post_interpreter=
- eval set -- "$(getopt -o '' -l nochroot,interpreter: -- "$@")" || { warn_getopt %post; return; }
+ eval set -- "$(parse_options %post nochroot,interpreter: "$@")"
while :; do
case $1 in
--nochroot)
diff --git handlers/pre.sh handlers/pre.sh
index fa03a80..7f8358c 100644
--- handlers/pre.sh
+++ handlers/pre.sh
@@ -1,7 +1,8 @@
#! /bin/sh
pre_handler_section () {
- eval set -- "$(getopt -o '' -l interpreter: -- "$@")" || { warn_getopt %pre; return; }
+
+ eval set -- "$(parse_options %pre interpreter: "$@")"
while :; do
case $1 in
--interpreter)
diff --git handlers/preseed.sh handlers/preseed.sh
index c2d99c2..9bbecb0 100644
--- handlers/preseed.sh
+++ handlers/preseed.sh
@@ -3,7 +3,7 @@
preseed_handler () {
owner=d-i
- eval set -- "$(getopt -o '' -l owner: -- "$@")" || { warn_getopt preseed; return; }
+ eval set -- "$(parse_options preseed owner: "$@")"
while :; do
case $1 in
--owner)
diff --git handlers/repo.sh handlers/repo.sh
index 2697e14..6e0aed0 100644
--- handlers/repo.sh
+++ handlers/repo.sh
@@ -11,7 +11,7 @@ repo_handler () {
key=
# TODO --mirrorlist=
- eval set -- "$(getopt -o '' -l name:,baseurl:,distribution:,components:,source,key: -- "$@")" || { warn_getopt repo; return; }
+ eval set -- "$(parse_options repo name:,baseurl:,distribution:,components:,source,key: "$@")"
while :; do
case $1 in
--name)
diff --git handlers/rootpw.sh handlers/rootpw.sh
index f375cd2..c6deab4 100644
--- handlers/rootpw.sh
+++ handlers/rootpw.sh
@@ -4,7 +4,7 @@ rootpw_handler () {
setpassword=1
crypted=
- eval set -- "$(getopt -o '' -l disabled,iscrypted -- "$@")" || { warn_getopt rootpw; return; }
+ eval set -- "$(parse_options rootpw disabled,iscrypted "$@")"
while :; do
case $1 in
--disabled)
diff --git handlers/timezone.sh handlers/timezone.sh
index d09bb04..f34a542 100644
--- handlers/timezone.sh
+++ handlers/timezone.sh
@@ -3,7 +3,7 @@
timezone_handler () {
utc=
- eval set -- "$(getopt -o '' -l utc -- "$@")" || { warn_getopt timezone; return; }
+ eval set -- "$(parse_options timezone utc "$@")"
while :; do
case $1 in
--utc)
diff --git handlers/url.sh handlers/url.sh
index 05b2404..d1f6d06 100644
--- handlers/url.sh
+++ handlers/url.sh
@@ -1,7 +1,7 @@
#! /bin/sh
url_handler () {
- eval set -- "$(getopt -o '' -l url: -- "$@")" || { warn_getopt url; return; }
+ eval set -- "$(parse_options url url: "$@")"
while :; do
case $1 in
--url)
diff --git handlers/user.sh handlers/user.sh
index d11b381..2577c4b 100644
--- handlers/user.sh
+++ handlers/user.sh
@@ -5,7 +5,7 @@ user_handler () {
crypted=
password=
- eval set -- "$(getopt -o '' -l disabled,fullname:,iscrypted,password: -- "$@")" || { warn_getopt user; return; }
+ eval set -- "$(parse_options user disabled,fullname:,iscrypted,password: "$@")"
while :; do
case $1 in
--disabled)
diff --git handlers/volgroup.sh handlers/volgroup.sh
index b3a4ea5..ee33146 100644
--- handlers/volgroup.sh
+++ handlers/volgroup.sh
@@ -34,7 +34,7 @@ volgroup_pull_recipe () {
volgroup_handler () {
existing=
- eval set -- "$(getopt -o '' -l noformat,useexisting,pesize: -- "$@")" || { warn_getopt volgroup; return; }
+ eval set -- "$(parse_options volgroup noformat,useexisting,pesize: "$@")"
while :; do
case $1 in
--noformat|--useexisting)
diff --git handlers/xconfig.sh handlers/xconfig.sh
index 43f064e..8e8bd94 100644
--- handlers/xconfig.sh
+++ handlers/xconfig.sh
@@ -1,7 +1,7 @@
#! /bin/sh
xconfig_handler () {
- eval set -- "$(getopt -o '' -l noprobe,card:,videoram:,monitor:,hsync:,vsync:,defaultdesktop:,startxonboot,resolution:,depth: -- "$@")" || { warn_getopt xconfig; return; }
+ eval set -- "$(parse_options xconfig noprobe,card:,videoram:,monitor:,hsync:,vsync:,defaultdesktop:,startxonboot,resolution:,depth: "$@")"
while :; do
case $1 in
--noprobe)
diff --git kickseed.sh kickseed.sh
index ccc65dd..476ccc1 100644
--- kickseed.sh
+++ kickseed.sh
@@ -24,7 +24,7 @@ die () {
}
warn_getopt () {
- warn "Failed to parse $1 options"
+ warn "Failed to parse some $1 options"
}
warn_bad_opt () {
@@ -35,6 +35,30 @@ warn_bad_arg () {
warn "Unimplemented $1 $2 argument $3"
}
+# Used by HANDLERS for parsing command options
+#
+# Handles unknown options by outputting errors to stderr and
+# syslog. Important because we only handle a few options, so
+# users need a good log if an install didn't complete as
+# expected
+parse_options () {
+ type=$1
+ options=$2
+ shift; shift;
+
+ # Disable errexit so we can handle it specially
+ set +e
+ output=$(getopt -o '' -l $options -- "$@")
+ errout=$(getopt -o '' -l $options -- "$@" 2>&1 >/dev/null)
+ set -e
+
+ if [[ -n "$errout" ]]; then
+ warn_getopt $type
+ warn "$type Parse Error: $errout"
+ fi
+ echo "$output"
+}
+
# Allow handler files to register functions to be called at the end of
# processing; this lets them build up preseed answers over several handler
# calls.
@@ -122,7 +146,7 @@ kickseed () {
echo "$line"
fi
done < "$1"; echo %final) | while read -r line; do
- # Work out the section.
+ # Check if we've reached a new section
keyword="${line%%[ ]*}"
if [ "$keyword" = '%packages' ]; then
save_post_script
@@ -151,6 +175,8 @@ kickseed () {
break
fi
+ # We are inside a section. Based on what section
+ # we are in, evaluate the current line
if [ "$SECTION" = main ]; then
if [ -z "$keyword" ] || [ "${keyword#\#}" != "$keyword" ]; then
# Ignore empty lines and comments.
Reply to: