[Ovirt-devel] [PATCH] Add trivial error() function and change code to use it

[Jeff Schroeder]

On Sat, Oct 4, 2008 at 1:46 AM, Jim Meyering <jim at meyering.net> wrote:
> "Perry N. Myers" <pmyers at redhat.com> wrote:
>> Jeff Schroeder wrote:
>>> Remove the multiple echo ... && exit 1 references and replace
>>> them with a simple function. This makes the code a bit cleaner.
>>
>> No problem with this patch in concept, but this patch appears to be
>> against the old repository.  See the instructions on:
>> http://ovirt.org/build-instructions.html
>>
>> for getting access to the new repositories.
>>
>> The file you're patching is now located in the ovirt-server repository
>> and it's location is:
>>
>> scripts/ovirt-server-install
>
> ...
>>>  +error() {
>>> +    echo $* >&2
>>> +    exit 1
>>> +}
>
> Hi Jeff,
>
> Thanks for the clean up.
>
> However, the above use of $* is underquoted.
> Here are a couple examples of what can go wrong:
>
>  shell tokenization
>    $ bash -c 'error() { echo $*; }; error "a          b"'
>    a b
>
>  ok when no file matches the "a*" glob:
>    $ bash -c 'error() { echo $*; }; error "a*"'
>    a*
>
>  but when one does, ...
>    $ touch a-file
>    $ bash -c 'error() { echo $*; }; error "a*"'
>    a-file
>
> So, please use something like the existing "die",
> which also prints the usual "$program_name: " prefix.
>
> The following appears in several files:
>
>  ME=$(basename "$0")
>  warn() { printf "$ME: $@\n" >&2; }
>  die() { warn "$@"; exit 1; }
>
> But I've just realized that's not quite correct, in case $ME or $@ ever
> expand to something containing a % (or, far less likely, a \c sequence
> that printf recognizes), so this is better:
>
>  ME=$(basename "$0")
>  warn() { printf '%s: %s\n' "$ME" "$*" >&2; }
>  die() { warn "$@"; exit 1; }

Uggg, you write perl in bourne shell! That looks hideous, but you are
completely right. Good catch. The split up repositories make this more
difficult, but what do you think of putting obvious helper functions like
this in a small utility library and sourcing it from the scripts? Is there any
way we could share these among the various ovirt-* projects? Does
git have a svn:externals equivalent?

> For the record, none of our existing uses of warn or die
> would trigger the malfunction.
>
> I've just posted a patch to fix those warn definitions.

Speaking of that, I've got a few silly ideas that you might have a better
way of doing. While looking at the ovirt-server-install script, there is a
FIXME that asks for input if it can't find something like the dns srv
records. I would love your input on this and especially on validate().
Is there a better way to do it? The code is rife with bashisms such as
[[ ]], ==, local varname, and the shebang is bash so a few more won't hurt.

prompt() {
    # Bourne shell's "return" doesn't support strings so use a global
    unset answer
    local question="$1";    shift
    test -z "$question" && return
    while [ -z "$answer" ]; do
        printf '%s?: ' "$question"
        read answer
    done
}

validate() {
    local text=$1;       shift
    local pattern="$1"; shift
    test -z "$pattern" -o -z "$text" && return 1
    printf '%s\n' "$text" | egrep -qs "$pattern" &>/dev/null
    return $?
}

A use case would be something like this probably whitespace damaged patch:
--- a/scripts/ovirt-server-install
+++ b/scripts/ovirt-server-install
@@ -124,9 +124,9 @@ if [ -n "$SRV_HOST" -a -n "$SRV_PORT" ]; then
         -e "s/base: .*/base: $srv_base/g" \
         $LDAP_CFG
 else
-    # FIXME: Eventually this script should prompt for things that can't
-    # be found in DNS SRV records.
-    error "Failed to get ldap host/port"
+    prompt 'Enter the LDAP hostname:port info (ie: ldap.example.com:389)'
+    validate "$answer" '^[[:alpha:]].*:[0-9]+$' || \
+    error "Failed to get a valid ldap host/port"
 fi

 # setup an NTP step-ticker

> paranoid^W defensively, ...

Paranoid is good. Thanks for the great reviews and pointing out subtle gotchas!
They won't be forgotten anytime soon. Once again though, these should be in a
utility lib somewhere so that all scripts can take advantage of them.
Thats why I
haven't yet posted an updated patch. Do you have any words of wisdom on this?

> Jim

-- 
Jeff Schroeder

Don't drink and derive, alcohol and analysis don't mix.
http://www.digitalprognosis.com