[ovs-dev] [PATCH] utilities: ovs-lib: Signal start_daemon failures

[Aaron Conole]

Markos Chandras <mchandras at suse.de> writes:

> Make sure we communicate failures to the caller when start_daemon fails
> to start a process as the caller may not be able to proceed after this.
>
> Signed-off-by: Markos Chandras <mchandras at suse.de>
> ---
>  utilities/ovs-lib.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
> index 773efb3..6fd861e 100644
> --- a/utilities/ovs-lib.in
> +++ b/utilities/ovs-lib.in
> @@ -190,7 +190,7 @@ start_daemon () {
>          set nice -n "$priority" "$@"
>      fi
>  
> -    action "Starting $daemon" "$@"
> +    action "Starting $daemon" "$@" || return 1
>  
>      if test X"$strace" != X; then
>          # Strace doesn't have the -D option so we attach after the fact.

I agree with this change, but I think it needs to be extended all the
way (so, that means into ovs-ctl as well).  Currently, there's no
difference in the script return value when I have your patch applied or
not (ie: ovs-ctl --no-ovsdb-server start and then echo $? yields 0 when
I have renamed ovs-vswitchd to cause failure).

Is it possible for you to post a follow-up patch for ovs-ctl.in that looks
something like below (NOTE: not tested at all - caveat emptor)?

diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index dc275f8..562b86d 100755
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -190,8 +190,9 @@ do_start_ovsdb () {
 
 start_ovsdb() {
     if test X"$OVSDB_SERVER" = Xyes; then
-        do_start_ovsdb
+        return do_start_ovsdb
     fi
+    return 0
 }
 
 add_managers () {
@@ -238,14 +239,16 @@ do_start_forwarding () {
         if test X"$SELF_CONFINEMENT" = Xno; then
             set "$@" --no-self-confinement
         fi
-        start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" "$@"
+        start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" "$@" ||
+            return 1
     fi
 }
 
 start_forwarding () {
     if test X"$OVS_VSWITCHD" = Xyes; then
-        do_start_forwarding
+        return do_start_forwarding
     fi
+    return 0
 }
 
 ## ---- ##
@@ -364,7 +367,7 @@ force_reload_kmod () {
     # Restart the database first, since a large database may take a
     # while to load, and we want to minimize forwarding disruption.
     stop_ovsdb
-    start_ovsdb
+    start_ovsdb || return 1
 
     stop_forwarding
 
@@ -395,7 +398,7 @@ force_reload_kmod () {
 
     # Start vswitchd by asking it to wait till flow restore is finished.
     flow_restore_wait
-    start_forwarding
+    start_forwarding || return 1
 
     # Restore saved flows and inform vswitchd that we are done.
     restore_flows
@@ -422,13 +425,13 @@ restart () {
     # Restart the database first, since a large database may take a
     # while to load, and we want to minimize forwarding disruption.
     stop_ovsdb
-    start_ovsdb
+    start_ovsdb || return 1
 
     stop_forwarding
 
     # Start vswitchd by asking it to wait till flow restore is finished.
     flow_restore_wait
-    start_forwarding
+    start_forwarding || return 1
 
     # Restore saved flows and inform vswitchd that we are done.
     restore_flows
@@ -686,7 +689,7 @@ done
 case $command in
     start)
         start_ovsdb || exit 1
-        start_forwarding
+        start_forwarding || exit 1
         add_managers
         ;;
     stop)