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

[Perry N. Myers]

Jim Meyering wrote:
> 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

Did some refactoring of the error handling for this function as well as 
some better debug output printing.  (see new patch in this thread)

There is still a problem with the Ruby controller however that Darryl will 
need to fix.  If there is no config to retrieve (because the hostname is 
not in the database yet... this occurs on the first boot of a node) the 
Ruby controller returns an error page but the HTTP status code is still 
200.  We should be returning a true error page here with an error status 
code so that wget returns a non-zero value.  Right now what happens is 
wget returns 0 and we try to the returned script which fails since it's 
not a proper bash script.  Things work fine since the check for ifcfg-eth 
files handles it and we still get the default net interfaces.

Perry

-- 
|=-        Red Hat, Engineering, Emerging Technologies, Boston        -=|
|=-                     Email: pmyers at redhat.com                      -=|
|=-         Office: +1 412 474 3552   Mobile: +1 703 362 9622         -=|
|=- GnuPG: E65E4F3D 88F9 F1C9 C2F3 1303 01FE 817C C5D2 8B91 E65E 4F3D -=|