diff mbox series

[ovs-dev,V2] lib: Make error messages more useful

Message ID 1514497961-31058-1-git-send-email-gvrose8192@gmail.com
State Changes Requested
Headers show
Series [ovs-dev,V2] lib: Make error messages more useful | expand

Commit Message

Gregory Rose Dec. 28, 2017, 9:52 p.m. UTC 
There are many "opening datapath" error messages and when one occurs
it is impossible to know just from the log message which of the
"opening datapath" errors occurred.  Add a helper function that
incorporates the calling function's name to help identify where
the datapath open error occurred.

Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
 lib/dpctl.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

Comments

Ben Pfaff Jan. 2, 2018, 7:52 p.m. UTC | #1 
On Thu, Dec 28, 2017 at 01:52:41PM -0800, Greg Rose wrote:
> There are many "opening datapath" error messages and when one occurs
> it is impossible to know just from the log message which of the
> "opening datapath" errors occurred.  Add a helper function that
> incorporates the calling function's name to help identify where
> the datapath open error occurred.
> 
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>

Thanks for working on improving OVS error messages.

Users don't want to see the details of OVS code, whether those details
are line numbers or function names, except maybe when they're at wits'
end anyway (e.g. assertion failures, debug-level log messages).  To
better distinguish error messages, we should provide something
meaningful to the user.  That could be the name of a dpctl or unixctl
command, or some other indication of why the code was opening the
datapath (e.g. "to add an interface").

The helper function I was envisioning would have included the initial
call to parsed_dpif_open(), since that's such a common step before
reporting an error.

Thanks,

Ben.
Gregory Rose Jan. 2, 2018, 9:15 p.m. UTC | #2 
On 1/2/2018 11:52 AM, Ben Pfaff wrote:
> On Thu, Dec 28, 2017 at 01:52:41PM -0800, Greg Rose wrote:
>> There are many "opening datapath" error messages and when one occurs
>> it is impossible to know just from the log message which of the
>> "opening datapath" errors occurred.  Add a helper function that
>> incorporates the calling function's name to help identify where
>> the datapath open error occurred.
>>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> Thanks for working on improving OVS error messages.
>
> Users don't want to see the details of OVS code, whether those details
> are line numbers or function names, except maybe when they're at wits'
> end anyway (e.g. assertion failures, debug-level log messages).  To
> better distinguish error messages, we should provide something
> meaningful to the user.  That could be the name of a dpctl or unixctl
> command, or some other indication of why the code was opening the
> datapath (e.g. "to add an interface").

Hmm.  Yeah, that's why I was including the datapath name in the original 
patch.

>
> The helper function I was envisioning would have included the initial
> call to parsed_dpif_open(), since that's such a common step before
> reporting an error.

OK, if you feel it is still worth pursuing then I'll use the approach 
you suggest.

Thanks,

- Greg

>
> Thanks,
>
> Ben.
diff mbox series

Patch

diff --git a/lib/dpctl.c b/lib/dpctl.c
index b769544..a0c639e 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -120,6 +120,14 @@  dpctl_error(struct dpctl_params* dpctl_p, int err_no, const char *fmt, ...)
 
     errno = save_errno;
 }
+
+static void
+dpctl_error_opening_dp(struct dpctl_params *dpctl_p, int err_no,
+                       const char *func_name)
+{
+    dpctl_error(dpctl_p, err_no, "%s: opening datapath", func_name);
+}
+
 
 static int dpctl_add_if(int argc, const char *argv[], struct dpctl_params *);
 
@@ -214,7 +222,7 @@  dpctl_del_dp(int argc OVS_UNUSED, const char *argv[],
 
     error = parsed_dpif_open(argv[1], false, &dpif);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
     error = dpif_delete(dpif);
@@ -235,7 +243,7 @@  dpctl_add_if(int argc OVS_UNUSED, const char *argv[],
 
     error = parsed_dpif_open(argv[1], false, &dpif);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
     for (i = 2; i < argc; i++) {
@@ -324,7 +332,7 @@  dpctl_set_if(int argc, const char *argv[], struct dpctl_params *dpctl_p)
 
     error = parsed_dpif_open(argv[1], false, &dpif);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
     for (i = 2; i < argc; i++) {
@@ -459,7 +467,7 @@  dpctl_del_if(int argc, const char *argv[], struct dpctl_params *dpctl_p)
 
     error = parsed_dpif_open(argv[1], false, &dpif);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
     for (i = 2; i < argc; i++) {
@@ -838,7 +846,7 @@  dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
     error = parsed_dpif_open(name, false, &dpif);
     free(name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         goto out_free;
     }
 
@@ -974,7 +982,7 @@  dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
     error = parsed_dpif_open(dp_name, false, &dpif);
     free(dp_name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
 
@@ -1086,7 +1094,7 @@  dpctl_get_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
     error = parsed_dpif_open(dp_name, false, &dpif);
     free(dp_name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
 
@@ -1145,7 +1153,7 @@  dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
     error = parsed_dpif_open(dp_name, false, &dpif);
     free(dp_name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
 
@@ -1225,7 +1233,7 @@  dpctl_del_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
     error = parsed_dpif_open(name, false, &dpif);
     free(name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
 
@@ -1294,7 +1302,7 @@  dpctl_dump_conntrack(int argc, const char *argv[],
     error = parsed_dpif_open(name, false, &dpif);
     free(name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
 
@@ -1360,7 +1368,7 @@  dpctl_flush_conntrack(int argc, const char *argv[],
         error = parsed_dpif_open(name, false, &dpif);
         free(name);
         if (error) {
-            dpctl_error(dpctl_p, error, "opening datapath");
+            dpctl_error_opening_dp(dpctl_p, error, __func__);
             return error;
         }
     }
@@ -1444,7 +1452,7 @@  dpctl_ct_stats_show(int argc, const char *argv[],
     error = parsed_dpif_open(name, false, &dpif);
     free(name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }
 
@@ -1581,7 +1589,7 @@  dpctl_ct_bkts(int argc, const char *argv[],
     error = parsed_dpif_open(name, false, &dpif);
     free(name);
     if (error) {
-        dpctl_error(dpctl_p, error, "opening datapath");
+        dpctl_error_opening_dp(dpctl_p, error, __func__);
         return error;
     }