Ping? On Saturday, June 28, 2014 10:40:01 PM UTC-4, Hamilton Turner wrote: > 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. > > > > > > -- > > To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org > > with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org > > Archive: > https://lists.debian.org/747b34a3-76d2-4da3-b170-92f7eddf2...@googlegroups.com
-- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/7800e2b5-1982-40b8-a2d2-b1939bfbb...@googlegroups.com