[ovs-dev] [PATCH v3 0/7] Add color output to `ovs-ofctl dump-flows` command

Quentin Monnet quentin.monnet at 6wind.com
Mon Mar 14 16:17:15 UTC 2016


2016-03-02 15:56 GMT+01:00 Quentin Monnet <quentin.monnet at 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



More information about the dev mailing list