[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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

   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

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.


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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]