2016-03-02 15:56 GMT+01:00 Quentin Monnet <quentin.mon...@6wind.com>:
> Proposal: add an option to ovs-ofctl utility so as to obtain colorized > output > in tty, for easier reading. Currently, only the dump-flows command supports > colors. > > A new `--color` option has been added to ovs-ofctl so as to indicate > whether > color markers should be used or not. It can be set to `always` (force > colors), > `never` (no colors) or `auto` (use colors only if output is a tty). If > provided > without any value, it is the same as `auto`. If the option is not provided > at > all, colors are disabled by default. > > An example screenshot of colored output is available at the following > address: > https://github.com/6WIND/ovs/blob/colors/README-colors.md > > I ran the test suite (`make check`) and got 1819 tests successful, 210 > skipped > (none failed). Logs are available at the following address: > https://github.com/6WIND/ovs/blob/colors/testsuite.log > > v2: Instead of using only hardcoded colors, it is now possible to define > custom > colors by using the `OVS_COLORS` environment variable (on the ls or grep > tools > model). See patch 2/6 (“ovs-ofctl: declare / set up colors for command > output”) > for more details. This version also splits the two patches of v1 into > several > smaller patches so as to ease comprehension and review. > > v3: This version addresses (nearly) all issues pointed out by Ben Pfaff in > his > review of the v2 (2016-02-24). Namely: > * ovs-ofctl manpage has been updated. > * NEWS file has been updated. > * Value for the `enable_color` variable is now a bool (was an int). > * No more sparse warnings on lib/colors.{c,h}. > * Better adherence to coding style. > * Colors are now held in a global struct. When color is not needed, this > struct > contains an empty string. In this way, passing the enable_color option > to all > printing functions is no longer required. > > Maybe not fixed: > * I did not see the clang or gcc warnings mentioned by Ben (on review of > patch > #2). Anyway this part of the code has been modified, and there are no > more > static strings for the compiler to complain about. But please tell me if > you > see warnings again. > > Not fixed: > * I have not squashed the first patch with another one yet, so there is > still > the issue of the `--color` option that is unused in the first two patches > (meanwhile, it makes reviewing the code easier). Should I squash the > first > three patches? > > Quentin Monnet (7): > ovs-ofctl: add option for color output to dump-flows command > ovs-ofctl: declare / set up colors for command output > ovs-ofctl: add output colors for flow attributes > match: color output of match conditions for ovs-ofctl dump-flows > ofp-actions: color output of flow actions for ovs-ofctl dump-flows > ovs-ofctl: update manpage for --color option > NEWS: update (--color option for ovs-ofctl) > > NEWS | 1 + > lib/automake.mk | 2 + > lib/bundle.c | 13 +-- > lib/colors.c | 146 ++++++++++++++++++++++++++++++ > lib/colors.h | 42 +++++++++ > lib/colors.man | 60 +++++++++++++ > lib/flow.c | 3 +- > lib/learn.c | 49 +++++++---- > lib/match.c | 126 ++++++++++++++------------ > lib/multipath.c | 9 +- > lib/nx-match.c | 9 +- > lib/ofp-actions.c | 224 > ++++++++++++++++++++++++++++------------------- > lib/ofp-print.c | 32 ++++--- > lib/util.c | 11 +++ > lib/util.h | 2 + > manpages.mk | 2 + > utilities/ovs-ofctl.8.in | 1 + > utilities/ovs-ofctl.c | 37 ++++++++ > 18 files changed, 581 insertions(+), 188 deletions(-) > create mode 100644 lib/colors.c > create mode 100644 lib/colors.h > create mode 100644 lib/colors.man > > -- > 1.9.1 > > Hi all, Ben, Do the fixes in v3 correctly address Ben's comments [1], or is there further work to do on this series? Best regards, Quentin [1]: http://openvswitch.org/pipermail/dev/2016-February/#66654 and following _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
