[ovs-dev] [PATCH v2 6/6] ofp-actions: color output of flow actions for ovs-ofctl dump-flows

2016-02-15 Thread Quentin Monnet
Add color output for flow actions for ovs-ofctl dump-flows command
utility, by calling ds_put_color() instead of ds_put_format() in the
functions responsible for printing the actions.

This also causes the option to be propagated upward to the other callers
of those functions (partially implemented, to be completed if colors are
to be provided for other commands / tools).

Signed-off-by: Quentin Monnet 
---
 lib/bundle.c |  19 +-
 lib/bundle.h |   3 +-
 lib/learn.c  |  59 --
 lib/learn.h  |   3 +-
 lib/multipath.c  |   9 +-
 lib/multipath.h  |   3 +-
 lib/nx-match.c   |  18 +-
 lib/nx-match.h   |   9 +-
 lib/ofp-actions.c| 428 ---
 lib/ofp-actions.h|   3 +-
 lib/ofp-print.c  |  10 +-
 ofproto/ofproto-dpif-xlate.c |   2 +-
 ofproto/ofproto-dpif.c   |   2 +-
 ofproto/ofproto.c|   2 +-
 ovn/controller/ofctrl.c  |   2 +-
 tests/test-ovn.c |   2 +-
 utilities/ovs-ofctl.c|   4 +-
 17 files changed, 381 insertions(+), 197 deletions(-)

diff --git a/lib/bundle.c b/lib/bundle.c
index 871a724e8a67..bfe6c861c9c2 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 
+#include "colors.h"
 #include "dynamic-string.h"
 #include "multipath.h"
 #include "meta-flow.h"
@@ -279,7 +280,8 @@ bundle_parse_load(const char *s, struct ofpbuf *ofpacts)
 
 /* Appends a human-readable representation of 'nab' to 's'. */
 void
