cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

First thought, a question:
  Would it be better to put this into base_files or even openssh-server ?
The scenario is not at all related to cloud-init.  I'd be just as happy
to have cloud-init disable its behavior based on a fix present in another
package.

You need to fix the commit message.
lines should only be 74 chars wide.

I suggest for a subject line:
 Z99-cloud-locale-test: Fix invalid locale settings, do not print messages.

http://paste.ubuntu.com/p/s8dgFRnx43/

I've played around a bit and suggest the above.

Things I've changed there:

 * fix some complaints from shellcheck
 * make 'locale_fixup' take a default locale as param 1, using UTF-8.C as the default.
 * Avoided the double 'case' by just checking for empty 'val' or val that starts with '"'.
   I admit the primary reason for this is that 'vim' syntax highlighting didn't like
   the \"*\" that you had.  My replaced version seems to be slightly faster, but
   that speedup is nominal compared to the subshells involved in $(locale | function)
 * I dropped the 'LANG' section because I can't figure out how to trip it.
   Also here the check for presense of 'bad' in 'bad_set' is now safer.  if both
   LANG and LANGUAGE ever *did* make it into the 'bad_set', then yours would have
   LANG match LAUNGUAGE.
 * append to a variable and a single 'echo' rather than multiple lines in output.


Diff comments:

> diff --git a/tools/Z99-cloud-locale-test.sh b/tools/Z99-cloud-locale-test.sh
> index 4978d87..38cfd05 100644
> --- a/tools/Z99-cloud-locale-test.sh
> +++ b/tools/Z99-cloud-locale-test.sh
> @@ -22,79 +25,42 @@ locale_warn() {
>      # VARIABLE=
>      # VARIABLE="value"
>      # locale: Cannot set LC_SOMETHING to default locale
> +    # glibc's 'locale' command outputs VARIABLE=foo for variables set in
> +    # the environment, and VARIABLE="foo" for locale settings that are
> +    # derived.  Use this to minimize the set of variables we are overriding
> +    # to those that are specifically detected to be broken.
>      while read -r w1 w2 w3 w4 remain; do
>          case "$w1" in
>              locale:) bad_names="${bad_names} ${w4}";;
>              *)
>                  key=${w1%%=*}
>                  val=${w1#*=}
> -                val=${val#\"}
> -                val=${val%\"}
> -                vars="${vars} $key=$val";;
> +                case $val in
> +                    \"*\"|"")
> +                         # quoted means inferred; don't override it,
> +                         # nor an empty value
> +                         ;;
> +                     *)
> +                         set_vars="$key $set_vars"
> +                         ;;
> +                esac
>          esac
>      done
>      for bad in $bad_names; do
> -        for var in ${vars}; do
> -            [ "${bad}" = "${var%=*}" ] || continue
> -            val=${var#*=}
> -            [ "${bad_lcs#* ${val}}" = "${bad_lcs}" ] &&
> -                bad_lcs="${bad_lcs} ${val}"
> -            bad_kv="${bad_kv} $bad=$val"
> -            break
> -        done
> -    done
> -    bad_lcs=${bad_lcs# }
> -    bad_kv=${bad_kv# }
> -    [ -n "$bad_lcs" ] || return 0
> -
> -    printf "_____________________________________________________________________\n"
> -    printf "WARNING! Your environment specifies an invalid locale.\n"
> -    printf " The unknown environment variables are:\n   %s\n" "$bad_kv"
> -    printf " This can affect your user experience significantly, including the\n"
> -    printf " ability to manage packages. You may install the locales by running:\n\n"
> -
> -    local bad invalid="" to_gen="" sfile="/usr/share/i18n/SUPPORTED"
> -    local pkgs=""
> -    if [ -e "$sfile" ]; then
> -        for bad in ${bad_lcs}; do
> -            grep -q -i "${bad}" "$sfile" &&
> -                to_gen="${to_gen} ${bad}" ||
> -                invalid="${invalid} ${bad}"
> -        done
> -    else
> -        printf "  sudo apt-get install locales\n"
> -        to_gen=$bad_lcs
> -    fi
> -    to_gen=${to_gen# }
> -
> -    local pkgs=""
> -    for bad in ${to_gen}; do
> -        pkgs="${pkgs} language-pack-${bad%%_*}"
> -    done
> -    pkgs=${pkgs# }
> -
> -    if [ -n "${pkgs}" ]; then
> -        printf "   sudo apt-get install ${pkgs# }\n"
> -        printf "     or\n"
> -        printf "   sudo locale-gen ${to_gen# }\n"
> -        printf "\n"
> -    fi
> -    for bad in ${invalid}; do
> -        printf "WARNING: '${bad}' is an invalid locale\n"
> +        case $set_vars in
> +            *$bad*)
> +                echo "$bad=$default_locale"
> +                echo export $bad

export is not necessary. we are not declaring new variables. in order for 'locale' to read the variable, it must already have been exported.  Even if that were not the case, if it *wasn't* exported, there is no reason for us to decide that it should be.

ie, simply changing the value of a variable in the environment changes the inherited variable. you only need export if you are putting a new variable into the inherited environment.

$ HOME=/home/smoser; sh -c 'echo first: HOME=$HOME; HOME=wark; echo after: HOME=$HOME; sh -c "echo subshell: HOME=\$HOME"' 
first: HOME=/home/smoser
after: HOME=wark
subshell: HOME=wark

> +                ;;
> +            *LANG\ *)
> +                echo "LANG=$default_locale"
> +                echo export LANG

>From what I can tell (cosmic) locale will never mention LANG in its 'locale: Cannot set' messages.

What case does this catch/fix?

> +                ;;
> +        esac
>      done
> -
> -    printf "To see all available language packs, run:\n"
> -    printf "   apt-cache search \"^language-pack-[a-z][a-z]$\"\n"
> -    printf "To disable this message for all users, run:\n"
> -    printf "   sudo touch /var/lib/cloud/instance/locale-check.skip\n"
> -    printf "_____________________________________________________________________\n\n"
> -
> -    # only show the message once
> -    : > ~/.cloud-locale-test.skip 2>/dev/null || :
>  }
>  
> -[ -f ~/.cloud-locale-test.skip -o -f /var/lib/cloud/instance/locale-check.skip ] ||
> -    locale 2>&1 | locale_warn
> +eval $(locale 2>&1 | locale_fixup)

eval is just scary. i'd really prefer avoiding it.
I cant find a problem with what we have as we are pretty safely parsing locale output.

Additionally, this will put an additional 2 subshells into all user's login
paths.  In the "happy server" path, thats not really that big of a deal
(my system will run "locale 2>&1 | locale_fixup" 1000 times in 1.4 seconds or so).
But in the case where a server is overloaded, and you're trying to get in there
to fix it.... things like this are painful.

The old code would only end up in an additional stat of a file (other than the
first time it ran).

>  
> -unset locale_warn
> +unset locale_fixup
>  # vi: ts=4 expandtab


-- 
https://code.launchpad.net/~vorlon/cloud-init/+git/cloud-init/+merge/348101
Your team cloud-init commiters is requested to review the proposed merge of ~vorlon/cloud-init:lp.1134036 into cloud-init:master.