ovs-vsctl: Print error when add-port fails.

When the add-port command fails, vsctl reports the failure and just
suggests to check the logs for more details.

ovs-vswitchd fills the error column in the Interface table with a
description of the error, so it might be helpful to print that.

This is useful especially for dpdk devices, because the port naming
change could use a better error reporting.

I'm planning another patch to make sure that ovs-vswitch writes
appropriates information in the error column, after the dpdk port naming
changes are merged.

CC: Ben Pfaff <blp@ovn.org>
Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
Acked-by: Ben Pfaff <blp@ovn.org>

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index 9737589c77b1e5bc5d40612097a31dd064478426..87ee967150029ab6807acfcbd9bd0ead224ee5de 100644 (file)
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -1238,9 +1238,11 @@ m4_foreach(
 [vxlan_sys]],
 [
 # Try creating the port
-AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [dnl
+cat >experr <<EOF
 ovs-vsctl: Error detected while setting up 'reserved_name'.  See ovs-vswitchd log for details.
-])
+ovs-vsctl: The default log directory is "$OVS_RUNDIR".
+EOF
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr])
 # Prevent race.
 OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1])
 # Detect the warning log message
@@ -1274,9 +1276,11 @@ m4_foreach(
 [vxlan_sys]],
 [
 # Try creating the port
-AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [dnl
+cat >experr <<EOF
 ovs-vsctl: Error detected while setting up 'reserved_name'.  See ovs-vswitchd log for details.
-])
+ovs-vsctl: The default log directory is "$OVS_RUNDIR".
+EOF
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr])
 # Prevent race.
 OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1])
 # Detect the warning log message

diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index b7c5a1747a7eb98262cf8c19a5d6d1a38a344730..84545305b759b74831557b632c619d3db4b560b5 100644 (file)
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -215,9 +215,12 @@ AT_CHECK([test -$S a.mgmt])
 # Create a bridge with an unsafe name and make sure that the management
 # socket does not get created.
 mkdir b
-AT_CHECK([ovs-vsctl add-br b/c -- set bridge b/c datapath-type=dummy], [0],
-  [], [ovs-vsctl: Error detected while setting up 'b/c'.  See ovs-vswitchd log for details.
-])
+
+cat >experr <<EOF
+ovs-vsctl: Error detected while setting up 'b/c'.  See ovs-vswitchd log for details.
+ovs-vsctl: The default log directory is "$OVS_RUNDIR".
+EOF
+AT_CHECK([ovs-vsctl add-br b/c -- set bridge b/c datapath-type=dummy], [0], [], [experr])
 AT_CHECK([test ! -e b/c.mgmt])
 
 OVS_VSWITCHD_STOP(['/ignoring bridge with invalid name/d'])

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index a050840160eb748bb380071449448f92e1108f8b..41fa85273c241f935d9dc865f34c6f4e268ac4a4 100644 (file)
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -31,6 +31,7 @@
 
 #include "command-line.h"
 #include "compiler.h"
+#include "dirs.h"
 #include "openvswitch/dynamic-string.h"
 #include "fatal-signal.h"
 #include "hash.h"
@@ -721,6 +722,7 @@ pre_get_info(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &ovsrec_interface_col_name);
 
     ovsdb_idl_add_column(ctx->idl, &ovsrec_interface_col_ofport);
+    ovsdb_idl_add_column(ctx->idl, &ovsrec_interface_col_error);
 }
 
 static void
@@ -2376,7 +2378,7 @@ post_db_reload_expect_iface(const struct ovsrec_interface *iface)
 static void
 post_db_reload_do_checks(const struct vsctl_context *vsctl_ctx)
 {
-    struct ds dead_ifaces = DS_EMPTY_INITIALIZER;
+    bool print_error = false;
     size_t i;
 
     for (i = 0; i < n_neoteric_ifaces; i++) {
@@ -2389,18 +2391,23 @@ post_db_reload_do_checks(const struct vsctl_context *vsctl_ctx)
 
             iface = ovsrec_interface_get_for_uuid(vsctl_ctx->base.idl, uuid);
             if (iface && (!iface->ofport || *iface->ofport == -1)) {
-                ds_put_format(&dead_ifaces, "'%s', ", iface->name);
+                if (iface->error && *iface->error) {
+                    ovs_error(0, "Error detected while setting up '%s': %s.  "
+                                 "See ovs-vswitchd log for details.",
+                              iface->name, iface->error);
+                } else {
+                    ovs_error(0, "Error detected while setting up '%s'.  "
+                                 "See ovs-vswitchd log for details.",
+                              iface->name);
+                }
+                print_error = true;
             }
         }
     }
 
-    if (dead_ifaces.length) {
-        dead_ifaces.length -= 2; /* Strip off trailing comma and space. */
-        ovs_error(0, "Error detected while setting up %s.  See ovs-vswitchd "
-                  "log for details.", ds_cstr(&dead_ifaces));
+    if (print_error) {
+        ovs_error(0, "The default log directory is \"%s\".", ovs_logdir());
     }
-
-    ds_destroy(&dead_ifaces);
 }
 
 \f