[Ovirt-devel] [PATCH node] Check to make sure a config file was applied, if not revert to default config

[Jim Meyering]

Perry Myers <pmyers at redhat.com> wrote:

> ovirt-early tries to use the managed_node controller on the server
> to get the remote config.  If there is none available a file is returned
> but there is presently no way to determine if that file is 'empty' or not
>
> This check looks to see if any ifup-eth* files were created.  If none were, then
> we assume the config was blank and use the default bridge config of one bridge
> per physical interface.
>
> Signed-off-by: Perry Myers <pmyers at redhat.com>
> ---
>  scripts/ovirt-early |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/scripts/ovirt-early b/scripts/ovirt-early
> index 5843fed..95e8bf7 100755
> --- a/scripts/ovirt-early
> +++ b/scripts/ovirt-early
> @@ -49,7 +49,7 @@ configure_from_network() {
>                          if [ -f /var/tmp/node-augtool ]; then
>                              augtool < /var/tmp/node-augtool
>                          fi
> -                        if [ $? -eq 0 ]; then
> +                        if [[ $? -eq 0 && -f /etc/sysconfig/network-scripts/ifcfg-eth* ]]; then

That fails with a syntax error if there are two or more matching files.

    $ touch ab1 ab2; bash -c 'test -f ab* && echo ok'
    bash: line 0: test: ab1: binary operator expected

How about this instead?  Then, we use the exit status of
ls to tell us whether there were matches:

  if test $? = 0 \
    && ls /etc/sysconfig/network-scripts/ifcfg-eth* > /dev/null 2>&1; then

There's also a problem independent of your change that I missed in
an earlier review:

That test of $? was supposed to refer to augtool's exit status.
But now, it can also refer to the exit status of the just-preceding
"if" statement; i.e., $? can be 0 even when /var/tmp/node-augtool
doesn't exist.

This is a good argument for avoiding the following paradigm altogether:

   run_command
   if test $? = 0; then      # BAD: separate line/stmt, might be separated

Instead, write is like this:

   run_command && stmt-if-successful # GOOD: harder to separate accidentally

or if the body is longer:

   run_command && {          # GOOD
     stmts-if-successful
     ...
     }

These latter two forms make it harder to accidentally separate
the command and the test for its success.

Alternatively, save the exit status in a variable
and test *that*, so it's ok to separate them:

   run_command; st=$?
   if test $st = 0; then # GOOD: now it's ok to separate set and test stmts