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

Reply via email to