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 [email protected]
>
> with a subject of "unsubscribe". Trouble? Contact [email protected]
>
> Archive:
> https://lists.debian.org/[email protected]
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
Archive:
https://lists.debian.org/[email protected]
