diff mbox

[ovs-dev] ovs-vsctl: Print error when add-port fails.

Message ID 20161221024924.26635-1-diproiettod@vmware.com
State Accepted
Headers show

Commit Message

Daniele Di Proietto Dec. 21, 2016, 2:49 a.m. UTC
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>
---
 utilities/ovs-vsctl.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Ben Pfaff Dec. 21, 2016, 5:37 a.m. UTC | #1
On Tue, Dec 20, 2016 at 06:49:24PM -0800, Daniele Di Proietto wrote:
> 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>

I have another idea for improving the output here.  Quite often, when
someone posts an error, they haven't looked in the log, even though they
quoted the error message recommending that.  Then, when I ask them to
look in the log, the next question is always "where's the log?"  So,
what if we added something like "The default log directory is %s.",
where %s is ovs_logdir(), to the error message?  Perhaps it will help at
least one person find the log.

Acked-by: Ben Pfaff <blp@ovn.org>
Daniele Di Proietto Dec. 21, 2016, 7:21 p.m. UTC | #2
On 20/12/2016 21:37, "Ben Pfaff" <blp@ovn.org> wrote:

>On Tue, Dec 20, 2016 at 06:49:24PM -0800, Daniele Di Proietto wrote:
>> 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>
>
>I have another idea for improving the output here.  Quite often, when
>someone posts an error, they haven't looked in the log, even though they
>quoted the error message recommending that.  Then, when I ask them to
>look in the log, the next question is always "where's the log?"  So,
>what if we added something like "The default log directory is %s.",
>where %s is ovs_logdir(), to the error message?  Perhaps it will help at
>least one person find the log.

Good idea, I've added that

>
>Acked-by: Ben Pfaff <blp@ovn.org>

Thanks, I pushed this to master
diff mbox

Patch

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index a050840..14113b9 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -721,6 +721,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 +2377,6 @@  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;
     size_t i;
 
     for (i = 0; i < n_neoteric_ifaces; i++) {
@@ -2389,18 +2389,18 @@  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);
+                }
             }
         }
     }
-
-    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));
-    }
-
-    ds_destroy(&dead_ifaces);
 }