-bundle_format(const struct ofpact_bundle *bundle, struct ds *s)
+bundle_format(const struct ofpact_bundle *bundle, struct ds *s,
+  int const color_option)
 {
 const char *action, *fields, *algorithm;
 size_t i;
@@ -299,22 +301,27 @@ bundle_format(const struct ofpact_bundle *bundle, struct 
ds *s)
 
 action = bundle->dst.field ? "bundle_load" : "bundle";
 
-ds_put_format(s, "%s(%s,%"PRIu16",%s,%s,", action, fields,
+ds_put_color_start(s, paren_color, color_option);
+ds_put_format(s, "%s(", action);
+ds_put_color_end(s, color_option);
+ds_put_format(s, "%s,%"PRIu16",%s,%s,", fields,
   bundle->basis, algorithm, "ofport");
 
 if (bundle->dst.field) {
 mf_format_subfield(&bundle->dst, s);
-ds_put_cstr(s, ",");
+ds_put_char(s, ',');
 }
 
-ds_put_cstr(s, "slaves:");
+ds_put_color(s, "slaves:", param_color, color_option);
 for (i = 0; i < bundle->n_slaves; i++) {
 if (i) {
-ds_put_cstr(s, ",");
+ds_put_char(s, ',');
 }
 
 ofputil_format_port(bundle->slaves[i], s);
 }
 
-ds_put_cstr(s, ")");
+ds_put_color_start(s, paren_color, color_option);
+ds_put_char(s, ')');
+ds_put_color_end(s, color_option);
 }
diff --git a/lib/bundle.h b/lib/bundle.h
index a1f0ef2abb69..34d4bc2e6e20 100644
--- a/lib/bundle.h
+++ b/lib/bundle.h
@@ -47,6 +47,7 @@ enum ofperr bundle_check(const struct ofpact_bundle *, 
ofp_port_t max_ports,
 char *bundle_parse(const char *, struct ofpbuf *ofpacts) 
OVS_WARN_UNUSED_RESULT;
 char *bundle_parse_load(const char *, struct ofpbuf *ofpacts)
 OVS_WARN_UNUSED_RESULT;
-void bundle_format(const struct ofpact_bundle *, struct ds *);
+void bundle_format(const struct ofpact_bundle *, struct ds *,
+   int const color_option);
 
 #endif /* bundle.h */
diff --git a/lib/learn.c b/lib/learn.c
index 66201d5016f0..e88930f06ef4 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -19,6 +19,7 @@
 #include "learn.h"
 
 #include "byte-order.h"
+#include "colors.h"
 #include "dynamic-string.h"
 #include "match.h"
 #include "meta-flow.h"
@@ -408,37 +409,55 @@ learn_parse(char *arg, struct ofpbuf *ofpacts)
 /* Appends a description of 'learn' to 's', in the format that ovs-ofctl(8)
  * describes. */
 void
-learn_format(const struct ofpact_learn *learn, struct ds *s)
+learn_format(const struct ofpact_learn *learn, struct ds *s,
+ int const color_option)
 {
 const struct ofpact_learn_spec *spec;
 struct match match;
 
 match_init_catchall(&match);
 
-ds_put_format(s, "learn(table=%"PRIu8, learn->table_id);
+ds_put_color(s, "learn(", learn_color, color_option);
+ds_put_color(s, "table=", special_color, color_option);
+ds_put_format(s, "%"PRIu8, learn->table_id);
+
 if (learn->idle_timeout != OFP_FLOW_PERMANENT) {
-ds_put_format(s, ",idle_timeout=%"PRIu16, learn->idle_timeout);
+ds_put_char(s, ',');
+ds_put_color(s, "idle_timeout=", param_color, color_option);
+ds_put_format(s, "%"PRIu16, learn->idle_timeout);
 }
 if (learn->hard_timeout != OFP_FLOW_PERMANENT) {
-ds_put_format(s, ",hard_timeout=%"PRIu16, learn->hard_timeout);
+ds_put_char(s, ',');
+ds_put_color(s, "hard_timeout=", param_color, color_option);
+ds_put_format(s, "%"PRIu16, learn->hard_timeout);
 }
 if (

Re: [ovs-dev] [PATCH v2 6/6] ofp-actions: color output of flow actions for ovs-ofctl dump-flows

2016-02-23 Thread Ben Pfaff
On Mon, Feb 15, 2016 at 04:22:07PM +0100, Quentin Monnet wrote:
> Add color output for flow actions for ovs-ofctl dump-flows command
> utility, by calling ds_put_color() instead of ds_put_format() in the
> functions responsible for printing the actions.
> 
> This also causes the option to be propagated upward to the other callers
> of those functions (partially implemented, to be completed if colors are
> to be provided for other commands / tools).
> 
> Signed-off-by: Quentin Monnet 

At a skim, I'm OK with the rest of this series.  It doesn't apply at the
moment due to patch rejects, but that can be fixed.

I have a suggestion that might make the code easier to read.  I haven't
tried it so I'm not sure.

It's inconvenient to have to use functions to emit color markers, either
by calling both ds_put_color_start() and ds_put_color_end() or by
calling ds_put_color().  However, these functions always emit a fixed
string for any given color_option and sgr_seq.  It seems like it would
be possible, then, to put all of these into a struct, like this:

struct colors {
/* Color codes for various situation.  Each of these is a fully formed
 * SGR start string for the appropriate color. */
char *actions;
char *drop;
char *learn;
char *param;
char *paren;
char *special;
char *value;

/* SGR end string. */
char *end;
};

Then it becomes easier to emit whatever colors ones want.  Just pass a
"struct colors" into the function that might want to emit color, and it
can go ahead and do things like this:

ds_put_format(string, " %scookie=%s0x%"PRIx64,
  colors->param, colors->end, ntohll(fs->cookie));

If the caller doesn't actually want color, then it can pass in a struct
colors whose members are all "".
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [PATCH v2 6/6] ofp-actions: color output of flow actions for ovs-ofctl dump-flows

2016-02-24 Thread Quentin Monnet
2016-02-24 1:01 GMT+01:00 Ben Pfaff :

> On Mon, Feb 15, 2016 at 04:22:07PM +0100, Quentin Monnet wrote:
> > Add color output for flow actions for ovs-ofctl dump-flows command
> > utility
> >
> > Signed-off-by: Quentin Monnet 
>
> At a skim, I'm OK with the rest of this series.  It doesn't apply at the
> moment due to patch rejects, but that can be fixed.
>
> I have a suggestion that might make the code easier to read.  I haven't
> tried it so I'm not sure.
>

Hi Ben,
Many thanks for your feedback. I am working to address the issues you
pointed out, and I will definitely try your last suggestion for passing a
struct to the ds_put_format(). I will come back with a new version soon.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [PATCH v2 6/6] ofp-actions: color output of flow actions for ovs-ofctl dump-flows

2016-02-25 Thread Ben Pfaff
On Thu, Feb 25, 2016 at 08:44:46AM +0100, Quentin Monnet wrote:
> 2016-02-24 1:01 GMT+01:00 Ben Pfaff :
> 
> > On Mon, Feb 15, 2016 at 04:22:07PM +0100, Quentin Monnet wrote:
> > > Add color output for flow actions for ovs-ofctl dump-flows command
> > > utility
> > >
> > > Signed-off-by: Quentin Monnet 
> >
> > At a skim, I'm OK with the rest of this series.  It doesn't apply at the
> > moment due to patch rejects, but that can be fixed.
> >
> > I have a suggestion that might make the code easier to read.  I haven't
> > tried it so I'm not sure.
> >
> 
> Hi Ben,
> Many thanks for your feedback. I am working to address the issues you
> pointed out, and I will definitely try your last suggestion for passing a
> struct to the ds_put_format(). I will come back with a new version soon.

Great.

I think that lots of users will like this feature, so I'm glad that
you're working on it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev