[pacman-dev] [PATCH 6/7] pacman-key: fix parsing of command line options

[Dan McGee]

On Thu, Apr 21, 2011 at 8:59 AM,  <ivan.kanak at gmail.com> wrote:
> From: Ivan Kanakarakis <ivan.kanak at gmail.com>
>
> This commit fixes handling of --help/-h --version/-V options. Previously
> $ pacman-key --help
> would return errors if the configuration file was missing or the user
> did not have root privileges. Same was for --version switch.
> Moreover this fixes a bug that caused the command line specified
> gpgdir (PACMAN_KEYRING_DIR) to be overridden by the values from the
> configuration file.
>
> The parsing now happens as follows:
> a. The script parses --config and/or --gpgdir if those are specified.
> These options should be the first arguments given to the script.
> b. The script reads the first command, that is the first argument after
> the --config and/or --gpgdir parsing is done.
> If no command was provided the script exits with an error.
> If the command is one of --help -h --version -V then it skip the checks
> that would otherwise be needed, executes those and exits.
> If the command is other, the script checks by order:
>  - the dependencies are met (currently existence of gpg executable)
>  - the user has root privileges
>  - the configuration file is readable
>  - the gpgdir is set and exists
> c. If all checks pass the command is matched to a case and the
> appropriate set of commands is executed.
>
> Signed-off-by: Ivan Kanakarakis <ivan.kanak at gmail.com>
> ---
>  scripts/pacman-key.sh.in |   66 +++++++++++++++++++++++++---------------------
>  1 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in
> index b5fca2b..1d0229b 100644
> --- a/scripts/pacman-key.sh.in
> +++ b/scripts/pacman-key.sh.in
> @@ -217,20 +217,11 @@ if ! type gettext &>/dev/null; then
>        }
>  fi
>
> -if [[ $1 != "--version" && $1 != "-V" && $1 != "--help" && $1 != "-h" && $1 != "" ]]; then
> -       if ! type -p gpg &>/dev/null; then
> -               error "$(gettext "gnupg does not seem to be installed.")"
> -               msg2 "$(gettext "pacman-key requires gnupg for most operations.")"
> -               exit 1
> -       elif (( EUID != 0 )); then
> -               error "$(gettext "pacman-key needs to be run as root.")"
> -               exit 1
> -       fi
> -fi
> +# Set default values
> +CONFIG_DEF="@sysconfdir@/pacman.conf"
> +PACMAN_KEYRING_DIR_DEF="@sysconfdir@/pacman.d/gnupg"
>
> -# Parse global options
> -CONFIG="@sysconfdir@/pacman.conf"
> -PACMAN_KEYRING_DIR="@sysconfdir@/pacman.d/gnupg"
> +# Parse command line options
>  while [[ $1 =~ ^--(config|gpgdir)$ ]]; do
>        case "$1" in
>                --config) shift; CONFIG="$1" ;;
> @@ -239,23 +230,7 @@ while [[ $1 =~ ^--(config|gpgdir)$ ]]; do
>        shift
>  done
>
> -if [[ ! -r "${CONFIG}" ]]; then
> -       error "$(gettext "%s not found.")" "$CONFIG"
> -       exit 1
> -fi
> -
> -# Read GPGDIR from $CONFIG.
> -if [[ GPGDIR="$(get_from "$CONFIG" "GPGDir")" == 0 ]]; then
> -       PACMAN_KEYRING_DIR="${GPGDIR}"
> -fi
> -GPG_PACMAN="gpg --homedir ${PACMAN_KEYRING_DIR} --no-permission-warning"
> -
> -# Try to create $PACMAN_KEYRING_DIR if non-existent
> -# Check for simple existence rather than for a directory as someone may want
> -# to use a symlink here
> -[[ -e ${PACMAN_KEYRING_DIR} ]] || mkdir -p -m 755 "${PACMAN_KEYRING_DIR}"
> -
> -# Parse and execute command
> +# Parse the command
>  command="$1"
>  if [[ -z "${command}" ]]; then
>        usage
> @@ -263,6 +238,37 @@ if [[ -z "${command}" ]]; then
>  fi
>  shift
>
> +# If command is --help/-h or --version/-V then skip checks and execute those
> +if [[ ! ${command} =~ ^(--help|-h|--version|-V)$ ]]; then
> +    # check dependencies
> +    if ! type -p gpg &>/dev/null; then
> +         error "$(gettext "gnupg does not seem to be installed.")"
> +         msg2 "$(gettext "pacman-key requires gnupg for most operations.")"
> +         exit 1
> +    fi
> +    # check permissions
> +    if (( ! EUID )); then
> +        error "$(gettext "pacman-key needs to be run as root.")"
> +        exit 1
> +    fi
> +    # Use the default CONFIG if the user didn't specify one
> +    # If configuration file is not readable use the default
> +    # even if it was specified by the user with --config
-1 from me. This makes no sense other than confusion for the user. If
I specify a bogus or unreadable file, blow up on me. Fixing this to
not be magic also means you don't need CONFIG_DEF at all either.

> +    [[ ! -r $CONFIG ]] && CONFIG="$CONFIG_DEF"
> +    # if the user didn't specify PACMAN_KEYRING_DIR
> +    # then read GPGDir from the configuration file
> +    [[ -z $PACMAN_KEYRING_DIR ]] && PACMAN_KEYRING_DIR="$(get_from "$CONFIG" "GPGDir")"
> +    # if no such setting exists, use the default value
> +    [[ -z $PACMAN_KEYRING_DIR ]] && PACMAN_KEYRING_DIR="$PACMAN_KEYRING_DIR_DEF"
> +    # Try to create $PACMAN_KEYRING_DIR if non-existent
> +    # Check for simple existence rather than a directory
> +    # as someone may want to use a symlink here
> +    [[ ! -e ${PACMAN_KEYRING_DIR} ]] && mkdir -p -m 755 "${PACMAN_KEYRING_DIR}"
> +fi
> +
> +GPG_PACMAN="gpg --homedir ${PACMAN_KEYRING_DIR} --no-permission-warning"
> +
> +# Execute the command
>  case "${command}" in
>        -a|--add)
>                # If there is no extra parameter, gpg will read stdin
> --

Looks OK to me otherwise, but I'll let Allan weigh in as well.

-Dan