[ovs-dev,v7,3/6] Migrate commands to extended unixctl API.

From: Jakob Meng <code@jakobmeng.de>

Previous commits introduced support different output formats to
ovs-xxx tools and its Python equivalents. However, the commands
were not yet migrated to the updated {unixctl_}command_register()
functions and unixctl_cb_func function type in order to highlight
the API changes only.

This patch accomplishes this command migration in a single sweep.
It replaces the old functions {unixctl_}command_register() in
lib/unixctl.* and python/ovs/unixctl/__init__.py with their
extended variants featuring the new 'output_fmts' parameter.
All command registrations have been updated to announce what output
formats each commands supports. No new output formats have been
added, i.e. all commands still support OVS_OUTPUT_FMT_TEXT only.

All command callbacks gained a 'enum ovs_output_fmt fmt OVS_UNUSED'
argument to conform with the updated function type unixctl_cb_func
in lib/unixctl.h. Without any new output formats being added, it is
always ignored for now.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng <code@jakobmeng.de>
---
 ipsec/ovs-monitor-ipsec.in     |  22 ++++--
 lib/bfd.c                      |  16 +++-
 lib/cfm.c                      |  13 +++-
 lib/coverage.c                 |  11 ++-
 lib/dpctl.c                    | 127 +++++++++++++++++++-------------
 lib/dpdk.c                     |  15 ++--
 lib/dpif-netdev-perf.c         |   1 +
 lib/dpif-netdev-perf.h         |   1 +
 lib/dpif-netdev.c              |  84 ++++++++++++---------
 lib/dpif-netlink.c             |   6 +-
 lib/lacp.c                     |   7 +-
 lib/memory.c                   |   5 +-
 lib/netdev-dpdk.c              |  16 +++-
 lib/netdev-dummy.c             |  30 +++++---
 lib/netdev-native-tnl.c        |   4 +-
 lib/netdev-native-tnl.h        |   4 +-
 lib/netdev-vport.c             |   1 +
 lib/odp-execute.c              |  10 ++-
 lib/ovs-lldp.c                 |  16 ++--
 lib/ovs-router.c               |  26 ++++---
 lib/rstp.c                     |  22 +++---
 lib/stopwatch.c                |  10 ++-
 lib/stp.c                      |  20 ++---
 lib/timeval.c                  |   9 ++-
 lib/tnl-neigh-cache.c          |  35 +++++----
 lib/tnl-ports.c                |   6 +-
 lib/unixctl.c                  |  53 ++++----------
 lib/unixctl.h                  |  12 +--
 lib/vlog.c                     |  46 ++++++++----
 ofproto/bond.c                 |  32 +++++---
 ofproto/ofproto-dpif-trace.c   |  10 ++-
 ofproto/ofproto-dpif-upcall.c  |  81 +++++++++++++++------
 ofproto/ofproto-dpif.c         |  64 +++++++++++-----
 ofproto/ofproto.c              |  10 ++-
 ofproto/tunnel.c               |   4 +-
 ovsdb/file.c                   |   4 +-
 ovsdb/ovsdb-client.c           |  40 ++++++----
 ovsdb/ovsdb-server.c           | 129 ++++++++++++++++++++++++---------
 ovsdb/ovsdb.c                  |   4 +-
 ovsdb/raft.c                   |  28 ++++---
 python/ovs/unixctl/__init__.py |  40 ++--------
 python/ovs/unixctl/server.py   |  34 ++++-----
 python/ovs/vlog.py             |  12 ++-
 tests/test-netflow.c           |   4 +-
 tests/test-sflow.c             |   4 +-
 tests/test-unixctl.c           |  33 ++++++---
 tests/test-unixctl.py          |  25 ++++---
 utilities/ovs-ofctl.c          |  43 +++++++----
 vswitchd/bridge.c              |  25 +++++--
 vswitchd/ovs-vswitchd.c        |   5 +-
 vtep/ovs-vtep.in               |   5 +-
 51 files changed, 790 insertions(+), 474 deletions(-)

On Thu, Jan 18, 2024 at 04:26:54PM +0100, jmeng@redhat.com wrote:
> From: Jakob Meng <code@jakobmeng.de>
> 
> Previous commits introduced support different output formats to
> ovs-xxx tools and its Python equivalents. However, the commands
> were not yet migrated to the updated {unixctl_}command_register()
> functions and unixctl_cb_func function type in order to highlight
> the API changes only.
> 
> This patch accomplishes this command migration in a single sweep.
> It replaces the old functions {unixctl_}command_register() in
> lib/unixctl.* and python/ovs/unixctl/__init__.py with their
> extended variants featuring the new 'output_fmts' parameter.
> All command registrations have been updated to announce what output
> formats each commands supports. No new output formats have been
> added, i.e. all commands still support OVS_OUTPUT_FMT_TEXT only.
> 
> All command callbacks gained a 'enum ovs_output_fmt fmt OVS_UNUSED'
> argument to conform with the updated function type unixctl_cb_func
> in lib/unixctl.h. Without any new output formats being added, it is
> always ignored for now.
> 
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <code@jakobmeng.de>

Recheck-request: github-robot

On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:

> From: Jakob Meng <code@jakobmeng.de>
>
> Previous commits introduced support different output formats to
> ovs-xxx tools and its Python equivalents. However, the commands
> were not yet migrated to the updated {unixctl_}command_register()
> functions and unixctl_cb_func function type in order to highlight
> the API changes only.
>
> This patch accomplishes this command migration in a single sweep.
> It replaces the old functions {unixctl_}command_register() in
> lib/unixctl.* and python/ovs/unixctl/__init__.py with their
> extended variants featuring the new 'output_fmts' parameter.
> All command registrations have been updated to announce what output
> formats each commands supports. No new output formats have been
> added, i.e. all commands still support OVS_OUTPUT_FMT_TEXT only.
>
> All command callbacks gained a 'enum ovs_output_fmt fmt OVS_UNUSED'
> argument to conform with the updated function type unixctl_cb_func
> in lib/unixctl.h. Without any new output formats being added, it is
> always ignored for now.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <code@jakobmeng.de>

Thank for the patch! What a beast to go trough ;)

I believe the current approach is acceptable. However, we could also
incorporate union callbacks: if registration only supports text, we would
use callback A, and if multiple formats exist, we could employ the new style
callback. This would mitigate the need for a significant overhaul. Just a
suggestion, as I'm ok with the current approach. :)

Some small style comments highlighted below.

> ---
>  ipsec/ovs-monitor-ipsec.in     |  22 ++++--
>  lib/bfd.c                      |  16 +++-
>  lib/cfm.c                      |  13 +++-
>  lib/coverage.c                 |  11 ++-
>  lib/dpctl.c                    | 127 +++++++++++++++++++-------------
>  lib/dpdk.c                     |  15 ++--
>  lib/dpif-netdev-perf.c         |   1 +
>  lib/dpif-netdev-perf.h         |   1 +
>  lib/dpif-netdev.c              |  84 ++++++++++++---------
>  lib/dpif-netlink.c             |   6 +-
>  lib/lacp.c                     |   7 +-
>  lib/memory.c                   |   5 +-
>  lib/netdev-dpdk.c              |  16 +++-
>  lib/netdev-dummy.c             |  30 +++++---
>  lib/netdev-native-tnl.c        |   4 +-
>  lib/netdev-native-tnl.h        |   4 +-
>  lib/netdev-vport.c             |   1 +
>  lib/odp-execute.c              |  10 ++-
>  lib/ovs-lldp.c                 |  16 ++--
>  lib/ovs-router.c               |  26 ++++---
>  lib/rstp.c                     |  22 +++---
>  lib/stopwatch.c                |  10 ++-
>  lib/stp.c                      |  20 ++---
>  lib/timeval.c                  |   9 ++-
>  lib/tnl-neigh-cache.c          |  35 +++++----
>  lib/tnl-ports.c                |   6 +-
>  lib/unixctl.c                  |  53 ++++----------
>  lib/unixctl.h                  |  12 +--
>  lib/vlog.c                     |  46 ++++++++----
>  ofproto/bond.c                 |  32 +++++---
>  ofproto/ofproto-dpif-trace.c   |  10 ++-
>  ofproto/ofproto-dpif-upcall.c  |  81 +++++++++++++++------
>  ofproto/ofproto-dpif.c         |  64 +++++++++++-----
>  ofproto/ofproto.c              |  10 ++-
>  ofproto/tunnel.c               |   4 +-
>  ovsdb/file.c                   |   4 +-
>  ovsdb/ovsdb-client.c           |  40 ++++++----
>  ovsdb/ovsdb-server.c           | 129 ++++++++++++++++++++++++---------
>  ovsdb/ovsdb.c                  |   4 +-
>  ovsdb/raft.c                   |  28 ++++---
>  python/ovs/unixctl/__init__.py |  40 ++--------
>  python/ovs/unixctl/server.py   |  34 ++++-----
>  python/ovs/vlog.py             |  12 ++-
>  tests/test-netflow.c           |   4 +-
>  tests/test-sflow.c             |   4 +-
>  tests/test-unixctl.c           |  33 ++++++---
>  tests/test-unixctl.py          |  25 ++++---
>  utilities/ovs-ofctl.c          |  43 +++++++----
>  vswitchd/bridge.c              |  25 +++++--
>  vswitchd/ovs-vswitchd.c        |   5 +-
>  vtep/ovs-vtep.in               |   5 +-
>  51 files changed, 790 insertions(+), 474 deletions(-)
>
> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in
> index 7945162f9..29c87b32a 100755
> --- a/ipsec/ovs-monitor-ipsec.in
> +++ b/ipsec/ovs-monitor-ipsec.in
> @@ -1229,25 +1229,25 @@ def address_family(address):
>      return "IPv4"
>
>
> -def unixctl_xfrm_policies(conn, unused_argv, unused_aux):
> +def unixctl_xfrm_policies(conn, unused_argv, unused_fmt, unused_aux):
>      global xfrm
>      policies = xfrm.get_policies()
>      conn.reply(str(policies))
>
>
> -def unixctl_xfrm_state(conn, unused_argv, unused_aux):
> +def unixctl_xfrm_state(conn, unused_argv, unused_fmt, unused_aux):
>      global xfrm
>      securities = xfrm.get_securities()
>      conn.reply(str(securities))
>
>
> -def unixctl_ipsec_status(conn, unused_argv, unused_aux):
> +def unixctl_ipsec_status(conn, unused_argv, unused_fmt, unused_aux):
>      global monitor
>      conns = monitor.ike_helper.get_active_conns()
>      conn.reply(str(conns))
>
>
> -def unixctl_show(conn, unused_argv, unused_aux):
> +def unixctl_show(conn, unused_argv, unused_fmt, unused_aux):
>      global monitor
>      global xfrm
>      policies = xfrm.get_policies()
> @@ -1255,13 +1255,13 @@ def unixctl_show(conn, unused_argv, unused_aux):
>      monitor.show(conn, policies, securities)
>
>
> -def unixctl_refresh(conn, unused_argv, unused_aux):
> +def unixctl_refresh(conn, unused_argv, unused_fmt, unused_aux):
>      global monitor
>      monitor.ike_helper.refresh(monitor)
>      conn.reply(None)
>
>
> -def unixctl_exit(conn, argv, unused_aux):
> +def unixctl_exit(conn, argv, unused_fmt, unused_aux):
>      global monitor
>      global exiting
>      ret = None
> @@ -1336,17 +1336,25 @@ def main():
>
>      ovs.daemon.daemonize()
>      ovs.unixctl.command_register("list-commands", "", 0, 0,
> +                                 ovs.util.OutputFormat.TEXT,
>                                   ovs.unixctl._unixctl_help, None)
>      ovs.unixctl.command_register("xfrm/policies", "", 0, 0,
> +                                 ovs.util.OutputFormat.TEXT,
>                                   unixctl_xfrm_policies, None)
>      ovs.unixctl.command_register("xfrm/state", "", 0, 0,
> +                                 ovs.util.OutputFormat.TEXT,
>                                   unixctl_xfrm_state, None)
>      ovs.unixctl.command_register("ipsec/status", "", 0, 0,
> +                                 ovs.util.OutputFormat.TEXT,
>                                   unixctl_ipsec_status, None)
>      ovs.unixctl.command_register("tunnels/show", "", 0, 0,
> +                                 ovs.util.OutputFormat.TEXT,
>                                   unixctl_show, None)
> -    ovs.unixctl.command_register("refresh", "", 0, 0, unixctl_refresh, None)
> +    ovs.unixctl.command_register("refresh", "", 0, 0,
> +                                 ovs.util.OutputFormat.TEXT,
> +                                 unixctl_refresh, None)
>      ovs.unixctl.command_register("exit", "[--no-cleanup]", 0, 1,
> +                                 ovs.util.OutputFormat.TEXT,
>                                   unixctl_exit, None)
>
>      error, unixctl_server = ovs.unixctl.server.UnixctlServer.create(None)
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 9698576d0..05d150878 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -267,9 +267,13 @@ static void bfd_status_changed(struct bfd *) OVS_REQUIRES(mutex);
>
>  static void bfd_forwarding_if_rx_update(struct bfd *) OVS_REQUIRES(mutex);
>  static void bfd_unixctl_show(struct unixctl_conn *, int argc,
> -                             const char *argv[], void *aux OVS_UNUSED);
> +                             const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED,
> +                             void *aux OVS_UNUSED);
>  static void bfd_unixctl_set_forwarding_override(struct unixctl_conn *,
>                                                  int argc, const char *argv[],
> +                                                enum ovs_output_fmt fmt
> +                                                OVS_UNUSED,
>                                                  void *aux OVS_UNUSED);
>  static void log_msg(enum vlog_level, const struct msg *, const char *message,
>                      const struct bfd *) OVS_REQUIRES(mutex);
> @@ -339,9 +343,10 @@ void
>  bfd_init(void)
>  {
>      unixctl_command_register("bfd/show", "[interface]", 0, 1,
> -                             bfd_unixctl_show, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bfd_unixctl_show, NULL);
>      unixctl_command_register("bfd/set-forwarding",
>                               "[interface] normal|false|true", 1, 2,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               bfd_unixctl_set_forwarding_override, NULL);
>  }
>
> @@ -1311,7 +1316,8 @@ bfd_put_details(struct ds *ds, const struct bfd *bfd) OVS_REQUIRES(mutex)
>
>  static void
>  bfd_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
> -                 void *aux OVS_UNUSED) OVS_EXCLUDED(mutex)
> +                 enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
> +    OVS_EXCLUDED(mutex)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      struct bfd *bfd;
> @@ -1340,7 +1346,9 @@ out:
>
>  static void
>  bfd_unixctl_set_forwarding_override(struct unixctl_conn *conn, int argc,
> -                                    const char *argv[], void *aux OVS_UNUSED)
> +                                    const char *argv[],
> +                                    enum ovs_output_fmt fmt OVS_UNUSED,
> +                                    void *aux OVS_UNUSED)
>      OVS_EXCLUDED(mutex)
>  {
>      const char *forward_str = argv[argc - 1];
> diff --git a/lib/cfm.c b/lib/cfm.c
> index c3742f3de..4f1598983 100644
> --- a/lib/cfm.c
> +++ b/lib/cfm.c
> @@ -327,10 +327,12 @@ lookup_remote_mp(const struct cfm *cfm, uint64_t mpid) OVS_REQUIRES(mutex)
>  void
>  cfm_init(void)
>  {
> -    unixctl_command_register("cfm/show", "[interface]", 0, 1, cfm_unixctl_show,
> +    unixctl_command_register("cfm/show", "[interface]", 0, 1,
> +                             OVS_OUTPUT_FMT_TEXT, cfm_unixctl_show,
>                               NULL);
>      unixctl_command_register("cfm/set-fault", "[interface] normal|false|true",
> -                             1, 2, cfm_unixctl_set_fault, NULL);
> +                             1, 2, OVS_OUTPUT_FMT_TEXT, cfm_unixctl_set_fault,
> +                             NULL);
>  }
>
>  /* Records the status change and changes the global connectivity seq. */
> @@ -1061,7 +1063,8 @@ cfm_print_details(struct ds *ds, struct cfm *cfm) OVS_REQUIRES(mutex)
>
>  static void
>  cfm_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
> -                 void *aux OVS_UNUSED) OVS_EXCLUDED(mutex)
> +                 enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
> +    OVS_EXCLUDED(mutex)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      struct cfm *cfm;
> @@ -1088,7 +1091,9 @@ out:
>
>  static void
>  cfm_unixctl_set_fault(struct unixctl_conn *conn, int argc, const char *argv[],
> -                      void *aux OVS_UNUSED) OVS_EXCLUDED(mutex)
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
> +    OVS_EXCLUDED(mutex)
>  {
>      const char *fault_str = argv[argc - 1];
>      int fault_override;
> diff --git a/lib/coverage.c b/lib/coverage.c
> index a95b6aa25..f32550fc3 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -61,7 +61,9 @@ coverage_counter_register(struct coverage_counter* counter)
>
>  static void
>  coverage_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                      const char *argv[] OVS_UNUSED,
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
>  {
>      struct svec lines;
>      char *reply;
> @@ -76,7 +78,9 @@ coverage_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  coverage_unixctl_read_counter(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                              const char *argv[], void *aux OVS_UNUSED)
> +                              const char *argv[],
> +                              enum ovs_output_fmt fmt OVS_UNUSED,
> +                              void *aux OVS_UNUSED)
>  {
>      unsigned long long count;
>      char *reply;
> @@ -96,9 +100,10 @@ coverage_unixctl_read_counter(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  void
>  coverage_init(void)
>  {
> -    unixctl_command_register("coverage/show", "", 0, 0,
> +    unixctl_command_register("coverage/show", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                               coverage_unixctl_show, NULL);
>      unixctl_command_register("coverage/read-counter", "COUNTER", 1, 1,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               coverage_unixctl_read_counter, NULL);
>  }
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 34ee7d0e2..27aaa21ee 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -66,6 +66,7 @@ struct dpctl_command {
>      const char *usage;
>      int min_args;
>      int max_args;
> +    int output_fmts;
>      dpctl_command_handler *handler;
>      enum { DP_RO, DP_RW} mode;
>  };
> @@ -2983,70 +2984,94 @@ out:
>  }
>  
>  static const struct dpctl_command all_commands[] = {
> -    { "add-dp", "dp [iface...]", 1, INT_MAX, dpctl_add_dp, DP_RW },
> -    { "del-dp", "dp", 1, 1, dpctl_del_dp, DP_RW },
> -    { "add-if", "dp iface...", 2, INT_MAX, dpctl_add_if, DP_RW },
> -    { "del-if", "dp iface...", 2, INT_MAX, dpctl_del_if, DP_RW },
> -    { "set-if", "dp iface...", 2, INT_MAX, dpctl_set_if, DP_RW },
> -    { "dump-dps", "", 0, 0, dpctl_dump_dps, DP_RO },
> -    { "show", "[-s] [dp...]", 0, INT_MAX, dpctl_show, DP_RO },
> +    { "add-dp", "dp [iface...]", 1, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_add_dp, DP_RW },
> +    { "del-dp", "dp", 1, 1, OVS_OUTPUT_FMT_TEXT, dpctl_del_dp, DP_RW },
> +    { "add-if", "dp iface...", 2, INT_MAX, OVS_OUTPUT_FMT_TEXT, dpctl_add_if,
> +      DP_RW },
> +    { "del-if", "dp iface...", 2, INT_MAX, OVS_OUTPUT_FMT_TEXT, dpctl_del_if,
> +      DP_RW },
> +    { "set-if", "dp iface...", 2, INT_MAX, OVS_OUTPUT_FMT_TEXT, dpctl_set_if,
> +      DP_RW },
> +    { "dump-dps", "", 0, 0, OVS_OUTPUT_FMT_TEXT, dpctl_dump_dps, DP_RO },
> +    { "show", "[-s] [dp...]", 0, INT_MAX, OVS_OUTPUT_FMT_TEXT, dpctl_show,
> +      DP_RO },
>      { "dump-flows", "[-m] [--names] [dp] [filter=..] [type=..] [pmd=..]",
> -      0, 6, dpctl_dump_flows, DP_RO },
> -    { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW },
> -    { "mod-flow", "[dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW },
> -    { "get-flow", "[dp] ufid", 1, 2, dpctl_get_flow, DP_RO },
> -    { "del-flow", "[dp] flow", 1, 2, dpctl_del_flow, DP_RW },
> -    { "add-flows", "[dp] file", 1, 2, dpctl_process_flows, DP_RW },
> -    { "mod-flows", "[dp] file", 1, 2, dpctl_process_flows, DP_RW },
> -    { "del-flows", "[dp] [file]", 0, 2, dpctl_del_flows, DP_RW },
> +      0, 6, OVS_OUTPUT_FMT_TEXT, dpctl_dump_flows, DP_RO },
> +    { "add-flow", "[dp] flow actions", 2, 3, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_add_flow, DP_RW },
> +    { "mod-flow", "[dp] flow actions", 2, 3, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_mod_flow, DP_RW },
> +    { "get-flow", "[dp] ufid", 1, 2, OVS_OUTPUT_FMT_TEXT, dpctl_get_flow,
> +      DP_RO },
> +    { "del-flow", "[dp] flow", 1, 2, OVS_OUTPUT_FMT_TEXT, dpctl_del_flow,
> +      DP_RW },
> +    { "add-flows", "[dp] file", 1, 2, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_process_flows, DP_RW },
> +    { "mod-flows", "[dp] file", 1, 2, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_process_flows, DP_RW },
> +    { "del-flows", "[dp] [file]", 0, 2, OVS_OUTPUT_FMT_TEXT, dpctl_del_flows,
> +      DP_RW },
>      { "offload-stats-show", "[dp]",
> -      0, 1, dpctl_offload_stats_show, DP_RO },
> +      0, 1, OVS_OUTPUT_FMT_TEXT, dpctl_offload_stats_show, DP_RO },
>      { "dump-conntrack", "[-m] [-s] [dp] [zone=N]",
> -      0, 4, dpctl_dump_conntrack, DP_RO },
> +      0, 4, OVS_OUTPUT_FMT_TEXT, dpctl_dump_conntrack, DP_RO },
>      { "dump-conntrack-exp", "[dp] [zone=N]",
> -      0, 2, dpctl_dump_conntrack_exp, DP_RO },
> +      0, 2, OVS_OUTPUT_FMT_TEXT, dpctl_dump_conntrack_exp, DP_RO },
>      { "flush-conntrack", "[dp] [zone=N] [mark=X[/M]] [labels=Y[/N]] "
>                           "[ct-orig-tuple [ct-reply-tuple]]",
> -      0, 6, dpctl_flush_conntrack, DP_RW },
> -    { "cache-get-size", "[dp]", 0, 1, dpctl_cache_get_size, DP_RO },
> -    { "cache-set-size", "dp cache <size>", 3, 3, dpctl_cache_set_size, DP_RW },
> +      0, 6, OVS_OUTPUT_FMT_TEXT, dpctl_flush_conntrack, DP_RW },
> +    { "cache-get-size", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_cache_get_size, DP_RO },
> +    { "cache-set-size", "dp cache <size>", 3, 3, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_cache_set_size, DP_RW },
>      { "ct-stats-show", "[dp] [zone=N]",
> -      0, 3, dpctl_ct_stats_show, DP_RO },
> -    { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO },
> -    { "ct-set-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns,
> -       DP_RW },
> -    { "ct-get-maxconns", "[dp]", 0, 1, dpctl_ct_get_maxconns, DP_RO },
> -    { "ct-get-nconns", "[dp]", 0, 1, dpctl_ct_get_nconns, DP_RO },
> -    { "ct-enable-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_enable_tcp_seq_chk,
> -       DP_RW },
> -    { "ct-disable-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_disable_tcp_seq_chk,
> -       DP_RW },
> -    { "ct-get-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_get_tcp_seq_chk, DP_RO },
> +      0, 3, OVS_OUTPUT_FMT_TEXT, dpctl_ct_stats_show, DP_RO },
> +    { "ct-bkts", "[dp] [gt=N]", 0, 2, OVS_OUTPUT_FMT_TEXT, dpctl_ct_bkts,
> +      DP_RO },
> +    { "ct-set-maxconns", "[dp] maxconns", 1, 2, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_set_maxconns, DP_RW },
> +    { "ct-get-maxconns", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_get_maxconns, DP_RO },
> +    { "ct-get-nconns", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT, dpctl_ct_get_nconns,
> +      DP_RO },
> +    { "ct-enable-tcp-seq-chk", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_enable_tcp_seq_chk, DP_RW },
> +    { "ct-disable-tcp-seq-chk", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_disable_tcp_seq_chk, DP_RW },
> +    { "ct-get-tcp-seq-chk", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_get_tcp_seq_chk, DP_RO },
>      { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, INT_MAX,
> -        dpctl_ct_set_limits, DP_RO },
> +      OVS_OUTPUT_FMT_TEXT, dpctl_ct_set_limits, DP_RO },
>      { "ct-del-limits", "[dp] [default] [zone=N1[,N2]...]", 1, 3,
> -        dpctl_ct_del_limits, DP_RO },
> -    { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, dpctl_ct_get_limits,
> -        DP_RO },
> -    { "ct-get-sweep-interval", "[dp]", 0, 1, dpctl_ct_get_sweep, DP_RO },
> -    { "ct-set-sweep-interval", "[dp] ms", 1, 2, dpctl_ct_set_sweep, DP_RW },
> -    { "ipf-set-enabled", "[dp] v4|v6", 1, 2, dpctl_ipf_set_enabled, DP_RW },
> -    { "ipf-set-disabled", "[dp] v4|v6", 1, 2, dpctl_ipf_set_disabled, DP_RW },
> +      OVS_OUTPUT_FMT_TEXT, dpctl_ct_del_limits, DP_RO },
> +    { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_get_limits, DP_RO },
> +    { "ct-get-sweep-interval", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_get_sweep, DP_RO },
> +    { "ct-set-sweep-interval", "[dp] ms", 1, 2, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_set_sweep, DP_RW },
> +    { "ipf-set-enabled", "[dp] v4|v6", 1, 2, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ipf_set_enabled, DP_RW },
> +    { "ipf-set-disabled", "[dp] v4|v6", 1, 2, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ipf_set_disabled, DP_RW },
>      { "ipf-set-min-frag", "[dp] v4|v6 minfragment", 2, 3,
> -       dpctl_ipf_set_min_frag, DP_RW },
> +      OVS_OUTPUT_FMT_TEXT, dpctl_ipf_set_min_frag, DP_RW },
>      { "ipf-set-max-nfrags", "[dp] maxfrags", 1, 2,
> -       dpctl_ipf_set_max_nfrags, DP_RW },
> -    { "ipf-get-status", "[dp]", 0, 1, dpctl_ct_ipf_get_status,
> -       DP_RO },
> -    { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
> -    { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
> +      OVS_OUTPUT_FMT_TEXT, dpctl_ipf_set_max_nfrags, DP_RW },
> +    { "ipf-get-status", "[dp]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_ct_ipf_get_status, DP_RO },
> +    { "help", "", 0, INT_MAX, OVS_OUTPUT_FMT_TEXT, dpctl_help, DP_RO },
> +    { "list-commands", "", 0, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_list_commands, DP_RO },
>
>      /* Undocumented commands for testing. */
> -    { "parse-actions", "actions", 1, INT_MAX, dpctl_parse_actions, DP_RO },
> +    { "parse-actions", "actions", 1, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +      dpctl_parse_actions, DP_RO },
>      { "normalize-actions", "actions",
> -      2, INT_MAX, dpctl_normalize_actions, DP_RO },
> +      2, INT_MAX, OVS_OUTPUT_FMT_TEXT, dpctl_normalize_actions, DP_RO },
>
> -    { NULL, NULL, 0, 0, NULL, DP_RO },
> +    { NULL, NULL, 0, 0, 0, NULL, DP_RO },
>  };
>
>  static const struct dpctl_command *get_all_dpctl_commands(void)
> @@ -3105,7 +3130,7 @@ dpctl_unixctl_print(void *userdata, bool error OVS_UNUSED, const char *msg)
>
>  static void
>  dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
> -                      void *aux)
> +                      enum ovs_output_fmt fmt, void *aux)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      bool error = false;
> @@ -3114,6 +3139,7 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
>          .is_appctl = true,
>          .output = dpctl_unixctl_print,
>          .aux = &ds,
> +        .format = fmt,
>      };
>
>      /* Parse options (like getopt). Unfortunately it does
> @@ -3202,6 +3228,7 @@ dpctl_unixctl_register(void)
>                                       p->usage,
>                                       p->min_args,
>                                       p->max_args,
> +                                     p->output_fmts,
>                                       dpctl_unixctl_handler,
>                                       p->handler);
>              free(cmd_name);
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index d76d53f8f..4e71a8234 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -213,7 +213,8 @@ static cookie_io_functions_t dpdk_log_func = {
>
>  static void
>  dpdk_unixctl_mem_stream(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                        const char *argv[] OVS_UNUSED, void *aux)
> +                        const char *argv[] OVS_UNUSED,
> +                        enum ovs_output_fmt fmt OVS_UNUSED, void *aux)
>  {
>      void (*callback)(FILE *) = aux;
>      char *response = NULL;
> @@ -260,6 +261,7 @@ dpdk_parse_log_level(const char *s)
>
>  static void
>  dpdk_unixctl_log_set(struct unixctl_conn *conn, int argc, const char *argv[],
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
>                       void *aux OVS_UNUSED)
>  {
>      int i;
> @@ -427,14 +429,15 @@ dpdk_init__(const struct smap *ovs_other_config)
>          }
>      }
>
> -    unixctl_command_register("dpdk/lcore-list", "", 0, 0,
> -                             dpdk_unixctl_mem_stream, rte_lcore_dump);
> -    unixctl_command_register("dpdk/log-list", "", 0, 0,
> +    unixctl_command_register("dpdk/lcore-list", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             dpdk_unixctl_mem_stream,  rte_lcore_dump);

One space too many before rte_lcore_dump.

> +    unixctl_command_register("dpdk/log-list", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                               dpdk_unixctl_mem_stream, rte_log_dump);
>      unixctl_command_register("dpdk/log-set", "{level | pattern:level}", 0,
> -                             INT_MAX, dpdk_unixctl_log_set, NULL);
> +                             INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +                             dpdk_unixctl_log_set, NULL);
>      unixctl_command_register("dpdk/get-malloc-stats", "", 0, 0,
> -                             dpdk_unixctl_mem_stream,
> +                             OVS_OUTPUT_FMT_TEXT, dpdk_unixctl_mem_stream,
>                               malloc_dump_stats_wrapper);
>
>      /* We are called from the main thread here */
> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c
> index 79ea5e3be..23e735ef0 100644
> --- a/lib/dpif-netdev-perf.c
> +++ b/lib/dpif-netdev-perf.c
> @@ -706,6 +706,7 @@ pmd_perf_log_susp_iteration_neighborhood(struct pmd_perf_stats *s)
>  void
>  pmd_perf_log_set_cmd(struct unixctl_conn *conn,
>                   int argc, const char *argv[],
> +                 enum ovs_output_fmt fmt OVS_UNUSED,
>                   void *aux OVS_UNUSED)
>  {
>      unsigned int it_before, it_after, us_thr, q_thr;
> diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h
> index 84beced15..1e5cddbac 100644
> --- a/lib/dpif-netdev-perf.h
> +++ b/lib/dpif-netdev-perf.h
> @@ -432,6 +432,7 @@ void pmd_perf_format_ms_history(struct ds *str, struct pmd_perf_stats *s,
>                                  int n_ms);
>  void pmd_perf_log_set_cmd(struct unixctl_conn *conn,
>                            int argc, const char *argv[],
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
>                            void *aux OVS_UNUSED);
>
>  #ifdef  __cplusplus
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c1981137f..ea8917c74 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1018,6 +1018,7 @@ sorted_poll_thread_list(struct dp_netdev *dp,
>  static void
>  dpif_netdev_subtable_lookup_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                                  const char *argv[] OVS_UNUSED,
> +                                enum ovs_output_fmt fmt OVS_UNUSED,
>                                  void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
> @@ -1029,7 +1030,9 @@ dpif_netdev_subtable_lookup_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                                const char *argv[], void *aux OVS_UNUSED)
> +                                const char *argv[],
> +                                enum ovs_output_fmt fmt OVS_UNUSED,
> +                                void *aux OVS_UNUSED)
>  {
>      /* This function requires 2 parameters (argv[1] and argv[2]) to execute.
>       *   argv[1] is subtable name
> @@ -1109,7 +1112,9 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  dpif_netdev_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                     const char *argv[] OVS_UNUSED,
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>      struct shash_node *node;
> @@ -1133,7 +1138,8 @@ dpif_netdev_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  dpif_netdev_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[], void *aux OVS_UNUSED)
> +                     const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      /* This function requires just one parameter, the DPIF name. */
>      const char *dpif_name = argv[1];
> @@ -1195,6 +1201,7 @@ dpif_netdev_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  static void
>  dpif_miniflow_extract_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                                 const char *argv[] OVS_UNUSED,
> +                               enum ovs_output_fmt fmt OVS_UNUSED,
>                                 void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
> @@ -1219,7 +1226,9 @@ dpif_miniflow_extract_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
> -                               const char *argv[], void *aux OVS_UNUSED)
> +                               const char *argv[],
> +                               enum ovs_output_fmt fmt OVS_UNUSED,
> +                               void *aux OVS_UNUSED)
>  {
>      /* This command takes some optional and mandatory arguments. The function
>       * here first parses all of the options, saving results in local variables.
> @@ -1408,7 +1417,9 @@ error:
>
>  static void
>  dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
> -                          const char *argv[], void *aux OVS_UNUSED)
> +                          const char *argv[],
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
> +                          void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>      struct dp_netdev *dp = NULL;
> @@ -1451,7 +1462,7 @@ pmd_info_show_sleep(struct ds *reply, unsigned core_id, int numa_id,
>
>  static void
>  dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[],
> -                     void *aux)
> +                     enum ovs_output_fmt fmt OVS_UNUSED, void *aux)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>      struct dp_netdev_pmd_thread **pmd_list;
> @@ -1550,9 +1561,8 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[],
>  }
>
>  static void
> -pmd_perf_show_cmd(struct unixctl_conn *conn, int argc,
> -                          const char *argv[],
> -                          void *aux OVS_UNUSED)
> +pmd_perf_show_cmd(struct unixctl_conn *conn, int argc, const char *argv[],
> +                  enum ovs_output_fmt fmt, void *aux OVS_UNUSED)
>  {
>      struct pmd_perf_params par;
>      long int it_hist = 0, ms_hist = 0;
> @@ -1588,12 +1598,13 @@ pmd_perf_show_cmd(struct unixctl_conn *conn, int argc,
>      par.iter_hist_len = it_hist;
>      par.ms_hist_len = ms_hist;
>      par.command_type = PMD_INFO_PERF_SHOW;
> -    dpif_netdev_pmd_info(conn, argc, argv, &par);
> +    dpif_netdev_pmd_info(conn, argc, argv, fmt, &par);
>  }
>
>  static void
> -dpif_netdev_bond_show(struct unixctl_conn *conn, int argc,
> -                      const char *argv[], void *aux OVS_UNUSED)
> +dpif_netdev_bond_show(struct unixctl_conn *conn, int argc, const char *argv[],
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>      struct dp_netdev *dp = NULL;
> @@ -1643,60 +1654,60 @@ dpif_netdev_init(void)
>                                sleep_aux = PMD_INFO_SLEEP_SHOW;
>
>      unixctl_command_register("dpif-netdev/pmd-stats-show", "[-pmd core] [dp]",
> -                             0, 3, dpif_netdev_pmd_info,
> +                             0, 3, OVS_OUTPUT_FMT_TEXT, dpif_netdev_pmd_info,
>                               (void *)&show_aux);
>      unixctl_command_register("dpif-netdev/pmd-stats-clear", "[-pmd core] [dp]",
> -                             0, 3, dpif_netdev_pmd_info,
> +                             0, 3, OVS_OUTPUT_FMT_TEXT, dpif_netdev_pmd_info,
>                               (void *)&clear_aux);
>      unixctl_command_register("dpif-netdev/pmd-rxq-show", "[-pmd core] "
>                               "[-secs secs] [dp]",
> -                             0, 5, dpif_netdev_pmd_info,
> +                             0, 5, OVS_OUTPUT_FMT_TEXT, dpif_netdev_pmd_info,
>                               (void *)&poll_aux);
>      unixctl_command_register("dpif-netdev/pmd-sleep-show", "[dp]",
> -                             0, 1, dpif_netdev_pmd_info,
> +                             0, 1, OVS_OUTPUT_FMT_TEXT, dpif_netdev_pmd_info,
>                               (void *)&sleep_aux);
>      unixctl_command_register("dpif-netdev/pmd-perf-show",
>                               "[-nh] [-it iter-history-len]"
>                               " [-ms ms-history-len]"
>                               " [-pmd core] [dp]",
> -                             0, 8, pmd_perf_show_cmd,
> +                             0, 8, OVS_OUTPUT_FMT_TEXT, pmd_perf_show_cmd,
>                               NULL);
>      unixctl_command_register("dpif-netdev/pmd-rxq-rebalance", "[dp]",
> -                             0, 1, dpif_netdev_pmd_rebalance,
> -                             NULL);
> +                             0, 1, OVS_OUTPUT_FMT_TEXT,
> +                             dpif_netdev_pmd_rebalance, NULL);
>      unixctl_command_register("dpif-netdev/pmd-perf-log-set",
>                               "on|off [-b before] [-a after] [-e|-ne] "
>                               "[-us usec] [-q qlen]",
> -                             0, 10, pmd_perf_log_set_cmd,
> +                             0, 10, OVS_OUTPUT_FMT_TEXT, pmd_perf_log_set_cmd,
>                               NULL);
>      unixctl_command_register("dpif-netdev/bond-show", "[dp]",
> -                             0, 1, dpif_netdev_bond_show,
> +                             0, 1, OVS_OUTPUT_FMT_TEXT, dpif_netdev_bond_show,
>                               NULL);
>      unixctl_command_register("dpif-netdev/subtable-lookup-prio-set",
>                               "[lookup_func] [prio]",
> -                             2, 2, dpif_netdev_subtable_lookup_set,
> -                             NULL);
> +                             2, 2, OVS_OUTPUT_FMT_TEXT,
> +                             dpif_netdev_subtable_lookup_set, NULL);
>      unixctl_command_register("dpif-netdev/subtable-lookup-info-get", "",
> -                             0, 0, dpif_netdev_subtable_lookup_get,
> -                             NULL);
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             dpif_netdev_subtable_lookup_get, NULL);
>      unixctl_command_register("dpif-netdev/subtable-lookup-prio-get", NULL,
> -                             0, 0, dpif_netdev_subtable_lookup_get,
> -                             NULL);
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             dpif_netdev_subtable_lookup_get, NULL);
>      unixctl_command_register("dpif-netdev/dpif-impl-set",
>                               "dpif_implementation_name",
> -                             1, 1, dpif_netdev_impl_set,
> +                             1, 1, OVS_OUTPUT_FMT_TEXT, dpif_netdev_impl_set,
>                               NULL);
>      unixctl_command_register("dpif-netdev/dpif-impl-get", "",
> -                             0, 0, dpif_netdev_impl_get,
> +                             0, 0, OVS_OUTPUT_FMT_TEXT, dpif_netdev_impl_get,
>                               NULL);
>      unixctl_command_register("dpif-netdev/miniflow-parser-set",
>                               "[-pmd core] miniflow_implementation_name"
>                               " [study_pkt_cnt]",
> -                             1, 5, dpif_miniflow_extract_impl_set,
> -                             NULL);
> +                             1, 5, OVS_OUTPUT_FMT_TEXT,
> +                             dpif_miniflow_extract_impl_set, NULL);
>      unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
> -                             0, 0, dpif_miniflow_extract_impl_get,
> -                             NULL);
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             dpif_miniflow_extract_impl_get, NULL);
>      return 0;
>  }
>
> @@ -10043,7 +10054,9 @@ const struct dpif_class dpif_netdev_class = {
>
>  static void
>  dpif_dummy_change_port_number(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                              const char *argv[], void *aux OVS_UNUSED)
> +                              const char *argv[],
> +                              enum ovs_output_fmt fmt OVS_UNUSED,
> +                              void *aux OVS_UNUSED)
>  {
>      struct dp_netdev_port *port;
>      struct dp_netdev *dp;
> @@ -10139,7 +10152,8 @@ dpif_dummy_register(enum dummy_level level)
>
>      unixctl_command_register("dpif-dummy/change-port-number",
>                               "dp port new-number",
> -                             3, 3, dpif_dummy_change_port_number, NULL);
> +                             3, 3, OVS_OUTPUT_FMT_TEXT,
> +                             dpif_dummy_change_port_number, NULL);
>  }
>  
>  /* Datapath Classifier. */
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 84e2bd8ea..670df6671 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -124,7 +124,9 @@ dpif_netlink_set_features(struct dpif *dpif_, uint32_t new_features);
>
>  static void
>  dpif_netlink_unixctl_dispatch_mode(struct unixctl_conn *conn, int argc,
> -                                   const char *argv[], void *aux);
> +                                   const char *argv[],
> +                                   enum ovs_output_fmt fmt OVS_UNUSED,
> +                                   void *aux);
>
>  struct dpif_netlink_flow {
>      /* Generic Netlink header. */
> @@ -4634,6 +4636,7 @@ dpif_netlink_init(void)
>          ovs_tunnels_out_of_tree = dpif_netlink_rtnl_probe_oot_tunnels();
>
>          unixctl_command_register("dpif-netlink/dispatch-mode", "", 0, 0,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   dpif_netlink_unixctl_dispatch_mode, NULL);
>
>          ovsthread_once_done(&once);
> @@ -5287,6 +5290,7 @@ static void
>  dpif_netlink_unixctl_dispatch_mode(struct unixctl_conn *conn,
>                                     int argc OVS_UNUSED,
>                                     const char *argv[] OVS_UNUSED,
> +                                   enum ovs_output_fmt fmt OVS_UNUSED,
>                                     void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
> diff --git a/lib/lacp.c b/lib/lacp.c
> index 3252f17eb..9923efb72 100644
> --- a/lib/lacp.c
> +++ b/lib/lacp.c
> @@ -225,10 +225,11 @@ parse_lacp_packet(const struct dp_packet *p, enum pdu_subtype *subtype)
>  void
>  lacp_init(void)
>  {
> -    unixctl_command_register("lacp/show", "[port]", 0, 1,
> +    unixctl_command_register("lacp/show", "[port]", 0, 1, OVS_OUTPUT_FMT_TEXT,
>                               lacp_unixctl_show, NULL);
>      unixctl_command_register("lacp/show-stats", "[port]", 0, 1,
> -                             lacp_unixctl_show_stats, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, lacp_unixctl_show_stats,
> +                             NULL);
>  }
>
>  static void
> @@ -1114,6 +1115,7 @@ lacp_print_stats(struct ds *ds, struct lacp *lacp) OVS_REQUIRES(mutex)
>
>  static void
>  lacp_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
>                    void *aux OVS_UNUSED) OVS_EXCLUDED(mutex)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -1144,6 +1146,7 @@ static void
>  lacp_unixctl_show_stats(struct unixctl_conn *conn,
>                    int argc,
>                    const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
>                    void *aux OVS_UNUSED) OVS_EXCLUDED(mutex)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> diff --git a/lib/memory.c b/lib/memory.c
> index da97476c6..cd6950aa3 100644
> --- a/lib/memory.c
> +++ b/lib/memory.c
> @@ -160,7 +160,8 @@ memory_report(const struct simap *usage)
>
>  static void
>  memory_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                    const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                    const char *argv[] OVS_UNUSED,
> +                    enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      conns = xrealloc(conns, (n_conns + 1) * sizeof *conns);
>      conns[n_conns++] = conn;
> @@ -173,7 +174,7 @@ memory_init(void)
>
>      if (!inited) {
>          inited = true;
> -        unixctl_command_register("memory/show", "", 0, 0,
> +        unixctl_command_register("memory/show", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                                   memory_unixctl_show, NULL);
>
>          next_check = time_boot_msec() + MEMORY_CHECK_INTERVAL;
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index fb26825ff..ef7811a31 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4367,7 +4367,9 @@ netdev_dpdk_set_admin_state__(struct netdev_dpdk *dev, bool admin_state)
>
>  static void
>  netdev_dpdk_set_admin_state(struct unixctl_conn *conn, int argc,
> -                            const char *argv[], void *aux OVS_UNUSED)
> +                            const char *argv[],
> +                            enum ovs_output_fmt fmt OVS_UNUSED,
> +                            void *aux OVS_UNUSED)
>  {
>      bool up;
>
> @@ -4412,7 +4414,9 @@ netdev_dpdk_set_admin_state(struct unixctl_conn *conn, int argc,
>
>  static void
>  netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                   const char *argv[], void *aux OVS_UNUSED)
> +                   const char *argv[],
> +                   enum ovs_output_fmt fmt OVS_UNUSED,
> +                   void *aux OVS_UNUSED)
>  {
>      struct ds used_interfaces = DS_EMPTY_INITIALIZER;
>      struct rte_eth_dev_info dev_info;
> @@ -4479,6 +4483,7 @@ error:
>  static void
>  netdev_dpdk_get_mempool_info(struct unixctl_conn *conn,
>                               int argc, const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED,
>                               void *aux OVS_UNUSED)
>  {
>      size_t size;
> @@ -4991,19 +4996,22 @@ netdev_dpdk_class_init(void)
>          ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL);
>          unixctl_command_register("netdev-dpdk/set-admin-state",
>                                   "[netdev] up|down", 1, 2,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   netdev_dpdk_set_admin_state, NULL);
>
>          unixctl_command_register("netdev-dpdk/detach",
>                                   "pci address of device", 1, 1,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   netdev_dpdk_detach, NULL);
>
>          unixctl_command_register("netdev-dpdk/get-mempool-info",
> -                                 "[netdev]", 0, 1,
> +                                 "[netdev]", 0, 1, OVS_OUTPUT_FMT_TEXT,
>                                   netdev_dpdk_get_mempool_info, NULL);
>
>          ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
>                                              RTE_ETH_EVENT_INTR_RESET,
> -                                            dpdk_eth_event_callback, NULL);
> +                                            dpdk_eth_event_callback,
> +                                            NULL);
>          if (ret != 0) {
>              VLOG_ERR("Ethernet device callback register error: %s",
>                       rte_strerror(-ret));
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index cd7e85a81..3ed0ec610 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -1887,7 +1887,8 @@ netdev_dummy_queue_packet(struct netdev_dummy *dummy, struct dp_packet *packet,
>
>  static void
>  netdev_dummy_receive(struct unixctl_conn *conn,
> -                     int argc, const char *argv[], void *aux OVS_UNUSED)
> +                     int argc, const char *argv[],
> +                     enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct netdev_dummy *dummy_dev;
>      struct netdev *netdev;
> @@ -1970,7 +1971,9 @@ netdev_dummy_set_admin_state__(struct netdev_dummy *dev, bool admin_state)
>
>  static void
>  netdev_dummy_set_admin_state(struct unixctl_conn *conn, int argc,
> -                             const char *argv[], void *aux OVS_UNUSED)
> +                             const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED,
> +                             void *aux OVS_UNUSED)
>  {
>      bool up;
>
> @@ -2036,7 +2039,9 @@ display_conn_state__(struct ds *s, const char *name,
>
>  static void
>  netdev_dummy_conn_state(struct unixctl_conn *conn, int argc,
> -                        const char *argv[], void *aux OVS_UNUSED)
> +                        const char *argv[],
> +                        enum ovs_output_fmt fmt OVS_UNUSED,
> +                        void *aux OVS_UNUSED)
>  {
>      enum dummy_netdev_conn_state state = CONN_STATE_UNKNOWN;
>      struct ds s;
> @@ -2078,7 +2083,9 @@ netdev_dummy_conn_state(struct unixctl_conn *conn, int argc,
>
>  static void
>  netdev_dummy_ip4addr(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[], void *aux OVS_UNUSED)
> +                     const char *argv[],
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      struct netdev *netdev = netdev_from_name(argv[1]);
>
> @@ -2103,7 +2110,8 @@ netdev_dummy_ip4addr(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  netdev_dummy_ip6addr(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[], void *aux OVS_UNUSED)
> +                     const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      struct netdev *netdev = netdev_from_name(argv[1]);
>
> @@ -2155,18 +2163,20 @@ netdev_dummy_register(enum dummy_level level)
>  {
>      unixctl_command_register("netdev-dummy/receive",
>                               "name [--qid queue_id] packet|flow [--len packet_len]",
> -                             2, INT_MAX, netdev_dummy_receive, NULL);
> +                             2, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +                             netdev_dummy_receive, NULL);
>      unixctl_command_register("netdev-dummy/set-admin-state",
> -                             "[netdev] up|down", 1, 2,
> +                             "[netdev] up|down", 1, 2, OVS_OUTPUT_FMT_TEXT,
>                               netdev_dummy_set_admin_state, NULL);
>      unixctl_command_register("netdev-dummy/conn-state",
> -                             "[netdev]", 0, 1,
> +                             "[netdev]", 0, 1, OVS_OUTPUT_FMT_TEXT,
>                               netdev_dummy_conn_state, NULL);
>      unixctl_command_register("netdev-dummy/ip4addr",
>                               "[netdev] ipaddr/mask-prefix-len", 2, 2,
> -                             netdev_dummy_ip4addr, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, netdev_dummy_ip4addr,
> +                             NULL);
>      unixctl_command_register("netdev-dummy/ip6addr",
> -                             "[netdev] ip6addr", 2, 2,
> +                             "[netdev] ip6addr", 2, 2, OVS_OUTPUT_FMT_TEXT,
>                               netdev_dummy_ip6addr, NULL);
>
>      if (level == DUMMY_OVERRIDE_ALL) {
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 0d6d803fe..6fcd675a2 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -1275,7 +1275,9 @@ netdev_geneve_build_header(const struct netdev *netdev,
>  
>  void
>  netdev_tnl_egress_port_range(struct unixctl_conn *conn, int argc,
> -                             const char *argv[], void *aux OVS_UNUSED)
> +                             const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED,
> +                             void *aux OVS_UNUSED)
>  {
>      int val1, val2;
>
> diff --git a/lib/netdev-native-tnl.h b/lib/netdev-native-tnl.h
> index eb55dd041..e5d067d97 100644
> --- a/lib/netdev-native-tnl.h
> +++ b/lib/netdev-native-tnl.h
> @@ -142,5 +142,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, const void *header,
>                            int size, int *ip_tot_size, ovs_be32 ipv6_label);
>  void
>  netdev_tnl_egress_port_range(struct unixctl_conn *conn, int argc,
> -                             const char *argv[], void *aux OVS_UNUSED);
> +                             const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED,
> +                             void *aux OVS_UNUSED);
>  #endif
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 60caa02fb..1acd0e5bf 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -1389,6 +1389,7 @@ netdev_vport_tunnel_register(void)
>          }
>
>          unixctl_command_register("tnl/egress_port_range", "min max", 0, 2,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   netdev_tnl_egress_port_range, NULL);
>
>          ovsthread_once_done(&once);
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index eb03b57c4..230bd26a8 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -916,7 +916,8 @@ odp_actions_impl_set(const char *name)
>
>  static void
>  action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                const char *argv[], void *aux OVS_UNUSED)
> +                const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>
> @@ -936,7 +937,8 @@ action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  action_impl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                 const char *argv[] OVS_UNUSED,
> +                 enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>
> @@ -949,10 +951,10 @@ static void
>  odp_execute_unixctl_init(void)
>  {
>      unixctl_command_register("odp-execute/action-impl-set", "name",
> -                             1, 1, action_impl_set,
> +                             1, 1, OVS_OUTPUT_FMT_TEXT, action_impl_set,
>                               NULL);
>      unixctl_command_register("odp-execute/action-impl-show", "",
> -                             0, 0, action_impl_show,
> +                             0, 0, OVS_OUTPUT_FMT_TEXT, action_impl_show,
>                               NULL);
>  }
>
> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
> index 2d13e971e..8dbc037a5 100644
> --- a/lib/ovs-lldp.c
> +++ b/lib/ovs-lldp.c
> @@ -326,7 +326,8 @@ aa_print_isid_status(struct ds *ds, struct lldp *lldp) OVS_REQUIRES(mutex)
>
>  static void
>  aa_unixctl_status(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                  const char *argv[] OVS_UNUSED,
> +                  enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>      OVS_EXCLUDED(mutex)
>  {
>      struct lldp *lldp;
> @@ -345,7 +346,8 @@ aa_unixctl_status(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  aa_unixctl_show_isid(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                     const char *argv[] OVS_UNUSED,
> +                     enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>      OVS_EXCLUDED(mutex)
>  {
>      struct lldp *lldp;
> @@ -364,7 +366,8 @@ aa_unixctl_show_isid(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  aa_unixctl_statistics(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                      const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                      const char *argv[] OVS_UNUSED,
> +                      enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>      OVS_EXCLUDED(mutex)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -644,11 +647,12 @@ void
>  lldp_init(void)
>  {
>      unixctl_command_register("autoattach/status", "[bridge]", 0, 1,
> -                             aa_unixctl_status, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, aa_unixctl_status, NULL);
>      unixctl_command_register("autoattach/show-isid", "[bridge]", 0, 1,
> -                             aa_unixctl_show_isid, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, aa_unixctl_show_isid, NULL);
>      unixctl_command_register("autoattach/statistics", "[bridge]", 0, 1,
> -                             aa_unixctl_statistics, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, aa_unixctl_statistics,
> +                             NULL);
>  }
>
>  /* Returns true if 'lldp' should process packets from 'flow'.  Sets
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index ca014d80e..625fb8942 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -390,8 +390,8 @@ scan_ipv4_route(const char *s, ovs_be32 *addr, unsigned int *plen)
>  }
>
>  static void
> -ovs_router_add(struct unixctl_conn *conn, int argc,
> -              const char *argv[], void *aux OVS_UNUSED)
> +ovs_router_add(struct unixctl_conn *conn, int argc, const char *argv[],
> +               enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct in6_addr src6 = in6addr_any;
>      struct in6_addr gw6 = in6addr_any;
> @@ -464,7 +464,8 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
>
>  static void
>  ovs_router_del(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -              const char *argv[], void *aux OVS_UNUSED)
> +               const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +               void *aux OVS_UNUSED)
>  {
>      struct in6_addr ip6;
>      uint32_t mark = 0;
> @@ -495,7 +496,8 @@ ovs_router_del(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovs_router_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -               const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                const char *argv[] OVS_UNUSED,
> +                enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct ovs_router_entry *rt;
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -535,8 +537,9 @@ ovs_router_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  }
>
>  static void
> -ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc,
> -                      const char *argv[], void *aux OVS_UNUSED)
> +ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc, const char *argv[],
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
>  {
>      struct in6_addr gw, src = in6addr_any;
>      char iface[IFNAMSIZ];
> @@ -606,14 +609,15 @@ ovs_router_init(void)
>          unixctl_command_register("ovs/route/add",
>                                   "ip/plen output_bridge [gw] "
>                                   "[pkt_mark=mark] [src=src_ip]",
> -                                 2, 5, ovs_router_add, NULL);
> +                                 2, 5, OVS_OUTPUT_FMT_TEXT, ovs_router_add,
> +                                 NULL);
>          unixctl_command_register("ovs/route/show", "", 0, 0,
> -                                 ovs_router_show, NULL);
> +                                 OVS_OUTPUT_FMT_TEXT,  ovs_router_show, NULL);

One space too many before ovs_router_show

>          unixctl_command_register("ovs/route/del", "ip/plen "
> -                                 "[pkt_mark=mark]", 1, 2, ovs_router_del,
> -                                 NULL);
> +                                 "[pkt_mark=mark]", 1, 2, OVS_OUTPUT_FMT_TEXT,
> +                                 ovs_router_del, NULL);
>          unixctl_command_register("ovs/route/lookup", "ip_addr "
> -                                 "[pkt_mark=mark]", 1, 2,
> +                                 "[pkt_mark=mark]", 1, 2, OVS_OUTPUT_FMT_TEXT,
>                                   ovs_router_lookup_cmd, NULL);
>          ovsthread_once_done(&once);
>      }
> diff --git a/lib/rstp.c b/lib/rstp.c
> index 2f01966f7..0f5a0e91d 100644
> --- a/lib/rstp.c
> +++ b/lib/rstp.c
> @@ -129,9 +129,11 @@ static struct rstp_port *rstp_get_root_port__(const struct rstp *rstp)
>  static rstp_identifier rstp_get_root_id__(const struct rstp *rstp)
>      OVS_REQUIRES(rstp_mutex);
>  static void rstp_unixctl_tcn(struct unixctl_conn *, int argc,
> -                             const char *argv[], void *aux);
> +                             const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED, void *aux);
>  static void rstp_unixctl_show(struct unixctl_conn *, int argc,
> -                              const char *argv[], void *aux);
> +                              const char *argv[],
> +                              enum ovs_output_fmt fmt OVS_UNUSED, void *aux);
>
>  const char *
>  rstp_state_name(enum rstp_state state)
> @@ -248,10 +250,10 @@ void
>  rstp_init(void)
>      OVS_EXCLUDED(rstp_mutex)
>  {
> -    unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, rstp_unixctl_tcn,
> -                             NULL);
> -    unixctl_command_register("rstp/show", "[bridge]", 0, 1, rstp_unixctl_show,
> -                             NULL);
> +    unixctl_command_register("rstp/tcn", "[bridge]", 0, 1,
> +                             OVS_OUTPUT_FMT_TEXT, rstp_unixctl_tcn, NULL);
> +    unixctl_command_register("rstp/show", "[bridge]", 0, 1,
> +                             OVS_OUTPUT_FMT_TEXT, rstp_unixctl_show, NULL);
>  }
>
>  /* Creates and returns a new RSTP instance that initially has no ports. */
> @@ -1550,8 +1552,8 @@ rstp_find(const char *name)
>  }
>
>  static void
> -rstp_unixctl_tcn(struct unixctl_conn *conn, int argc,
> -                 const char *argv[], void *aux OVS_UNUSED)
> +rstp_unixctl_tcn(struct unixctl_conn *conn, int argc, const char *argv[],
> +                 enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>      OVS_EXCLUDED(rstp_mutex)
>  {
>      ovs_mutex_lock(&rstp_mutex);
> @@ -1651,8 +1653,8 @@ rstp_print_details(struct ds *ds, const struct rstp *rstp)
>  }
>
>  static void
> -rstp_unixctl_show(struct unixctl_conn *conn, int argc,
> -                  const char *argv[], void *aux OVS_UNUSED)
> +rstp_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>      OVS_EXCLUDED(rstp_mutex)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> diff --git a/lib/stopwatch.c b/lib/stopwatch.c
> index ec567603b..70fcc2979 100644
> --- a/lib/stopwatch.c
> +++ b/lib/stopwatch.c
> @@ -314,7 +314,8 @@ stopwatch_show_protected(int argc, const char *argv[], struct ds *s)
>
>  static void
>  stopwatch_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -               const char *argv[], void *aux OVS_UNUSED)
> +               const char *argv[],
> +               enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct ds s = DS_EMPTY_INITIALIZER;
>      bool success;
> @@ -351,7 +352,8 @@ stopwatch_packet_write(struct stopwatch_packet *pkt)
>
>  static void
>  stopwatch_reset(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                const char *argv[], void *aux OVS_UNUSED)
> +                const char *argv[],
> +                enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct stopwatch_packet *pkt = stopwatch_packet_create(OP_RESET);
>      if (argc > 1) {
> @@ -488,9 +490,9 @@ static void
>  do_init_stopwatch(void)
>  {
>      unixctl_command_register("stopwatch/show", "[NAME]", 0, 1,
> -                             stopwatch_show, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, stopwatch_show, NULL);
>      unixctl_command_register("stopwatch/reset", "[NAME]", 0, 1,
> -                             stopwatch_reset, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, stopwatch_reset, NULL);
>      guarded_list_init(&stopwatch_commands);
>      latch_init(&stopwatch_latch);
>      stopwatch_thread_id = ovs_thread_create(
> diff --git a/lib/stp.c b/lib/stp.c
> index f37337992..0d1b6b053 100644
> --- a/lib/stp.c
> +++ b/lib/stp.c
> @@ -233,9 +233,11 @@ static bool stp_timer_expired(struct stp_timer *, int elapsed, int timeout);
>  static void stp_send_bpdu(struct stp_port *, const void *, size_t)
>      OVS_REQUIRES(mutex);
>  static void stp_unixctl_tcn(struct unixctl_conn *, int argc,
> -                            const char *argv[], void *aux);
> +                            const char *argv[],
> +                            enum ovs_output_fmt fmt OVS_UNUSED, void *aux);
>  static void stp_unixctl_show(struct unixctl_conn *, int argc,
> -                             const char *argv[], void *aux);
> +                             const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED, void *aux);
>
>  void
>  stp_init(void)
> @@ -249,10 +251,10 @@ stp_init(void)
>           * the call back function, but for now this is what we have. */
>          ovs_mutex_init_recursive(&mutex);
>
> -        unixctl_command_register("stp/tcn", "[bridge]", 0, 1, stp_unixctl_tcn,
> -                                 NULL);
> +        unixctl_command_register("stp/tcn", "[bridge]", 0, 1,
> +                                 OVS_OUTPUT_FMT_TEXT, stp_unixctl_tcn, NULL);
>          unixctl_command_register("stp/show", "[bridge]", 0, 1,
> -                                 stp_unixctl_show, NULL);
> +                                 OVS_OUTPUT_FMT_TEXT, stp_unixctl_show, NULL);
>          ovsthread_once_done(&once);
>      }
>  }
> @@ -1611,8 +1613,8 @@ stp_find(const char *name) OVS_REQUIRES(mutex)
>  }
>
>  static void
> -stp_unixctl_tcn(struct unixctl_conn *conn, int argc,
> -                const char *argv[], void *aux OVS_UNUSED)
> +stp_unixctl_tcn(struct unixctl_conn *conn, int argc, const char *argv[],
> +                enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      ovs_mutex_lock(&mutex);
>      if (argc > 1) {
> @@ -1702,8 +1704,8 @@ stp_print_details(struct ds *ds, const struct stp *stp)
>  }
>
>  static void
> -stp_unixctl_show(struct unixctl_conn *conn, int argc,
> -                 const char *argv[], void *aux OVS_UNUSED)
> +stp_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
> +                 enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>
> diff --git a/lib/timeval.c b/lib/timeval.c
> index 10c1b9ca1..f0d785a70 100644
> --- a/lib/timeval.c
> +++ b/lib/timeval.c
> @@ -780,6 +780,7 @@ timeval_stop(void)
>  static void
>  timeval_stop_cb(struct unixctl_conn *conn,
>                  int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
> +                enum ovs_output_fmt fmt OVS_UNUSED,
>                  void *aux OVS_UNUSED)
>  {
>      timeval_stop();
> @@ -798,7 +799,8 @@ timeval_stop_cb(struct unixctl_conn *conn,
>   * Does not affect wall clock readings. */
>  static void
>  timeval_warp_cb(struct unixctl_conn *conn,
> -                int argc OVS_UNUSED, const char *argv[], void *aux OVS_UNUSED)
> +                int argc OVS_UNUSED, const char *argv[],
> +                enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      long long int total_warp = argc > 2 ? atoll(argv[1]) : 0;
>      long long int msecs = argc > 2 ? atoll(argv[2]) : atoll(argv[1]);
> @@ -842,9 +844,10 @@ void
>  timeval_dummy_register(void)
>  {
>      timewarp_enabled = true;
> -    unixctl_command_register("time/stop", "", 0, 0, timeval_stop_cb, NULL);
> +    unixctl_command_register("time/stop", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             timeval_stop_cb, NULL);
>      unixctl_command_register("time/warp", "[large_msecs] msecs", 1, 2,
> -                             timeval_warp_cb, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, timeval_warp_cb, NULL);
>  }
>
>
> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
> index bdff1debc..b8aeee29a 100644
> --- a/lib/tnl-neigh-cache.c
> +++ b/lib/tnl-neigh-cache.c
> @@ -273,7 +273,9 @@ tnl_neigh_flush(const char br_name[IFNAMSIZ])
>
>  static void
>  tnl_neigh_cache_flush(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                    const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                      const char *argv[] OVS_UNUSED,
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
>  {
>      struct tnl_neigh_entry *neigh;
>      bool changed = false;
> @@ -291,8 +293,9 @@ tnl_neigh_cache_flush(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  }
>
>  static void
> -tnl_neigh_cache_aging(struct unixctl_conn *conn, int argc,
> -                        const char *argv[], void *aux OVS_UNUSED)
> +tnl_neigh_cache_aging(struct unixctl_conn *conn, int argc, const char *argv[],
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
>  {
>      long long int new_exp, curr_exp;
>      struct tnl_neigh_entry *neigh;
> @@ -348,7 +351,8 @@ lookup_any(const char *host_name, struct in6_addr *address)
>
>  static void
>  tnl_neigh_cache_add(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                    const char *argv[], void *aux OVS_UNUSED)
> +                    const char *argv[],
> +                    enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      const char *br_name = argv[1];
>      struct eth_addr mac;
> @@ -370,7 +374,9 @@ tnl_neigh_cache_add(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  tnl_neigh_cache_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                     const char *argv[] OVS_UNUSED,
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      struct tnl_neigh_entry *neigh;
> @@ -404,20 +410,23 @@ void
>  tnl_neigh_cache_init(void)
>  {
>      atomic_init(&neigh_aging, NEIGH_ENTRY_DEFAULT_IDLE_TIME_MS);
> -    unixctl_command_register("tnl/arp/show", "", 0, 0,
> +    unixctl_command_register("tnl/arp/show", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                               tnl_neigh_cache_show, NULL);
>      unixctl_command_register("tnl/arp/set", "BRIDGE IP MAC", 3, 3,
> -                             tnl_neigh_cache_add, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, tnl_neigh_cache_add, NULL);
>      unixctl_command_register("tnl/arp/flush", "", 0, 0,
> -                             tnl_neigh_cache_flush, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, tnl_neigh_cache_flush,
> +                             NULL);
>      unixctl_command_register("tnl/arp/aging", "[SECS]", 0, 1,
> -                             tnl_neigh_cache_aging, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, tnl_neigh_cache_aging,
> +                             NULL);
>      unixctl_command_register("tnl/neigh/show", "", 0, 0,
> -                             tnl_neigh_cache_show, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, tnl_neigh_cache_show, NULL);
>      unixctl_command_register("tnl/neigh/set", "BRIDGE IP MAC", 3, 3,
> -                             tnl_neigh_cache_add, NULL);
> -    unixctl_command_register("tnl/neigh/flush", "", 0, 0,
> +                             OVS_OUTPUT_FMT_TEXT, tnl_neigh_cache_add, NULL);
> +    unixctl_command_register("tnl/neigh/flush", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                               tnl_neigh_cache_flush, NULL);
>      unixctl_command_register("tnl/neigh/aging", "[SECS]", 0, 1,
> -                             tnl_neigh_cache_aging, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, tnl_neigh_cache_aging,
> +                             NULL);
>  }
> diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
> index bb0b0b0c5..e3739882d 100644
> --- a/lib/tnl-ports.c
> +++ b/lib/tnl-ports.c
> @@ -354,7 +354,8 @@ tnl_port_show_v(struct ds *ds)
>
>  static void
>  tnl_port_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -               const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +              const char *argv[] OVS_UNUSED,
> +              enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      struct tnl_port *p;
> @@ -523,5 +524,6 @@ tnl_port_map_init(void)
>      classifier_init(&cls, flow_segment_u64s);
>      ovs_list_init(&addr_list);
>      ovs_list_init(&port_list);
> -    unixctl_command_register("tnl/ports/show", "-v", 0, 1, tnl_port_show, NULL);
> +    unixctl_command_register("tnl/ports/show", "-v", 0, 1,
> +                             OVS_OUTPUT_FMT_TEXT, tnl_port_show, NULL);
>  }
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index dd1450f22..3b9f300ba 100644
> --- a/lib/unixctl.c
> +++ b/lib/unixctl.c
> @@ -67,7 +67,8 @@ static struct shash commands = SHASH_INITIALIZER(&commands);
>
>  static void
>  unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                      const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                      const char *argv[] OVS_UNUSED,
> +                      enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      const struct shash_node **nodes = shash_sort(&commands);
> @@ -91,13 +92,15 @@ unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  unixctl_version(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                const char *argv[] OVS_UNUSED,
> +                enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      unixctl_command_reply(conn, ovs_get_program_version());
>  }
>
>  static void
>  unixctl_set_options(struct unixctl_conn *conn, int argc, const char *argv[],
> +                    enum ovs_output_fmt fmt OVS_UNUSED,
>                      void *aux OVS_UNUSED)
>  {
>      char * error = NULL;
> @@ -129,26 +132,6 @@ error:
>      free(error);
>  }
>
> -/* Registers a unixctl command with the given 'name'.  'usage' describes the
> - * arguments to the command; it is used only for presentation to the user in
> - * "list-commands" output.  (If 'usage' is NULL, then the command is hidden.)
> - *
> - * 'cb' is called when the command is received.  It is passed an array
> - * containing the command name and arguments, plus a copy of 'aux'.  Normally
> - * 'cb' should reply by calling unixctl_command_reply() or
> - * unixctl_command_reply_error() before it returns, but if the command cannot
> - * be handled immediately then it can defer the reply until later.  A given
> - * connection can only process a single request at a time, so a reply must be
> - * made eventually to avoid blocking that connection. */
> -void
> -unixctl_command_register(const char *name, const char *usage,
> -                         int min_args, int max_args,
> -                         unixctl_cb_func *cb, void *aux)
> -{
> -    unixctl_command_register_fmt(name, usage, min_args, max_args,
> -                                 OVS_OUTPUT_FMT_TEXT, cb, aux);
> -}
> -
>  /* Registers a unixctl command with the given 'name'.  'usage' describes the
>   * arguments to the command; it is used only for presentation to the user in
>   * "list-commands" output.  (If 'usage' is NULL, then the command is hidden.)
> @@ -163,9 +146,9 @@ unixctl_command_register(const char *name, const char *usage,
>   * connection can only process a single request at a time, so a reply must be
>   * made eventually to avoid blocking that connection. */
>  void
> -unixctl_command_register_fmt(const char *name, const char *usage,
> -                             int min_args, int max_args, int output_fmts,
> -                             unixctl_cb_func *cb, void *aux)
> +unixctl_command_register(const char *name, const char *usage,
> +                         int min_args, int max_args, int output_fmts,
> +                         unixctl_cb_func *cb, void *aux)
>  {
>      struct unixctl_command *command;
>      struct unixctl_command *lookup = shash_find_data(&commands, name);
> @@ -323,11 +306,12 @@ unixctl_server_create(const char *path, struct unixctl_server **serverp)
>          return error;
>      }
>
> -    unixctl_command_register("list-commands", "", 0, 0, unixctl_list_commands,
> -                             NULL);
> -    unixctl_command_register("version", "", 0, 0, unixctl_version, NULL);
> +    unixctl_command_register("list-commands", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             unixctl_list_commands, NULL);
> +    unixctl_command_register("version", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             unixctl_version, NULL);
>      unixctl_command_register("set-options", "[--format text|json]", 2, 2,
> -                             unixctl_set_options, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, unixctl_set_options, NULL);
>
>      struct unixctl_server *server = xmalloc(sizeof *server);
>      server->listener = listener;
> @@ -376,11 +360,6 @@ process_command(struct unixctl_conn *conn, struct jsonrpc_msg *request)
>                            " \"%s\" (supported: %d, requested: %d)",
>                            request->method, ovs_output_fmt_to_string(conn->fmt),
>                            command->output_fmts, conn->fmt);
> -    } else if (conn->fmt != OVS_OUTPUT_FMT_TEXT) {
> -        /* FIXME: Remove this check once output format will be passed to the
> -         *        command handler below. */
> -        error = xasprintf("output format \"%s\" has not been implemented yet",
> -                          ovs_output_fmt_to_string(conn->fmt));
>      } else {
>          struct svec argv = SVEC_EMPTY_INITIALIZER;
>          int  i;
> @@ -397,10 +376,8 @@ process_command(struct unixctl_conn *conn, struct jsonrpc_msg *request)
>          svec_terminate(&argv);
>
>          if (!error) {
> -            /* FIXME: Output format will be passed as 'fmt' to the command in
> -             *        later patch. */
> -            command->cb(conn, argv.n, (const char **) argv.names,
> -                        /* conn->fmt, */ command->aux);
> +            command->cb(conn, argv.n, (const char **) argv.names, conn->fmt,
> +                        command->aux);
>          }
>
>          svec_destroy(&argv);
> diff --git a/lib/unixctl.h b/lib/unixctl.h
> index 8200b9e11..35ef6a548 100644
> --- a/lib/unixctl.h
> +++ b/lib/unixctl.h
> @@ -43,20 +43,12 @@ int unixctl_client_transact(struct jsonrpc *client,
>
>  /* Command registration. */
>  struct unixctl_conn;
> -/* FIXME: Output format will be passed as 'fmt' to the command in later patch.
> - */
>  typedef void unixctl_cb_func(struct unixctl_conn *,
>                               int argc, const char *argv[],
> -                             /* enum ovs_output_fmt fmt, */ void *aux);
> -/* FIXME: unixctl_command_register() will be replaced with
> - *        unixctl_command_register_fmt() in a later patch of this series.  It
> - *        is kept temporarily to reduce the amount of changes in this patch. */
> +                             enum ovs_output_fmt fmt, void *aux);
>  void unixctl_command_register(const char *name, const char *usage,
> -                              int min_args, int max_args,
> +                              int min_args, int max_args, int output_fmts,
>                                unixctl_cb_func *cb, void *aux);
> -void unixctl_command_register_fmt(const char *name, const char *usage,
> -                                  int min_args, int max_args, int output_fmts,
> -                                  unixctl_cb_func *cb, void *aux);
>  void unixctl_command_reply_error(struct unixctl_conn *, const char *error);
>  void unixctl_command_reply(struct unixctl_conn *, const char *body);
>  void unixctl_command_reply_json(struct unixctl_conn *,
> diff --git a/lib/vlog.c b/lib/vlog.c
> index b2653142f..a8ab97b1b 100644
> --- a/lib/vlog.c
> +++ b/lib/vlog.c
> @@ -689,6 +689,7 @@ vlog_facility_exists(const char* facility, int *value)
>
>  static void
>  vlog_unixctl_set(struct unixctl_conn *conn, int argc, const char *argv[],
> +                 enum ovs_output_fmt fmt OVS_UNUSED,
>                   void *aux OVS_UNUSED)
>  {
>      int i;
> @@ -710,7 +711,8 @@ vlog_unixctl_set(struct unixctl_conn *conn, int argc, const char *argv[],
>
>  static void
>  vlog_unixctl_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                  const char *argv[] OVS_UNUSED,
> +                  enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      char *msg = vlog_get_levels();
>      unixctl_command_reply(conn, msg);
> @@ -719,7 +721,9 @@ vlog_unixctl_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  vlog_unixctl_list_pattern(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                          const char *argv[] OVS_UNUSED,
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
> +                          void *aux OVS_UNUSED)
>  {
>      char *msg;
>
> @@ -730,7 +734,9 @@ vlog_unixctl_list_pattern(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  vlog_unixctl_reopen(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                    const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                    const char *argv[] OVS_UNUSED,
> +                    enum ovs_output_fmt fmt OVS_UNUSED,
> +                    void *aux OVS_UNUSED)
>  {
>      bool has_log_file;
>
> @@ -752,7 +758,9 @@ vlog_unixctl_reopen(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  vlog_unixctl_close(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                   const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                   const char *argv[] OVS_UNUSED,
> +                   enum ovs_output_fmt fmt OVS_UNUSED,
> +                   void *aux OVS_UNUSED)
>  {
>      ovs_mutex_lock(&log_file_mutex);
>      if (log_fd >= 0) {
> @@ -811,14 +819,18 @@ set_rate_limits(struct unixctl_conn *conn, int argc,
>
>  static void
>  vlog_enable_rate_limit(struct unixctl_conn *conn, int argc,
> -                       const char *argv[], void *aux OVS_UNUSED)
> +                       const char *argv[],
> +                       enum ovs_output_fmt fmt OVS_UNUSED,
> +                       void *aux OVS_UNUSED)
>  {
>      set_rate_limits(conn, argc, argv, true);
>  }
>
>  static void
>  vlog_disable_rate_limit(struct unixctl_conn *conn, int argc,
> -                       const char *argv[], void *aux OVS_UNUSED)
> +                        const char *argv[],
> +                        enum ovs_output_fmt fmt OVS_UNUSED,
> +                        void *aux OVS_UNUSED)
>  {
>      set_rate_limits(conn, argc, argv, false);
>  }
> @@ -860,20 +872,24 @@ vlog_init(void)
>              free(s);
>          }
>
> -        unixctl_command_register(
> -            "vlog/set", "{spec | PATTERN:destination:pattern}",
> -            0, INT_MAX, vlog_unixctl_set, NULL);
> -        unixctl_command_register("vlog/list", "", 0, 0, vlog_unixctl_list,
> -                                 NULL);
> +        unixctl_command_register("vlog/set",
> +                                 "{spec | PATTERN:destination:pattern}",
> +                                 0, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +                                 vlog_unixctl_set, NULL);
> +        unixctl_command_register("vlog/list", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                                 vlog_unixctl_list, NULL);
>          unixctl_command_register("vlog/list-pattern", "", 0, 0,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   vlog_unixctl_list_pattern, NULL);
>          unixctl_command_register("vlog/enable-rate-limit", "[module]...",
> -                                 0, INT_MAX, vlog_enable_rate_limit, NULL);
> +                                 0, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +                                 vlog_enable_rate_limit, NULL);
>          unixctl_command_register("vlog/disable-rate-limit", "[module]...",
> -                                 0, INT_MAX, vlog_disable_rate_limit, NULL);
> -        unixctl_command_register("vlog/reopen", "", 0, 0,
> +                                 0, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +                                 vlog_disable_rate_limit, NULL);
> +        unixctl_command_register("vlog/reopen", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                                   vlog_unixctl_reopen, NULL);
> -        unixctl_command_register("vlog/close", "", 0, 0,
> +        unixctl_command_register("vlog/close", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                                   vlog_unixctl_close, NULL);
>
>          ovs_rwlock_rdlock(&pattern_rwlock);
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index cfdf44f85..fcab5f932 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -1472,6 +1472,7 @@ bond_lookup_member(struct bond *bond, const char *member_name)
>  static void
>  bond_unixctl_list(struct unixctl_conn *conn,
>                    int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
>                    void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -1617,6 +1618,7 @@ bond_print_details(struct ds *ds, const struct bond *bond)
>  static void
>  bond_unixctl_show(struct unixctl_conn *conn,
>                    int argc, const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
>                    void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -1648,6 +1650,7 @@ out:
>  static void
>  bond_unixctl_migrate(struct unixctl_conn *conn,
>                       int argc OVS_UNUSED, const char *argv[],
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
>                       void *aux OVS_UNUSED)
>  {
>      const char *bond_s = argv[1];
> @@ -1701,6 +1704,7 @@ out:
>  static void
>  bond_unixctl_set_active_member(struct unixctl_conn *conn,
>                                 int argc OVS_UNUSED, const char *argv[],
> +                               enum ovs_output_fmt fmt OVS_UNUSED,
>                                 void *aux OVS_UNUSED)
>  {
>      const char *bond_s = argv[1];
> @@ -1773,6 +1777,7 @@ out:
>  static void
>  bond_unixctl_enable_member(struct unixctl_conn *conn,
>                             int argc OVS_UNUSED, const char *argv[],
> +                           enum ovs_output_fmt fmt OVS_UNUSED,
>                             void *aux OVS_UNUSED)
>  {
>      enable_member(conn, argv, true);
> @@ -1781,6 +1786,7 @@ bond_unixctl_enable_member(struct unixctl_conn *conn,
>  static void
>  bond_unixctl_disable_member(struct unixctl_conn *conn,
>                              int argc OVS_UNUSED, const char *argv[],
> +                            enum ovs_output_fmt fmt OVS_UNUSED,
>                              void *aux OVS_UNUSED)
>  {
>      enable_member(conn, argv, false);
> @@ -1788,6 +1794,7 @@ bond_unixctl_disable_member(struct unixctl_conn *conn,
>
>  static void
>  bond_unixctl_hash(struct unixctl_conn *conn, int argc, const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
>                    void *aux OVS_UNUSED)
>  {
>      const char *mac_s = argv[1];
> @@ -1831,27 +1838,34 @@ bond_unixctl_hash(struct unixctl_conn *conn, int argc, const char *argv[],
>  void
>  bond_init(void)
>  {
> -    unixctl_command_register("bond/list", "", 0, 0, bond_unixctl_list, NULL);
> -    unixctl_command_register("bond/show", "[port]", 0, 1, bond_unixctl_show,
> -                             NULL);
> +    unixctl_command_register("bond/list", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             bond_unixctl_list, NULL);
> +    unixctl_command_register("bond/show", "[port]", 0, 1, OVS_OUTPUT_FMT_TEXT,
> +                             bond_unixctl_show, NULL);
>      unixctl_command_register("bond/migrate", "port hash member", 3, 3,
> -                             bond_unixctl_migrate, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bond_unixctl_migrate, NULL);
>      unixctl_command_register("bond/set-active-member", "port member", 2, 2,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               bond_unixctl_set_active_member, NULL);
>      unixctl_command_register("bond/enable-member", "port member", 2, 2,
> -                             bond_unixctl_enable_member, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bond_unixctl_enable_member,
> +                             NULL);
>      unixctl_command_register("bond/disable-member", "port member", 2, 2,
> -                             bond_unixctl_disable_member, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bond_unixctl_disable_member,
> +                             NULL);
>      unixctl_command_register("bond/hash", "mac [vlan] [basis]", 1, 3,
> -                             bond_unixctl_hash, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bond_unixctl_hash, NULL);
>
>      /* Backward-compatibility command names. */
>      unixctl_command_register("bond/set-active-slave", NULL, 2, 2,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               bond_unixctl_set_active_member, NULL);
>      unixctl_command_register("bond/enable-slave", NULL, 2, 2,
> -                             bond_unixctl_enable_member, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bond_unixctl_enable_member,
> +                             NULL);
>      unixctl_command_register("bond/disable-slave", NULL, 2, 2,
> -                             bond_unixctl_disable_member, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bond_unixctl_disable_member,
> +                             NULL);
>  }
>  
>  static void
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index b86e7fe07..77b424452 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -471,6 +471,7 @@ free_ct_states(struct ovs_list *ct_states)
>
>  static void
>  ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
>                        void *aux OVS_UNUSED)
>  {
>      struct ofproto_dpif *ofproto;
> @@ -500,7 +501,9 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
>
>  static void
>  ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
> -                              const char *argv[], void *aux OVS_UNUSED)
> +                              const char *argv[],
> +                              enum ovs_output_fmt fmt OVS_UNUSED,
> +                              void *aux OVS_UNUSED)
>  {
>      enum ofputil_protocol usable_protocols;
>      struct ofproto_dpif *ofproto;
> @@ -870,12 +873,13 @@ ofproto_dpif_trace_init(void)
>      unixctl_command_register(
>          "ofproto/trace",
>          "{[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
> -        "[-generate|packet]", 1, INT_MAX, ofproto_unixctl_trace, NULL);
> +        "[-generate|packet]", 1, INT_MAX, OVS_OUTPUT_FMT_TEXT,
> +        ofproto_unixctl_trace, NULL);
>      unixctl_command_register(
>          "ofproto/trace-packet-out",
>          "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
>          "[-generate|packet] actions",
> -        2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
> +        2, INT_MAX, OVS_OUTPUT_FMT_TEXT, ofproto_unixctl_trace_actions, NULL);
>  }
>
>  void
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index b5cbeed87..764178578 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -357,25 +357,38 @@ static void revalidator_pause(struct revalidator *);
>  static void revalidator_sweep(struct revalidator *);
>  static void revalidator_purge(struct revalidator *);
>  static void upcall_unixctl_show(struct unixctl_conn *conn, int argc,
> -                                const char *argv[], void *aux);
> +                                const char *argv[],
> +                                enum ovs_output_fmt fmt,
> +                                void *aux);
>  static void upcall_unixctl_disable_megaflows(struct unixctl_conn *, int argc,
> -                                             const char *argv[], void *aux);
> +                                             const char *argv[],
> +                                             enum ovs_output_fmt fmt,
> +                                             void *aux);
>  static void upcall_unixctl_enable_megaflows(struct unixctl_conn *, int argc,
> -                                            const char *argv[], void *aux);
> +                                            const char *argv[],
> +                                            enum ovs_output_fmt fmt,
> +                                            void *aux);
>  static void upcall_unixctl_disable_ufid(struct unixctl_conn *, int argc,
> -                                              const char *argv[], void *aux);
> +                                        const char *argv[],
> +                                        enum ovs_output_fmt fmt, void *aux);
>  static void upcall_unixctl_enable_ufid(struct unixctl_conn *, int argc,
> -                                             const char *argv[], void *aux);
> +                                       const char *argv[],
> +                                       enum ovs_output_fmt fmt, void *aux);
>  static void upcall_unixctl_set_flow_limit(struct unixctl_conn *conn, int argc,
> -                                            const char *argv[], void *aux);
> +                                          const char *argv[],
> +                                          enum ovs_output_fmt fmt, void *aux);
>  static void upcall_unixctl_dump_wait(struct unixctl_conn *conn, int argc,
> -                                     const char *argv[], void *aux);
> +                                     const char *argv[],
> +                                     enum ovs_output_fmt fmt, void *aux);
>  static void upcall_unixctl_purge(struct unixctl_conn *conn, int argc,
> -                                 const char *argv[], void *aux);
> +                                 const char *argv[], enum ovs_output_fmt fmt,
> +                                 void *aux);
>  static void upcall_unixctl_pause(struct unixctl_conn *conn, int argc,
> -                                 const char *argv[], void *aux);
> +                                 const char *argv[], enum ovs_output_fmt fmt,
> +                                 void *aux);
>  static void upcall_unixctl_resume(struct unixctl_conn *conn, int argc,
> -                                  const char *argv[], void *aux);
> +                                  const char *argv[], enum ovs_output_fmt fmt,
> +                                  void *aux);
>
>  static struct udpif_key *ukey_create_from_upcall(struct upcall *,
>                                                   struct flow_wildcards *);
> @@ -433,26 +446,36 @@ udpif_init(void)
>  {
>      static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>      if (ovsthread_once_start(&once)) {
> -        unixctl_command_register("upcall/show", "", 0, 0, upcall_unixctl_show,
> +        unixctl_command_register("upcall/show", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                                 upcall_unixctl_show,
>                                   NULL);
>          unixctl_command_register("upcall/disable-megaflows", "", 0, 0,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   upcall_unixctl_disable_megaflows, NULL);
>          unixctl_command_register("upcall/enable-megaflows", "", 0, 0,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   upcall_unixctl_enable_megaflows, NULL);
>          unixctl_command_register("upcall/disable-ufid", "", 0, 0,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   upcall_unixctl_disable_ufid, NULL);
>          unixctl_command_register("upcall/enable-ufid", "", 0, 0,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   upcall_unixctl_enable_ufid, NULL);
>          unixctl_command_register("upcall/set-flow-limit", "flow-limit-number",
> -                                 1, 1, upcall_unixctl_set_flow_limit, NULL);
> +                                 1, 1, OVS_OUTPUT_FMT_TEXT,
> +                                 upcall_unixctl_set_flow_limit, NULL);
>          unixctl_command_register("revalidator/wait", "", 0, 0,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   upcall_unixctl_dump_wait, NULL);
>          unixctl_command_register("revalidator/purge", "", 0, 0,
> -                                 upcall_unixctl_purge, NULL);
> +                                 OVS_OUTPUT_FMT_TEXT, upcall_unixctl_purge,
> +                                 NULL);
>          unixctl_command_register("revalidator/pause", NULL, 0, 0,
> -                                 upcall_unixctl_pause, NULL);
> +                                 OVS_OUTPUT_FMT_TEXT, upcall_unixctl_pause,
> +                                 NULL);
>          unixctl_command_register("revalidator/resume", NULL, 0, 0,
> -                                 upcall_unixctl_resume, NULL);
> +                                 OVS_OUTPUT_FMT_TEXT, upcall_unixctl_resume,
> +                                 NULL);
>          ovsthread_once_done(&once);
>      }
>  }
> @@ -3078,7 +3101,9 @@ dp_purge_cb(void *aux, unsigned pmd_id)
>  
>  static void
>  upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                    const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                    const char *argv[] OVS_UNUSED,
> +                    enum ovs_output_fmt fmt OVS_UNUSED,
> +                    void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      uint64_t n_offloaded_flows;
> @@ -3132,6 +3157,7 @@ static void
>  upcall_unixctl_disable_megaflows(struct unixctl_conn *conn,
>                                   int argc OVS_UNUSED,
>                                   const char *argv[] OVS_UNUSED,
> +                                 enum ovs_output_fmt fmt OVS_UNUSED,
>                                   void *aux OVS_UNUSED)
>  {
>      atomic_store_relaxed(&enable_megaflows, false);
> @@ -3147,6 +3173,7 @@ static void
>  upcall_unixctl_enable_megaflows(struct unixctl_conn *conn,
>                                  int argc OVS_UNUSED,
>                                  const char *argv[] OVS_UNUSED,
> +                                enum ovs_output_fmt fmt OVS_UNUSED,
>                                  void *aux OVS_UNUSED)
>  {
>      atomic_store_relaxed(&enable_megaflows, true);
> @@ -3160,7 +3187,9 @@ upcall_unixctl_enable_megaflows(struct unixctl_conn *conn,
>   * documented in the man page. */
>  static void
>  upcall_unixctl_disable_ufid(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                           const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                            const char *argv[] OVS_UNUSED,
> +                            enum ovs_output_fmt fmt OVS_UNUSED,
> +                            void *aux OVS_UNUSED)
>  {
>      atomic_store_relaxed(&enable_ufid, false);
>      unixctl_command_reply(conn, "Datapath dumping tersely using UFID disabled");
> @@ -3172,7 +3201,9 @@ upcall_unixctl_disable_ufid(struct unixctl_conn *conn, int argc OVS_UNUSED,
>   * in the man page. */
>  static void
>  upcall_unixctl_enable_ufid(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                           const char *argv[] OVS_UNUSED,
> +                           enum ovs_output_fmt fmt OVS_UNUSED,
> +                           void *aux OVS_UNUSED)
>  {
>      atomic_store_relaxed(&enable_ufid, true);
>      unixctl_command_reply(conn, "Datapath dumping tersely using UFID enabled "
> @@ -3187,6 +3218,7 @@ static void
>  upcall_unixctl_set_flow_limit(struct unixctl_conn *conn,
>                                int argc OVS_UNUSED,
>                                const char *argv[],
> +                              enum ovs_output_fmt fmt OVS_UNUSED,
>                                void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -3205,6 +3237,7 @@ static void
>  upcall_unixctl_dump_wait(struct unixctl_conn *conn,
>                           int argc OVS_UNUSED,
>                           const char *argv[] OVS_UNUSED,
> +                         enum ovs_output_fmt fmt OVS_UNUSED,
>                           void *aux OVS_UNUSED)
>  {
>      if (ovs_list_is_singleton(&all_udpifs)) {
> @@ -3223,7 +3256,9 @@ upcall_unixctl_dump_wait(struct unixctl_conn *conn,
>
>  static void
>  upcall_unixctl_purge(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                     const char *argv[] OVS_UNUSED,
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      struct udpif *udpif;
>
> @@ -3247,7 +3282,9 @@ upcall_unixctl_purge(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  upcall_unixctl_pause(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                     const char *argv[] OVS_UNUSED,
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      struct udpif *udpif;
>
> @@ -3259,7 +3296,9 @@ upcall_unixctl_pause(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  upcall_unixctl_resume(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                      const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                      const char *argv[] OVS_UNUSED,
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
>  {
>      struct udpif *udpif;
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index f59d69c4d..46d6c5014 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5892,7 +5892,9 @@ ofproto_dpif_lookup_by_uuid(const struct uuid *uuid)
>
>  static void
>  ofproto_unixctl_fdb_flush(struct unixctl_conn *conn, int argc,
> -                          const char *argv[], void *aux OVS_UNUSED)
> +                          const char *argv[],
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
> +                          void *aux OVS_UNUSED)
>  {
>      struct ofproto_dpif *ofproto;
>
> @@ -5919,7 +5921,9 @@ ofproto_unixctl_fdb_flush(struct unixctl_conn *conn, int argc,
>
>  static void
>  ofproto_unixctl_mcast_snooping_flush(struct unixctl_conn *conn, int argc,
> -                                     const char *argv[], void *aux OVS_UNUSED)
> +                                     const char *argv[],
> +                                     enum ovs_output_fmt fmt OVS_UNUSED,
> +                                     void *aux OVS_UNUSED)
>  {
>      struct ofproto_dpif *ofproto;
>
> @@ -5957,7 +5961,9 @@ ofbundle_get_a_port(const struct ofbundle *bundle)
>
>  static void
>  ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                         const char *argv[], void *aux OVS_UNUSED)
> +                         const char *argv[],
> +                         enum ovs_output_fmt fmt OVS_UNUSED,
> +                         void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      const struct ofproto_dpif *ofproto;
> @@ -5993,7 +5999,9 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ofproto_unixctl_fdb_add(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                         const char *argv[], void *aux OVS_UNUSED)
> +                        const char *argv[],
> +                        enum ovs_output_fmt fmt OVS_UNUSED,
> +                        void *aux OVS_UNUSED)
>  {
>      const struct ofproto_dpif *ofproto;
>      const struct mac_entry *mac_entry;
> @@ -6058,7 +6066,9 @@ ofproto_unixctl_fdb_add(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ofproto_unixctl_fdb_delete(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                           const char *argv[], void *aux OVS_UNUSED)
> +                           const char *argv[],
> +                           enum ovs_output_fmt fmt OVS_UNUSED,
> +                           void *aux OVS_UNUSED)
>  {
>      const struct ofproto_dpif *ofproto;
>      const char *br_name = argv[1];
> @@ -6084,7 +6094,9 @@ ofproto_unixctl_fdb_delete(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ofproto_unixctl_fdb_stats_clear(struct unixctl_conn *conn, int argc,
> -                                const char *argv[], void *aux OVS_UNUSED)
> +                                const char *argv[],
> +                                enum ovs_output_fmt fmt OVS_UNUSED,
> +                                void *aux OVS_UNUSED)
>  {
>      struct ofproto_dpif *ofproto;
>
> @@ -6111,7 +6123,9 @@ ofproto_unixctl_fdb_stats_clear(struct unixctl_conn *conn, int argc,
>
>  static void
>  ofproto_unixctl_fdb_stats_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                               const char *argv[], void *aux OVS_UNUSED)
> +                               const char *argv[],
> +                               enum ovs_output_fmt fmt OVS_UNUSED,
> +                               void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      const struct ofproto_dpif *ofproto;
> @@ -6152,6 +6166,7 @@ static void
>  ofproto_unixctl_mcast_snooping_show(struct unixctl_conn *conn,
>                                      int argc OVS_UNUSED,
>                                      const char *argv[],
> +                                    enum ovs_output_fmt fmt OVS_UNUSED,
>                                      void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -6227,6 +6242,7 @@ get_ofprotos(struct shash *ofproto_shash)
>  static void
>  ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                                const char *argv[] OVS_UNUSED,
> +                              enum ovs_output_fmt fmt OVS_UNUSED,
>                                void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -6473,7 +6489,9 @@ dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
>
>  static void
>  ofproto_unixctl_dpif_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                          const char *argv[] OVS_UNUSED,
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
> +                          void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      const struct shash_node **backers;
> @@ -6492,6 +6510,7 @@ ofproto_unixctl_dpif_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  static void
>  ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
>                                  int argc OVS_UNUSED, const char *argv[],
> +                                enum ovs_output_fmt fmt OVS_UNUSED,
>                                  void *aux OVS_UNUSED)
>  {
>      const struct ofproto_dpif *ofproto;
> @@ -6586,6 +6605,7 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
>  static void
>  ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
>                                        int argc, const char *argv[],
> +                                      enum ovs_output_fmt fmt OVS_UNUSED,
>                                        void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -6605,6 +6625,7 @@ ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
>  static void
>  ofproto_unixctl_dpif_set_dp_features(struct unixctl_conn *conn,
>                                       int argc, const char *argv[],
> +                                     enum ovs_output_fmt fmt OVS_UNUSED,
>                                       void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -6641,31 +6662,40 @@ ofproto_unixctl_init(void)
>      registered = true;
>
>      unixctl_command_register("fdb/add", "bridge port vlan mac", 4, 4,
> -                             ofproto_unixctl_fdb_add, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, ofproto_unixctl_fdb_add,
> +                             NULL);
>      unixctl_command_register("fdb/del", "bridge vlan mac", 3, 3,
> -                             ofproto_unixctl_fdb_delete, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, ofproto_unixctl_fdb_delete,
> +                             NULL);
>      unixctl_command_register("fdb/flush", "[bridge]", 0, 1,
> -                             ofproto_unixctl_fdb_flush, NULL);
> -    unixctl_command_register("fdb/show", "bridge", 1, 1,
> +                             OVS_OUTPUT_FMT_TEXT, ofproto_unixctl_fdb_flush,
> +                             NULL);
> +    unixctl_command_register("fdb/show", "bridge", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_fdb_show, NULL);
>      unixctl_command_register("fdb/stats-clear", "[bridge]", 0, 1,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_fdb_stats_clear, NULL);
>      unixctl_command_register("fdb/stats-show", "bridge", 1, 1,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_fdb_stats_show, NULL);
>      unixctl_command_register("mdb/flush", "[bridge]", 0, 1,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_mcast_snooping_flush, NULL);
> -    unixctl_command_register("mdb/show", "bridge", 1, 1,
> +    unixctl_command_register("mdb/show", "bridge", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_mcast_snooping_show, NULL);
> -    unixctl_command_register("dpif/dump-dps", "", 0, 0,
> +    unixctl_command_register("dpif/dump-dps", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_dpif_dump_dps, NULL);
> -    unixctl_command_register("dpif/show", "", 0, 0, ofproto_unixctl_dpif_show,
> -                             NULL);
> +    unixctl_command_register("dpif/show", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ofproto_unixctl_dpif_show, NULL);
>      unixctl_command_register("dpif/show-dp-features", "bridge", 1, 1,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_dpif_show_dp_features, NULL);
>      unixctl_command_register("dpif/dump-flows",
>                               "[-m] [--names | --no-names] bridge", 1, INT_MAX,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_dpif_dump_flows, NULL);
> -    unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
> +    unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_dpif_set_dp_features, NULL);
>  }
>  
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 122a06f30..3eea543dd 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6389,7 +6389,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
>      error = ofproto_flow_mod_start(ofproto, &ofm);
>      if (!error) {
>          ofproto_bump_tables_version(ofproto);
> -        error = ofproto_flow_mod_finish(ofproto, &ofm, req);
> +        error = ofproto_flow_mod_finish(ofproto, &ofm, req);
>          ofmonitor_flush(ofproto->connmgr);
>      }
>      ovs_mutex_unlock(&ofproto_mutex);
> @@ -8460,7 +8460,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
>              /* Send error referring to the original message. */
>              ofconn_send_error(ofconn, be->msg, error);
>              error = OFPERR_OFPBFC_MSG_FAILED;
> -
> +
>              /* 2. Revert.  Undo all the changes made above. */
>              LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
>                  if (be->type == OFPTYPE_FLOW_MOD) {
> @@ -9476,7 +9476,9 @@ ofproto_lookup(const char *name)
>
>  static void
>  ofproto_unixctl_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                     const char *argv[] OVS_UNUSED,
> +                     enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *aux OVS_UNUSED)
>  {
>      struct ofproto *ofproto;
>      struct ds results;
> @@ -9498,7 +9500,7 @@ ofproto_unixctl_init(void)
>      }
>      registered = true;
>
> -    unixctl_command_register("ofproto/list", "", 0, 0,
> +    unixctl_command_register("ofproto/list", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                               ofproto_unixctl_list, NULL);
>  }
>
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 80ddee78a..3a7e1594d 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -141,7 +141,7 @@ ofproto_tunnel_init(void)
>      if (ovsthread_once_start(&once)) {
>          fat_rwlock_init(&rwlock);
>          unixctl_command_register("ofproto/list-tunnels", "", 0, 0,
> -                                 tnl_unixctl_list, NULL);
> +                                 OVS_OUTPUT_FMT_TEXT, tnl_unixctl_list, NULL);
>          ovsthread_once_done(&once);
>      }
>  }
> @@ -757,7 +757,7 @@ tnl_port_build_header(const struct ofport_dpif *ofport,
>  static void
>  tnl_unixctl_list(struct unixctl_conn *conn,
>                   int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
> -                 void *aux OVS_UNUSED)
> +                 enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>
> diff --git a/ovsdb/file.c b/ovsdb/file.c
> index 66ef87a1f..1b888ebec 100644
> --- a/ovsdb/file.c
> +++ b/ovsdb/file.c
> @@ -63,6 +63,7 @@ static bool use_column_diff = true;
>  static void
>  ovsdb_file_column_diff_enable(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                                const char *argv[] OVS_UNUSED,
> +                              enum ovs_output_fmt fmt OVS_UNUSED,
>                                void *arg OVS_UNUSED)
>  {
>      use_column_diff = true;
> @@ -77,7 +78,8 @@ ovsdb_file_column_diff_disable(void)
>      }
>      use_column_diff = false;
>      unixctl_command_register("ovsdb/file/column-diff-enable", "",
> -                             0, 0, ovsdb_file_column_diff_enable, NULL);
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_file_column_diff_enable, NULL);
>  }
>
>  static struct ovsdb_error *
> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> index 7249805ba..50de20ee1 100644
> --- a/ovsdb/ovsdb-client.c
> +++ b/ovsdb/ovsdb-client.c
> @@ -1253,7 +1253,8 @@ parse_monitor_columns(char *arg, const char *server, const char *database,
>
>  static void
>  ovsdb_client_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[] OVS_UNUSED, void *exiting_)
> +                  const char *argv[] OVS_UNUSED,
> +                  enum ovs_output_fmt fmt OVS_UNUSED, void *exiting_)
>  {
>      bool *exiting = exiting_;
>      *exiting = true;
> @@ -1262,7 +1263,8 @@ ovsdb_client_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_client_block(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                   const char *argv[] OVS_UNUSED, void *blocked_)
> +                   const char *argv[] OVS_UNUSED,
> +                   enum ovs_output_fmt fmt OVS_UNUSED, void *blocked_)
>  {
>      bool *blocked = blocked_;
>
> @@ -1276,7 +1278,8 @@ ovsdb_client_block(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_client_unblock(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[] OVS_UNUSED, void *blocked_)
> +                     const char *argv[] OVS_UNUSED,
> +                     enum ovs_output_fmt fmt OVS_UNUSED, void *blocked_)
>  {
>      bool *blocked = blocked_;
>
> @@ -1290,7 +1293,8 @@ ovsdb_client_unblock(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_client_cond_change(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                     const char *argv[], void *rpc_)
> +                         const char *argv[],
> +                         enum ovs_output_fmt fmt OVS_UNUSED, void *rpc_)
>  {
>      struct jsonrpc *rpc = rpc_;
>      struct json *monitor_cond_update_requests = json_object_create();
> @@ -1404,13 +1408,16 @@ do_monitor__(struct jsonrpc *rpc, const char *database,
>              ovs_fatal(error, "failed to create unixctl server");
>          }
>
> -        unixctl_command_register("exit", "", 0, 0,
> +        unixctl_command_register("exit", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                                   ovsdb_client_exit, &exiting);
>          unixctl_command_register("ovsdb-client/block", "", 0, 0,
> -                                 ovsdb_client_block, &blocked);
> +                                 OVS_OUTPUT_FMT_TEXT,  ovsdb_client_block,

One space too many before ovsdb_client_block

> +                                 &blocked);
>          unixctl_command_register("ovsdb-client/unblock", "", 0, 0,
> -                                 ovsdb_client_unblock, &blocked);
> +                                 OVS_OUTPUT_FMT_TEXT, ovsdb_client_unblock,
> +                                 &blocked);
>          unixctl_command_register("ovsdb-client/cond_change", "TABLE COND", 2, 2,
> +                                 OVS_OUTPUT_FMT_TEXT,
>                                   ovsdb_client_cond_change, rpc);
>      } else {
>          unixctl = NULL;
> @@ -2244,7 +2251,9 @@ create_lock_request(struct ovsdb_client_lock_req *lock_req)
>
>  static void
>  ovsdb_client_lock(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[], void *lock_req_)
> +                  const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
> +                  void *lock_req_)
>  {
>      struct ovsdb_client_lock_req *lock_req = lock_req_;
>      lock_req_init(lock_req, "lock", argv[1]);
> @@ -2253,7 +2262,9 @@ ovsdb_client_lock(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_client_unlock(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                    const char *argv[], void *lock_req_)
> +                    const char *argv[],
> +                    enum ovs_output_fmt fmt OVS_UNUSED,
> +                    void *lock_req_)
>  {
>      struct ovsdb_client_lock_req *lock_req = lock_req_;
>      lock_req_init(lock_req, "unlock", argv[1]);
> @@ -2262,7 +2273,8 @@ ovsdb_client_unlock(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_client_steal(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                   const char *argv[], void *lock_req_)
> +                   const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                   void *lock_req_)
>  {
>      struct ovsdb_client_lock_req *lock_req = lock_req_;
>      lock_req_init(lock_req, "steal", argv[1]);
> @@ -2292,13 +2304,13 @@ do_lock(struct jsonrpc *rpc, const char *method, const char *lock)
>              ovs_fatal(error, "failed to create unixctl server");
>          }
>
> -        unixctl_command_register("unlock", "LOCK", 1, 1,
> +        unixctl_command_register("unlock", "LOCK", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                                    ovsdb_client_unlock, &lock_req);
> -        unixctl_command_register("steal", "LOCK", 1, 1,
> +        unixctl_command_register("steal", "LOCK", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                                    ovsdb_client_steal, &lock_req);
> -        unixctl_command_register("lock", "LOCK", 1, 1,
> +        unixctl_command_register("lock", "LOCK", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                                    ovsdb_client_lock, &lock_req);
> -        unixctl_command_register("exit", "", 0, 0,
> +        unixctl_command_register("exit", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                                   ovsdb_client_exit, &exiting);
>      } else {
>          unixctl = NULL;
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index b51fd42fe..7b831536e 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -819,72 +819,97 @@ main(int argc, char *argv[])
>          VLOG_INFO("%s (Open vSwitch) %s", program_name, VERSION);
>      }
>
> -    unixctl_command_register("exit", "", 0, 0, ovsdb_server_exit, &exiting);
> +    unixctl_command_register("exit", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_server_exit, &exiting);
>      unixctl_command_register("ovsdb-server/compact", "", 0, 1,
> -                             ovsdb_server_compact, &all_dbs);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_compact,
> +                             &all_dbs);
>      unixctl_command_register("ovsdb-server/memory-trim-on-compaction",
> -                             "on|off", 1, 1,
> +                             "on|off", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               ovsdb_server_memory_trim_on_compaction, NULL);
>      unixctl_command_register("ovsdb-server/reconnect", "", 0, 0,
> -                             ovsdb_server_reconnect, jsonrpc);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_reconnect,
> +                             jsonrpc);
>      unixctl_command_register("ovsdb-server/reload", "", 0, 0,
> -                             ovsdb_server_reload, &server_config);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_reload,
> +                             &server_config);
>
>      unixctl_command_register("ovsdb-server/add-remote", "REMOTE", 1, 1,
> -                             ovsdb_server_add_remote, &server_config);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_add_remote,
> +                             &server_config);
>      unixctl_command_register("ovsdb-server/remove-remote", "REMOTE", 1, 1,
> -                             ovsdb_server_remove_remote, &server_config);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_remove_remote,
> +                             &server_config);
>      unixctl_command_register("ovsdb-server/list-remotes", "", 0, 0,
> -                             ovsdb_server_list_remotes, &remotes);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_list_remotes,
> +                             &remotes);
>
>      unixctl_command_register("ovsdb-server/add-db", "DB", 1, 1,
> -                             ovsdb_server_add_database, &server_config);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_add_database,
> +                             &server_config);
>      unixctl_command_register("ovsdb-server/remove-db", "DB", 1, 1,
> -                             ovsdb_server_remove_database, &server_config);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_remove_database,
> +                             &server_config);
>      unixctl_command_register("ovsdb-server/list-dbs", "", 0, 0,
> -                             ovsdb_server_list_databases, &all_dbs);
> +                             OVS_OUTPUT_FMT_TEXT, ovsdb_server_list_databases,
> +                             &all_dbs);
>      unixctl_command_register("ovsdb-server/tlog-set", "DB:TABLE on|off",
> -                             2, 2, ovsdb_server_tlog_set, &all_dbs);
> +                             2, 2, OVS_OUTPUT_FMT_TEXT, ovsdb_server_tlog_set,
> +                             &all_dbs);
>      unixctl_command_register("ovsdb-server/tlog-list", "",
> -                             0, 0, ovsdb_server_tlog_list, &all_dbs);
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_server_tlog_list, &all_dbs);
>      unixctl_command_register("ovsdb-server/perf-counters-show", "", 0, 0,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ovsdb_server_perf_counters_show, NULL);
>      unixctl_command_register("ovsdb-server/perf-counters-clear", "", 0, 0,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ovsdb_server_perf_counters_clear, NULL);
>      unixctl_command_register("ovsdb-server/set-active-ovsdb-server", "", 1, 1,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ovsdb_server_set_active_ovsdb_server,
>                               &server_config);
>      unixctl_command_register("ovsdb-server/get-active-ovsdb-server", "", 0, 0,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ovsdb_server_get_active_ovsdb_server,
>                               &server_config);
>      unixctl_command_register("ovsdb-server/connect-active-ovsdb-server", "",
> -                             0, 0, ovsdb_server_connect_active_ovsdb_server,
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_server_connect_active_ovsdb_server,
>                               &server_config);
>      unixctl_command_register("ovsdb-server/disconnect-active-ovsdb-server", "",
> -                             0, 0, ovsdb_server_disconnect_active_ovsdb_server,
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_server_disconnect_active_ovsdb_server,
>                               &server_config);
>      unixctl_command_register(
>          "ovsdb-server/set-active-ovsdb-server-probe-interval", "", 1, 1,
> +        OVS_OUTPUT_FMT_TEXT,
>          ovsdb_server_set_active_ovsdb_server_probe_interval, &server_config);
>      unixctl_command_register(
>          "ovsdb-server/set-relay-source-probe-interval", "", 1, 1,
> +        OVS_OUTPUT_FMT_TEXT,
>          ovsdb_server_set_relay_source_interval, &server_config);
>      unixctl_command_register("ovsdb-server/set-sync-exclude-tables", "",
> -                             0, 1, ovsdb_server_set_sync_exclude_tables,
> +                             0, 1, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_server_set_sync_exclude_tables,
>                               &server_config);
>      unixctl_command_register("ovsdb-server/get-sync-exclude-tables", "",
> -                             0, 0, ovsdb_server_get_sync_exclude_tables,
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_server_get_sync_exclude_tables,
>                               &server_config);
>      unixctl_command_register("ovsdb-server/sync-status", "",
> -                             0, 0, ovsdb_server_get_sync_status,
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_server_get_sync_status,
>                               &server_config);
>      unixctl_command_register("ovsdb-server/get-db-storage-status", "DB", 1, 1,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ovsdb_server_get_db_storage_status,
>                               &server_config);
>
>      /* Simulate the behavior of OVS release prior to version 2.5 that
>       * does not support the monitor_cond method.  */
>      unixctl_command_register("ovsdb-server/disable-monitor-cond", "", 0, 0,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               ovsdb_server_disable_monitor_cond, jsonrpc);
>
>      main_loop(&server_config, jsonrpc, &all_dbs, unixctl, &remotes,
> @@ -1843,6 +1868,7 @@ check_config_file_on_unixctl(struct unixctl_conn *conn)
>  static void
>  ovsdb_server_set_active_ovsdb_server(struct unixctl_conn *conn,
>                                       int argc OVS_UNUSED, const char *argv[],
> +                                     enum ovs_output_fmt fmt OVS_UNUSED,
>                                       void *config_)
>  {
>      struct server_config *config = config_;
> @@ -1873,6 +1899,7 @@ static void
>  ovsdb_server_get_active_ovsdb_server(struct unixctl_conn *conn,
>                                       int argc OVS_UNUSED,
>                                       const char *argv[] OVS_UNUSED,
> +                                     enum ovs_output_fmt fmt OVS_UNUSED,
>                                       void *config_ )
>  {
>      struct server_config *config = config_;
> @@ -1884,6 +1911,7 @@ static void
>  ovsdb_server_connect_active_ovsdb_server(struct unixctl_conn *conn,
>                                           int argc OVS_UNUSED,
>                                           const char *argv[] OVS_UNUSED,
> +                                         enum ovs_output_fmt fmt OVS_UNUSED,
>                                           void *config_)
>  {
>      struct server_config *config = config_;
> @@ -1932,6 +1960,7 @@ static void
>  ovsdb_server_disconnect_active_ovsdb_server(struct unixctl_conn *conn,
>                                              int argc OVS_UNUSED,
>                                              const char *argv[] OVS_UNUSED,
> +                                            enum ovs_output_fmt fmt OVS_UNUSED,
>                                              void *config_)
>  {
>      struct server_config *config = config_;
> @@ -1956,9 +1985,11 @@ ovsdb_server_disconnect_active_ovsdb_server(struct unixctl_conn *conn,
>
>  static void
>  ovsdb_server_set_active_ovsdb_server_probe_interval(struct unixctl_conn *conn,
> -                                                   int argc OVS_UNUSED,
> -                                                   const char *argv[],
> -                                                   void *config_)
> +                                                    int argc OVS_UNUSED,
> +                                                    const char *argv[],
> +                                                    enum ovs_output_fmt fmt
> +                                                    OVS_UNUSED,
> +                                                    void *config_)
>  {
>      struct server_config *config = config_;
>      struct shash_node *node;
> @@ -2000,6 +2031,7 @@ static void
>  ovsdb_server_set_relay_source_interval(struct unixctl_conn *conn,
>                                         int argc OVS_UNUSED,
>                                         const char *argv[],
> +                                       enum ovs_output_fmt fmt OVS_UNUSED,
>                                         void *config_)
>  {
>      struct server_config *config = config_;
> @@ -2037,6 +2069,7 @@ static void
>  ovsdb_server_set_sync_exclude_tables(struct unixctl_conn *conn,
>                                       int argc OVS_UNUSED,
>                                       const char *argv[],
> +                                     enum ovs_output_fmt fmt OVS_UNUSED,
>                                       void *config_)
>  {
>      struct server_config *config = config_;
> @@ -2082,6 +2115,7 @@ static void
>  ovsdb_server_get_sync_exclude_tables(struct unixctl_conn *conn,
>                                       int argc OVS_UNUSED,
>                                       const char *argv[] OVS_UNUSED,
> +                                     enum ovs_output_fmt fmt OVS_UNUSED,
>                                       void *config_)
>  {
>      struct server_config *config = config_;
> @@ -2092,6 +2126,7 @@ ovsdb_server_get_sync_exclude_tables(struct unixctl_conn *conn,
>  static void
>  ovsdb_server_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                    const char *argv[] OVS_UNUSED,
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
>                    void *exiting_)
>  {
>      bool *exiting = exiting_;
> @@ -2102,6 +2137,7 @@ ovsdb_server_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  static void
>  ovsdb_server_perf_counters_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                                  const char *argv[] OVS_UNUSED,
> +                                enum ovs_output_fmt fmt OVS_UNUSED,
>                                  void *arg_ OVS_UNUSED)
>  {
>      char *s = perf_counters_to_string();
> @@ -2113,6 +2149,7 @@ ovsdb_server_perf_counters_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  static void
>  ovsdb_server_perf_counters_clear(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                                   const char *argv[] OVS_UNUSED,
> +                                 enum ovs_output_fmt fmt OVS_UNUSED,
>                                   void *arg_ OVS_UNUSED)
>  {
>      perf_counters_clear();
> @@ -2126,6 +2163,7 @@ static void
>  ovsdb_server_disable_monitor_cond(struct unixctl_conn *conn,
>                                    int argc OVS_UNUSED,
>                                    const char *argv[] OVS_UNUSED,
> +                                  enum ovs_output_fmt fmt OVS_UNUSED,
>                                    void *jsonrpc_)
>  {
>      struct ovsdb_jsonrpc_server *jsonrpc = jsonrpc_;
> @@ -2138,7 +2176,8 @@ ovsdb_server_disable_monitor_cond(struct unixctl_conn *conn,
>
>  static void
>  ovsdb_server_compact(struct unixctl_conn *conn, int argc,
> -                     const char *argv[], void *dbs_)
> +                     const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                     void *dbs_)
>  {
>      const char *db_name = argc < 2 ? NULL : argv[1];
>      struct shash *all_dbs = dbs_;
> @@ -2201,6 +2240,7 @@ static void
>  ovsdb_server_memory_trim_on_compaction(struct unixctl_conn *conn,
>                                         int argc OVS_UNUSED,
>                                         const char *argv[],
> +                                       enum ovs_output_fmt fmt OVS_UNUSED,
>                                         void *arg OVS_UNUSED)
>  {
>      bool old_trim_memory = trim_memory;
> @@ -2232,7 +2272,9 @@ ovsdb_server_memory_trim_on_compaction(struct unixctl_conn *conn,
>   * connections and reconnect. */
>  static void
>  ovsdb_server_reconnect(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                       const char *argv[] OVS_UNUSED, void *jsonrpc_)
> +                       const char *argv[] OVS_UNUSED,
> +                       enum ovs_output_fmt fmt OVS_UNUSED,
> +                       void *jsonrpc_)
>  {
>      struct ovsdb_jsonrpc_server *jsonrpc = jsonrpc_;
>      ovsdb_jsonrpc_server_reconnect(
> @@ -2244,7 +2286,9 @@ ovsdb_server_reconnect(struct unixctl_conn *conn, int argc OVS_UNUSED,
>   * 'config_file_path', read it and sync the runtime configuration with it. */
>  static void
>  ovsdb_server_reload(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                    const char *argv[] OVS_UNUSED, void *config_)
> +                    const char *argv[] OVS_UNUSED,
> +                    enum ovs_output_fmt fmt OVS_UNUSED,
> +                    void *config_)
>  {
>      struct server_config *config = config_;
>
> @@ -2266,7 +2310,9 @@ ovsdb_server_reload(struct unixctl_conn *conn, int argc OVS_UNUSED,
>   * ovsdb-server services. */
>  static void
>  ovsdb_server_add_remote(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                        const char *argv[], void *config_)
> +                        const char *argv[],
> +                        enum ovs_output_fmt fmt OVS_UNUSED,
> +                        void *config_)
>  {
>      struct server_config *config = config_;
>      const char *remote = argv[1];
> @@ -2299,7 +2345,9 @@ ovsdb_server_add_remote(struct unixctl_conn *conn, int argc OVS_UNUSED,
>   * that ovsdb-server services. */
>  static void
>  ovsdb_server_remove_remote(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                           const char *argv[], void *config_)
> +                           const char *argv[],
> +                           enum ovs_output_fmt fmt OVS_UNUSED,
> +                           void *config_)
>  {
>      struct server_config *config = config_;
>      struct ovsdb_jsonrpc_options *options;
> @@ -2321,7 +2369,9 @@ ovsdb_server_remove_remote(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  /* "ovsdb-server/list-remotes": outputs a list of configured rmeotes. */
>  static void
>  ovsdb_server_list_remotes(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                          const char *argv[] OVS_UNUSED, void *remotes_)
> +                          const char *argv[] OVS_UNUSED,
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
> +                          void *remotes_)
>  {
>      const struct shash *remotes = remotes_;
>      const struct shash_node **list;
> @@ -2343,7 +2393,9 @@ ovsdb_server_list_remotes(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  /* "ovsdb-server/add-db DB": adds the DB to ovsdb-server. */
>  static void
>  ovsdb_server_add_database(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                          const char *argv[], void *config_)
> +                          const char *argv[],
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
> +                          void *config_)
>  {
>      struct server_config *config = config_;
>      const char *filename = argv[1];
> @@ -2386,7 +2438,9 @@ remove_db(struct server_config *config, struct shash_node *node, char *comment)
>
>  static void
>  ovsdb_server_remove_database(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                             const char *argv[], void *config_)
> +                             const char *argv[],
> +                             enum ovs_output_fmt fmt OVS_UNUSED,
> +                             void *config_)
>  {
>      struct server_config *config = config_;
>      struct shash_node *node;
> @@ -2412,7 +2466,9 @@ ovsdb_server_remove_database(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_server_list_databases(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                            const char *argv[] OVS_UNUSED, void *all_dbs_)
> +                            const char *argv[] OVS_UNUSED,
> +                            enum ovs_output_fmt fmt OVS_UNUSED,
> +                            void *all_dbs_)
>  {
>      struct shash *all_dbs = all_dbs_;
>      const struct shash_node **nodes;
> @@ -2437,7 +2493,9 @@ ovsdb_server_list_databases(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_server_tlog_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                      const char *argv[], void *all_dbs_)
> +                      const char *argv[],
> +                      enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *all_dbs_)
>  {
>      struct shash *all_dbs = all_dbs_;
>      const char *name_ = argv[1];
> @@ -2483,7 +2541,9 @@ out:
>
>  static void
>  ovsdb_server_tlog_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                       const char *argv[] OVS_UNUSED, void *all_dbs_)
> +                       const char *argv[] OVS_UNUSED,
> +                       enum ovs_output_fmt fmt OVS_UNUSED,
> +                       void *all_dbs_)
>  {
>      const struct shash_node **db_nodes;
>      struct ds s = DS_EMPTY_INITIALIZER;
> @@ -2518,7 +2578,9 @@ ovsdb_server_tlog_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ovsdb_server_get_sync_status(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                             const char *argv[] OVS_UNUSED, void *config_)
> +                             const char *argv[] OVS_UNUSED,
> +                             enum ovs_output_fmt fmt OVS_UNUSED,
> +                             void *config_)
>  {
>      struct server_config *config = config_;
>      struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -2559,6 +2621,7 @@ static void
>  ovsdb_server_get_db_storage_status(struct unixctl_conn *conn,
>                                     int argc OVS_UNUSED,
>                                     const char *argv[],
> +                                   enum ovs_output_fmt fmt OVS_UNUSED,
>                                     void *config_)
>  {
>      struct server_config *config = config_;
> diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
> index 298616a64..fe6d24eaa 100644
> --- a/ovsdb/ovsdb.c
> +++ b/ovsdb/ovsdb.c
> @@ -186,6 +186,7 @@ static bool use_no_data_conversion = true;
>  static void
>  ovsdb_no_data_conversion_enable(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                                  const char *argv[] OVS_UNUSED,
> +                                enum ovs_output_fmt fmt OVS_UNUSED,
>                                  void *arg OVS_UNUSED)
>  {
>      use_no_data_conversion = true;
> @@ -200,7 +201,8 @@ ovsdb_no_data_conversion_disable(void)
>      }
>      use_no_data_conversion = false;
>      unixctl_command_register("ovsdb/file/no-data-conversion-enable", "",
> -                             0, 0, ovsdb_no_data_conversion_enable, NULL);
> +                             0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ovsdb_no_data_conversion_enable, NULL);
>  }
>
>  /* Returns true if the database storage allows conversion records without
> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> index f463afcb3..f12a5cb74 100644
> --- a/ovsdb/raft.c
> +++ b/ovsdb/raft.c
> @@ -4624,6 +4624,7 @@ raft_lookup_by_name(const char *name)
>  static void
>  raft_unixctl_cid(struct unixctl_conn *conn,
>                   int argc OVS_UNUSED, const char *argv[],
> +                 enum ovs_output_fmt fmt OVS_UNUSED,
>                   void *aux OVS_UNUSED)
>  {
>      struct raft *raft = raft_lookup_by_name(argv[1]);
> @@ -4641,6 +4642,7 @@ raft_unixctl_cid(struct unixctl_conn *conn,
>  static void
>  raft_unixctl_sid(struct unixctl_conn *conn,
>                   int argc OVS_UNUSED, const char *argv[],
> +                 enum ovs_output_fmt fmt OVS_UNUSED,
>                   void *aux OVS_UNUSED)
>  {
>      struct raft *raft = raft_lookup_by_name(argv[1]);
> @@ -4672,6 +4674,7 @@ raft_put_sid(const char *title, const struct uuid *sid,
>  static void
>  raft_unixctl_status(struct unixctl_conn *conn,
>                      int argc OVS_UNUSED, const char *argv[],
> +                    enum ovs_output_fmt fmt OVS_UNUSED,
>                      void *aux OVS_UNUSED)
>  {
>      struct raft *raft = raft_lookup_by_name(argv[1]);
> @@ -4830,7 +4833,8 @@ raft_unixctl_leave__(struct unixctl_conn *conn, struct raft *raft)
>
>  static void
>  raft_unixctl_leave(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                   const char *argv[], void *aux OVS_UNUSED)
> +                   const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                   void *aux OVS_UNUSED)
>  {
>      struct raft *raft = raft_lookup_by_name(argv[1]);
>      if (!raft) {
> @@ -4866,7 +4870,8 @@ raft_lookup_server_best_match(struct raft *raft, const char *id)
>
>  static void
>  raft_unixctl_kick(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[], void *aux OVS_UNUSED)
> +                  const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                  void *aux OVS_UNUSED)
>  {
>      const char *cluster_name = argv[1];
>      const char *server_name = argv[2];
> @@ -4939,6 +4944,7 @@ raft_log_election_timer(struct raft *raft)
>  static void
>  raft_unixctl_change_election_timer(struct unixctl_conn *conn,
>                                     int argc OVS_UNUSED, const char *argv[],
> +                                   enum ovs_output_fmt fmt OVS_UNUSED,
>                                     void *aux OVS_UNUSED)
>  {
>      const char *cluster_name = argv[1];
> @@ -4993,6 +4999,7 @@ raft_unixctl_change_election_timer(struct unixctl_conn *conn,
>  static void
>  raft_unixctl_set_backlog_threshold(struct unixctl_conn *conn,
>                                     int argc OVS_UNUSED, const char *argv[],
> +                                   enum ovs_output_fmt fmt OVS_UNUSED,
>                                     void *aux OVS_UNUSED)
>  {
>      const char *cluster_name = argv[1];
> @@ -5029,6 +5036,7 @@ raft_unixctl_set_backlog_threshold(struct unixctl_conn *conn,
>  static void
>  raft_unixctl_failure_test(struct unixctl_conn *conn OVS_UNUSED,
>                            int argc OVS_UNUSED, const char *argv[],
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
>                            void *aux OVS_UNUSED)
>  {
>      const char *test = argv[1];
> @@ -5083,22 +5091,24 @@ raft_init(void)
>      if (!ovsthread_once_start(&once)) {
>          return;
>      }
> -    unixctl_command_register("cluster/cid", "DB", 1, 1,
> +    unixctl_command_register("cluster/cid", "DB", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               raft_unixctl_cid, NULL);
> -    unixctl_command_register("cluster/sid", "DB", 1, 1,
> +    unixctl_command_register("cluster/sid", "DB", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               raft_unixctl_sid, NULL);
> -    unixctl_command_register("cluster/status", "DB", 1, 1,
> +    unixctl_command_register("cluster/status", "DB", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               raft_unixctl_status, NULL);
> -    unixctl_command_register("cluster/leave", "DB", 1, 1,
> +    unixctl_command_register("cluster/leave", "DB", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               raft_unixctl_leave, NULL);
>      unixctl_command_register("cluster/kick", "DB SERVER", 2, 2,
> -                             raft_unixctl_kick, NULL);
> +                             OVS_OUTPUT_FMT_TEXT,  raft_unixctl_kick, NULL);

One space too many before raft_unixctl_kick

>      unixctl_command_register("cluster/change-election-timer", "DB TIME", 2, 2,
> +                             OVS_OUTPUT_FMT_TEXT,
>                               raft_unixctl_change_election_timer, NULL);
>      unixctl_command_register("cluster/set-backlog-threshold",
> -                             "DB N_MSGS N_BYTES", 3, 3,
> +                             "DB N_MSGS N_BYTES", 3, 3, OVS_OUTPUT_FMT_TEXT,
>                               raft_unixctl_set_backlog_threshold, NULL);
>      unixctl_command_register("cluster/failure-test", "FAILURE SCENARIO", 1, 1,
> -                             raft_unixctl_failure_test, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, raft_unixctl_failure_test,
> +                             NULL);
>      ovsthread_once_done(&once);
>  }
> diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
> index 6d9bd5715..ebbd28487 100644
> --- a/python/ovs/unixctl/__init__.py
> +++ b/python/ovs/unixctl/__init__.py
> @@ -20,19 +20,16 @@ commands = {}
>
>
>  class _UnixctlCommand(object):
> -    # FIXME: Output format will be passed as 'output_fmts' to the command in
> -    #        later patch.
> -    def __init__(self, usage, min_args, max_args,  # FIXME: output_fmts,
> -                 callback, aux):
> +    def __init__(self, usage, min_args, max_args, output_fmts, callback, aux):
>          self.usage = usage
>          self.min_args = min_args
>          self.max_args = max_args
> -        # FIXME: self.output_fmts = output_fmts
> +        self.output_fmts = output_fmts
>          self.callback = callback
>          self.aux = aux
>
>
> -def _unixctl_help(conn, unused_argv, unused_aux):
> +def _unixctl_help(conn, unused_argv, unused_fmt, unused_aux):
>      reply = "The available commands are:\n"
>      command_names = sorted(commands.keys())
>      for name in command_names:
> @@ -46,8 +43,8 @@ def _unixctl_help(conn, unused_argv, unused_aux):
>      conn.reply(reply)
>
>
> -def command_register_fmt(name, usage, min_args, max_args, output_fmts,
> -                         callback, aux):
> +def command_register(name, usage, min_args, max_args, output_fmts, callback,
> +                     aux):
>      """ Registers a command with the given 'name' to be exposed by the
>      UnixctlServer. 'usage' describes the arguments to the command; it is used
>      only for presentation to the user in "help" output.  'output_fmts' is a
> @@ -71,29 +68,7 @@ def command_register_fmt(name, usage, min_args, max_args, output_fmts,
>
>      if name not in commands:
>          commands[name] = _UnixctlCommand(usage, min_args, max_args,
> -                                         # FIXME: output_fmts,
> -                                         callback, aux)
> -
> -
> -# FIXME: command_register() will be replaced with command_register_fmt() in a
> -#        later patch of this series. It is is kept temporarily to reduce the
> -#        amount of changes in this patch.
> -def command_register(name, usage, min_args, max_args, callback, aux):
> -    """ Registers a command with the given 'name' to be exposed by the
> -    UnixctlServer. 'usage' describes the arguments to the command; it is used
> -    only for presentation to the user in "help" output.
> -
> -    'callback' is called when the command is received.  It is passed a
> -    UnixctlConnection object, the list of arguments as unicode strings, and
> -    'aux'.  Normally 'callback' should reply by calling
> -    UnixctlConnection.reply() or UnixctlConnection.reply_error() before it
> -    returns, but if the command cannot be handled immediately, then it can
> -    defer the reply until later.  A given connection can only process a single
> -    request at a time, so a reply must be made eventually to avoid blocking
> -    that connection."""
> -
> -    command_register_fmt(name, usage, min_args, max_args,
> -                         ovs.util.OutputFormat.TEXT, callback, aux)
> +                                         output_fmts, callback, aux)
>
>
>  def socket_name_from_target(target):
> @@ -114,4 +89,5 @@ def socket_name_from_target(target):
>          return 0, "%s/%s.%d.ctl" % (ovs.dirs.RUNDIR, target, pid)
>
>
> -command_register("help", "", 0, 0, _unixctl_help, None)
> +command_register("help", "", 0, 0, ovs.util.OutputFormat.TEXT,
> +                 _unixctl_help, None)
> diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
> index 1d7f3337d..062bc8d33 100644
> --- a/python/ovs/unixctl/server.py
> +++ b/python/ovs/unixctl/server.py
> @@ -118,17 +118,11 @@ class UnixctlConnection(object):
>          elif len(params) > command.max_args:
>              error = '"%s" command takes at most %d arguments' \
>                      % (method, command.max_args)
> -        # FIXME: Uncomment when command.output_fmts is available
> -        # elif self._fmt not in command.output_fmts:
> -        #     error = '"%s" command does not support output format' \
> -        #             ' "%s" (supported: %d, requested: %d)' \
> -        #             % (method, self._fmt.name.lower(), command.output_fmts,
> -        #                self._fmt)
> -        # FIXME: Remove this check once output format will be passed to the
> -        #        command handler below.
> -        elif self._fmt != ovs.util.OutputFormat.TEXT:
> -            error = 'output format "%s" has not been implemented yet' \
> -                    % self._fmt.name.lower()
> +        elif self._fmt not in command.output_fmts:
> +            error = '"%s" command does not support output format' \
> +                    ' "%s" (supported: %d, requested: %d)' \
> +                    % (method, self._fmt.name.lower(), command.output_fmts,
> +                       self._fmt)
>          else:
>              for param in params:
>                  if not isinstance(param, str):
> @@ -137,20 +131,19 @@ class UnixctlConnection(object):
>
>              if error is None:
>                  unicode_params = [str(p) for p in params]
> -                command.callback(self, unicode_params,  # FIXME: self._fmt,
> -                                 command.aux)
> +                command.callback(self, unicode_params, self._fmt, command.aux)
>
>          if error:
>              self.reply_error(error)
>
>
> -def _unixctl_version(conn, unused_argv, version):
> +def _unixctl_version(conn, unused_argv, unused_fmt, version):
>      assert isinstance(conn, UnixctlConnection)
>      version = "%s (Open vSwitch) %s" % (ovs.util.PROGRAM_NAME, version)
>      conn.reply(version)
>
>
> -def _unixctl_set_options(conn, argv, unused_aux):
> +def _unixctl_set_options(conn, argv, unused_fmt, unused_aux):
>      assert isinstance(conn, UnixctlConnection)
>
>      parser = argparse.ArgumentParser()
> @@ -159,7 +152,7 @@ def _unixctl_set_options(conn, argv, unused_aux):
>                                   for fmt in ovs.util.OutputFormat])
>
>      try:
> -        args = parser.parse_args(args=argv)
> +        args = parser.parse_args(args=argv[1:])
>      except argparse.ArgumentError as e:
>          conn.reply_error(str(e))
>          return
> @@ -239,10 +232,11 @@ class UnixctlServer(object):
>                                 % path)
>              return error, None
>
> -        ovs.unixctl.command_register("version", "", 0, 0, _unixctl_version,
> -                                     version)
> -
> +        ovs.unixctl.command_register("version", "", 0, 0,
> +                                     ovs.util.OutputFormat.TEXT,
> +                                     _unixctl_version, version)
>          ovs.unixctl.command_register("set-options", "[--format text|json]", 2,
> -                                     2, _unixctl_set_options, None)
> +                                     2, ovs.util.OutputFormat.TEXT,
> +                                     _unixctl_set_options, None)
>
>          return 0, UnixctlServer(listener)
> diff --git a/python/ovs/vlog.py b/python/ovs/vlog.py
> index 61f5928db..ba8701040 100644
> --- a/python/ovs/vlog.py
> +++ b/python/ovs/vlog.py
> @@ -237,8 +237,10 @@ class Vlog(object):
>                  logger.disabled = True
>
>          ovs.unixctl.command_register("vlog/reopen", "", 0, 0,
> +                                     ovs.util.OutputFormat.TEXT,
>                                       Vlog._unixctl_vlog_reopen, None)
>          ovs.unixctl.command_register("vlog/close", "", 0, 0,
> +                                     ovs.util.OutputFormat.TEXT,
>                                       Vlog._unixctl_vlog_close, None)
>          try:
>              # Windows limitation on Python 2, sys.maxsize is a long number
> @@ -248,8 +250,10 @@ class Vlog(object):
>          except AttributeError:
>              maxsize_int = sys.maxsize
>          ovs.unixctl.command_register("vlog/set", "spec", 1, maxsize_int,
> +                                     ovs.util.OutputFormat.TEXT,
>                                       Vlog._unixctl_vlog_set, None)
>          ovs.unixctl.command_register("vlog/list", "", 0, 0,
> +                                     ovs.util.OutputFormat.TEXT,
>                                       Vlog._unixctl_vlog_list, None)
>
>      @staticmethod
> @@ -406,7 +410,7 @@ class Vlog(object):
>              Vlog.__file_handler.close()
>
>      @staticmethod
> -    def _unixctl_vlog_reopen(conn, unused_argv, unused_aux):
> +    def _unixctl_vlog_reopen(conn, unused_argv, unused_fmt, unused_aux):
>          if Vlog.__log_file:
>              Vlog.reopen_log_file()
>              conn.reply(None)
> @@ -414,7 +418,7 @@ class Vlog(object):
>              conn.reply("Logging to file not configured")
>
>      @staticmethod
> -    def _unixctl_vlog_close(conn, unused_argv, unused_aux):
> +    def _unixctl_vlog_close(conn, unused_argv, unused_fmt, unused_aux):
>          if Vlog.__log_file:
>              if sys.platform != 'win32':
>                  logger = logging.getLogger("file")
> @@ -424,7 +428,7 @@ class Vlog(object):
>          conn.reply(None)
>
>      @staticmethod
> -    def _unixctl_vlog_set(conn, argv, unused_aux):
> +    def _unixctl_vlog_set(conn, argv, unused_fmt, unused_aux):
>          for arg in argv:
>              msg = Vlog.set_levels_from_string(arg)
>              if msg:
> @@ -433,7 +437,7 @@ class Vlog(object):
>          conn.reply(None)
>
>      @staticmethod
> -    def _unixctl_vlog_list(conn, unused_argv, unused_aux):
> +    def _unixctl_vlog_list(conn, unused_argv, unused_fmt, unused_aux):
>          conn.reply(Vlog.get_levels())
>
>
> diff --git a/tests/test-netflow.c b/tests/test-netflow.c
> index 7f89cfcae..b808cb9fd 100644
> --- a/tests/test-netflow.c
> +++ b/tests/test-netflow.c
> @@ -201,7 +201,8 @@ test_netflow_main(int argc, char *argv[])
>      if (error) {
>          ovs_fatal(error, "failed to create unixctl server");
>      }
> -    unixctl_command_register("exit", "", 0, 0, test_netflow_exit, &exiting);
> +    unixctl_command_register("exit", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             test_netflow_exit, &exiting);
>
>      daemonize_complete();
>
> @@ -294,6 +295,7 @@ usage(void)
>  static void
>  test_netflow_exit(struct unixctl_conn *conn,
>                    int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
>                    void *exiting_)
>  {
>      bool *exiting = exiting_;
> diff --git a/tests/test-sflow.c b/tests/test-sflow.c
> index 3c617bdd1..82ef66178 100644
> --- a/tests/test-sflow.c
> +++ b/tests/test-sflow.c
> @@ -715,7 +715,8 @@ test_sflow_main(int argc, char *argv[])
>      if (error) {
>          ovs_fatal(error, "failed to create unixctl server");
>      }
> -    unixctl_command_register("exit", "", 0, 0, test_sflow_exit, &exiting);
> +    unixctl_command_register("exit", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             test_sflow_exit, &exiting);
>
>      daemonize_complete();
>
> @@ -804,6 +805,7 @@ usage(void)
>  static void
>  test_sflow_exit(struct unixctl_conn *conn,
>                  int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
> +                enum ovs_output_fmt fmt OVS_UNUSED,
>                  void *exiting_)
>  {
>      bool *exiting = exiting_;
> diff --git a/tests/test-unixctl.c b/tests/test-unixctl.c
> index 9e8982789..e344a8203 100644
> --- a/tests/test-unixctl.c
> +++ b/tests/test-unixctl.c
> @@ -33,7 +33,9 @@ OVS_NO_RETURN static void usage(void);
>
>  static void
>  test_unixctl_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[] OVS_UNUSED, void *exiting_)
> +                  const char *argv[] OVS_UNUSED,
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
> +                  void *exiting_)
>  {
>      bool *exiting = exiting_;
>      *exiting = true;
> @@ -42,21 +44,26 @@ test_unixctl_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  test_unixctl_echo(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[], void *aux OVS_UNUSED)
> +                  const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED,
> +                  void *aux OVS_UNUSED)
>  {
>      unixctl_command_reply(conn, argv[1]);
>  }
>
>  static void
>  test_unixctl_echo_error(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                        const char *argv[], void *aux OVS_UNUSED)
> +                        const char *argv[],
> +                        enum ovs_output_fmt fmt OVS_UNUSED,
> +                        void *aux OVS_UNUSED)
>  {
>      unixctl_command_reply_error(conn, argv[1]);
>  }
>
>  static void
>  test_unixctl_log(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                  const char *argv[], void *aux OVS_UNUSED)
> +                  const char *argv[],
> +                 enum ovs_output_fmt fmt OVS_UNUSED, void *aux OVS_UNUSED)
>  {
>      VLOG_INFO("%s", argv[1]);
>      unixctl_command_reply(conn, NULL);
> @@ -64,7 +71,9 @@ test_unixctl_log(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  test_unixctl_block(struct unixctl_conn *conn OVS_UNUSED, int argc OVS_UNUSED,
> -                   const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +                   const char *argv[] OVS_UNUSED,
> +                   enum ovs_output_fmt fmt OVS_UNUSED,
> +                   void *aux OVS_UNUSED)
>  {
>      VLOG_INFO("%s", argv[1]);
>      unixctl_command_reply(conn, NULL);
> @@ -88,12 +97,16 @@ test_unixctl_main(int argc, char *argv[])
>      if (retval) {
>          exit(EXIT_FAILURE);
>      }
> -    unixctl_command_register("exit", "", 0, 0, test_unixctl_exit, &exiting);
> -    unixctl_command_register("echo", "ARG", 1, 1, test_unixctl_echo, NULL);
> -    unixctl_command_register("echo_error", "ARG", 1, 1,
> +    unixctl_command_register("exit", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             test_unixctl_exit, &exiting);
> +    unixctl_command_register("echo", "ARG", 1, 1, OVS_OUTPUT_FMT_TEXT,
> +                             test_unixctl_echo, NULL);
> +    unixctl_command_register("echo_error", "ARG", 1, 1, OVS_OUTPUT_FMT_TEXT,
>                               test_unixctl_echo_error, NULL);
> -    unixctl_command_register("log", "ARG", 1, 1, test_unixctl_log, NULL);
> -    unixctl_command_register("block", "", 0, 0, test_unixctl_block, NULL);
> +    unixctl_command_register("log", "ARG", 1, 1, OVS_OUTPUT_FMT_TEXT,
> +                             test_unixctl_log, NULL);
> +    unixctl_command_register("block", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             test_unixctl_block, NULL);
>      daemonize_complete();
>
>      VLOG_INFO("Entering run loop.");
> diff --git a/tests/test-unixctl.py b/tests/test-unixctl.py
> index 4fa27b09f..b5fe12eb7 100644
> --- a/tests/test-unixctl.py
> +++ b/tests/test-unixctl.py
> @@ -17,12 +17,13 @@ import argparse
>  import ovs.daemon
>  import ovs.unixctl
>  import ovs.unixctl.server
> +import ovs.util
>
>  vlog = ovs.vlog.Vlog("test-unixctl")
>  exiting = False
>
>
> -def unixctl_exit(conn, unused_argv, aux):
> +def unixctl_exit(conn, unused_argv, unused_fmt, aux):
>      assert aux == "aux_exit"
>      global exiting
>
> @@ -30,22 +31,22 @@ def unixctl_exit(conn, unused_argv, aux):
>      conn.reply(None)
>
>
> -def unixctl_echo(conn, argv, aux):
> +def unixctl_echo(conn, argv, unused_fmt, aux):
>      assert aux == "aux_echo"
>      conn.reply(str(argv))
>
>
> -def unixctl_echo_error(conn, argv, aux):
> +def unixctl_echo_error(conn, argv, unused_fmt, aux):
>      assert aux == "aux_echo_error"
>      conn.reply_error(str(argv))
>
>
> -def unixctl_log(conn, argv, unused_aux):
> +def unixctl_log(conn, argv, unused_fmt, unused_aux):
>      vlog.info(str(argv[0]))
>      conn.reply(None)
>
>
> -def unixctl_block(conn, unused_argv, unused_aux):
> +def unixctl_block(conn, unused_argv, unused_fmt, unused_aux):
>      pass
>
>
> @@ -66,13 +67,19 @@ def main():
>          ovs.util.ovs_fatal(error, "could not create unixctl server at %s"
>                             % args.unixctl, vlog)
>
> -    ovs.unixctl.command_register("exit", "", 0, 0, unixctl_exit, "aux_exit")
> -    ovs.unixctl.command_register("echo", "[arg ...]", 1, 2, unixctl_echo,
> +    ovs.unixctl.command_register("exit", "", 0, 0, ovs.util.OutputFormat.TEXT,
> +                                 unixctl_exit, "aux_exit")
> +    ovs.unixctl.command_register("echo", "[arg ...]", 1, 2,
> +                                 ovs.util.OutputFormat.TEXT, unixctl_echo,
>                                   "aux_echo")
> -    ovs.unixctl.command_register("log", "[arg ...]", 1, 2, unixctl_log, None)
> +    ovs.unixctl.command_register("log", "[arg ...]", 1, 2,
> +                                 ovs.util.OutputFormat.TEXT, unixctl_log,
> +                                 None)
>      ovs.unixctl.command_register("echo_error", "[arg ...]", 1, 2,
> +                                 ovs.util.OutputFormat.TEXT,
>                                   unixctl_echo_error, "aux_echo_error")
> -    ovs.unixctl.command_register("block", "", 0, 0, unixctl_block, None)
> +    ovs.unixctl.command_register("block", "", 0, 0, ovs.util.OutputFormat.TEXT,
> +                                 unixctl_block, None)
>      ovs.daemon.daemonize_complete()
>
>      vlog.info("Entering run loop.")
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index ba3458e55..9f7e78f61 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -534,7 +534,8 @@ usage(void)
>
>  static void
>  ofctl_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -           const char *argv[] OVS_UNUSED, void *exiting_)
> +           const char *argv[] OVS_UNUSED, enum ovs_output_fmt fmt OVS_UNUSED,
> +           void *exiting_)
>  {
>      bool *exiting = exiting_;
>      *exiting = true;
> @@ -1945,7 +1946,8 @@ openflow_from_hex(const char *hex, struct ofpbuf **msgp)
>
>  static void
>  ofctl_send(struct unixctl_conn *conn, int argc,
> -           const char *argv[], void *vconn_)
> +           const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +           void *vconn_)
>  {
>      struct vconn *vconn = vconn_;
>      struct ds reply;
> @@ -1991,7 +1993,8 @@ ofctl_send(struct unixctl_conn *conn, int argc,
>
>  static void
>  unixctl_packet_out(struct unixctl_conn *conn, int OVS_UNUSED argc,
> -                   const char *argv[], void *vconn_)
> +                   const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                   void *vconn_)
>  {
>      struct vconn *vconn = vconn_;
>      enum ofputil_protocol protocol
> @@ -2052,7 +2055,8 @@ struct barrier_aux {
>
>  static void
>  ofctl_barrier(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -              const char *argv[] OVS_UNUSED, void *aux_)
> +              const char *argv[] OVS_UNUSED,
> +              enum ovs_output_fmt fmt OVS_UNUSED, void *aux_)
>  {
>      struct barrier_aux *aux = aux_;
>      struct ofpbuf *msg;
> @@ -2075,7 +2079,8 @@ ofctl_barrier(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ofctl_set_output_file(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                      const char *argv[], void *aux OVS_UNUSED)
> +                      const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                      void *aux OVS_UNUSED)
>  {
>      int fd;
>
> @@ -2093,7 +2098,9 @@ ofctl_set_output_file(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ofctl_block(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -            const char *argv[] OVS_UNUSED, void *blocked_)
> +            const char *argv[] OVS_UNUSED,
> +            enum ovs_output_fmt fmt OVS_UNUSED,
> +            void *blocked_)
>  {
>      bool *blocked = blocked_;
>
> @@ -2107,7 +2114,9 @@ ofctl_block(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  ofctl_unblock(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -              const char *argv[] OVS_UNUSED, void *blocked_)
> +              const char *argv[] OVS_UNUSED,
> +              enum ovs_output_fmt fmt OVS_UNUSED,
> +              void *blocked_)
>  {
>      bool *blocked = blocked_;
>
> @@ -2142,19 +2151,21 @@ monitor_vconn(struct vconn *vconn, bool reply_to_echo_requests,
>      if (error) {
>          ovs_fatal(error, "failed to create unixctl server");
>      }
> -    unixctl_command_register("exit", "", 0, 0, ofctl_exit, &exiting);
> +    unixctl_command_register("exit", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ofctl_exit, &exiting);
>      unixctl_command_register("ofctl/send", "OFMSG...", 1, INT_MAX,
> -                             ofctl_send, vconn);
> +                             OVS_OUTPUT_FMT_TEXT, ofctl_send, vconn);
>      unixctl_command_register("ofctl/packet-out", "\"in_port=<port> packet=<hex data> actions=...\"", 1, 1,
> -                             unixctl_packet_out, vconn);
> -    unixctl_command_register("ofctl/barrier", "", 0, 0,
> +                             OVS_OUTPUT_FMT_TEXT, unixctl_packet_out, vconn);
> +    unixctl_command_register("ofctl/barrier", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
>                               ofctl_barrier, &barrier_aux);
>      unixctl_command_register("ofctl/set-output-file", "FILE", 1, 1,
> -                             ofctl_set_output_file, NULL);
> -
> -    unixctl_command_register("ofctl/block", "", 0, 0, ofctl_block, &blocked);
> -    unixctl_command_register("ofctl/unblock", "", 0, 0, ofctl_unblock,
> -                             &blocked);
> +                             OVS_OUTPUT_FMT_TEXT, ofctl_set_output_file,
> +                             NULL);
> +    unixctl_command_register("ofctl/block", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ofctl_block, &blocked);
> +    unixctl_command_register("ofctl/unblock", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> +                             ofctl_unblock, &blocked);
>
>      daemonize_complete();
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 95a65fcdc..953e9647f 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -516,13 +516,16 @@ bridge_init(const char *remote)
>
>      /* Register unixctl commands. */
>      unixctl_command_register("qos/show-types", "interface", 1, 1,
> -                             qos_unixctl_show_types, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, qos_unixctl_show_types,
> +                             NULL);
>      unixctl_command_register("qos/show", "interface", 1, 1,
> -                             qos_unixctl_show, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, qos_unixctl_show, NULL);
>      unixctl_command_register("bridge/dump-flows", "[--offload-stats] bridge",
> -                             1, 2, bridge_unixctl_dump_flows, NULL);
> +                             1, 2, OVS_OUTPUT_FMT_TEXT,
> +                             bridge_unixctl_dump_flows, NULL);
>      unixctl_command_register("bridge/reconnect", "[bridge]", 0, 1,
> -                             bridge_unixctl_reconnect, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, bridge_unixctl_reconnect,
> +                             NULL);
>      lacp_init();
>      bond_init();
>      cfm_init();
> @@ -3512,7 +3515,8 @@ qos_unixctl_show_queue(unsigned int queue_id,
>
>  static void
>  qos_unixctl_show_types(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                       const char *argv[], void *aux OVS_UNUSED)
> +                       const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                       void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      struct sset types = SSET_INITIALIZER(&types);
> @@ -3550,7 +3554,8 @@ qos_unixctl_show_types(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>  static void
>  qos_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                 const char *argv[], void *aux OVS_UNUSED)
> +                 const char *argv[], enum ovs_output_fmt fmt OVS_UNUSED,
> +                 void *aux OVS_UNUSED)
>  {
>      struct ds ds = DS_EMPTY_INITIALIZER;
>      struct smap smap = SMAP_INITIALIZER(&smap);
> @@ -3670,7 +3675,9 @@ bridge_lookup(const char *name)
>   * stack, including those normally hidden. */
>  static void
>  bridge_unixctl_dump_flows(struct unixctl_conn *conn, int argc,
> -                          const char *argv[], void *aux OVS_UNUSED)
> +                          const char *argv[],
> +                          enum ovs_output_fmt fmt OVS_UNUSED,
> +                          void *aux OVS_UNUSED)
>  {
>      struct bridge *br;
>      struct ds results;
> @@ -3700,7 +3707,9 @@ bridge_unixctl_dump_flows(struct unixctl_conn *conn, int argc,
>   * drop their controller connections and reconnect. */
>  static void
>  bridge_unixctl_reconnect(struct unixctl_conn *conn, int argc,
> -                         const char *argv[], void *aux OVS_UNUSED)
> +                         const char *argv[],
> +                         enum ovs_output_fmt fmt OVS_UNUSED,
> +                         void *aux OVS_UNUSED)
>  {
>      struct bridge *br;
>      if (argc > 1) {
> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
> index 273af9f5d..74b755778 100644
> --- a/vswitchd/ovs-vswitchd.c
> +++ b/vswitchd/ovs-vswitchd.c
> @@ -111,7 +111,7 @@ main(int argc, char *argv[])
>          exit(EXIT_FAILURE);
>      }
>      unixctl_command_register("exit", "[--cleanup]", 0, 1,
> -                             ovs_vswitchd_exit, NULL);
> +                             OVS_OUTPUT_FMT_TEXT, ovs_vswitchd_exit, NULL);
>
>      bridge_init(remote);
>      free(remote);
> @@ -308,7 +308,8 @@ usage(void)
>
>  static void
>  ovs_vswitchd_exit(struct unixctl_conn *conn, int argc,
> -                  const char *argv[], void *args OVS_UNUSED)
> +                  const char *argv[],
> +                  enum ovs_output_fmt fmt OVS_UNUSED, void *args OVS_UNUSED)
>  {
>      exit_args.n_conns++;
>      exit_args.conns = xrealloc(exit_args.conns,
> diff --git a/vtep/ovs-vtep.in b/vtep/ovs-vtep.in
> index 0ee23b119..54fb3a7e2 100755
> --- a/vtep/ovs-vtep.in
> +++ b/vtep/ovs-vtep.in
> @@ -76,7 +76,7 @@ def vtep_ctl(args):
>      return call_prog("vtep-ctl", shlex.split(args))
>
>
> -def unixctl_exit(conn, unused_argv, unused_aux):
> +def unixctl_exit(conn, unused_argv, unused_fmt, unused_aux):
>      global exiting
>      exiting = True
>      conn.reply(None)
> @@ -732,7 +732,8 @@ def main():
>
>      ovs.daemon.daemonize()
>
> -    ovs.unixctl.command_register("exit", "", 0, 0, unixctl_exit, None)
> +    ovs.unixctl.command_register("exit", "", 0, 0, ovs.util.OutputFormat.TEXT,
> +                                 unixctl_exit, None)
>      error, unixctl = ovs.unixctl.server.UnixctlServer.create(None,
>                                                               version=VERSION)
>      if error:
> -- 
> 2.39.2

Hi!

On 15.03.24 11:19, Eelco Chaudron wrote:
> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>
>> ...
> Thank for the patch! What a beast to go trough ;)

Thank you for doing it anyway ☺️

> I believe the current approach is acceptable. However, we could also
> incorporate union callbacks: if registration only supports text, we would
> use callback A, and if multiple formats exist, we could employ the new style
> callback. This would mitigate the need for a significant overhaul. Just a
> suggestion, as I'm ok with the current approach. :)

This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.

We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.

I do not have a strong opinion here. What do you, Ilya and the others think?

> Some small style comments highlighted below.

I would like to have your eagle eyes...👀 Fixed them!

Thanks,
Jakob

On 19 Mar 2024, at 13:11, Jakob Meng wrote:

> Hi!
>
> On 15.03.24 11:19, Eelco Chaudron wrote:
>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>
>>> ...
>> Thank for the patch! What a beast to go trough ;)
>
> Thank you for doing it anyway ☺️
>
>> I believe the current approach is acceptable. However, we could also
>> incorporate union callbacks: if registration only supports text, we would
>> use callback A, and if multiple formats exist, we could employ the new style
>> callback. This would mitigate the need for a significant overhaul. Just a
>> suggestion, as I'm ok with the current approach. :)
>
> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>
> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.

Or have a utility function to return the format, which keeps the structure invisible.

> I do not have a strong opinion here. What do you, Ilya and the others think?

I’ll wait for others to chime in...

>> Some small style comments highlighted below.
>
> I would like to have your eagle eyes...👀 Fixed them!
>
> Thanks,
> Jakob

On 3/19/24 13:17, Eelco Chaudron wrote:
> 
> 
> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
> 
>> Hi!
>>
>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>
>>>> ...
>>> Thank for the patch! What a beast to go trough ;)
>>
>> Thank you for doing it anyway ☺️
>>
>>> I believe the current approach is acceptable. However, we could also
>>> incorporate union callbacks: if registration only supports text, we would
>>> use callback A, and if multiple formats exist, we could employ the new style
>>> callback. This would mitigate the need for a significant overhaul. Just a
>>> suggestion, as I'm ok with the current approach. :)
>>
>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>
>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
> 
> Or have a utility function to return the format, which keeps the structure invisible.

+1, just add some access methods.

> 
>> I do not have a strong opinion here. What do you, Ilya and the others think?
> 
> I’ll wait for others to chime in...

I don't think changing all the callbacks is a good thing to do.
i.e. this patch should not exist or be very small.

> 
>>> Some small style comments highlighted below.
>>
>> I would like to have your eagle eyes...👀 Fixed them!
>>
>> Thanks,
>> Jakob
>

On 3/19/24 13:21, Ilya Maximets wrote:
> On 3/19/24 13:17, Eelco Chaudron wrote:
>>
>>
>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>
>>> Hi!
>>>
>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>
>>>>> ...
>>>> Thank for the patch! What a beast to go trough ;)
>>>
>>> Thank you for doing it anyway ☺️
>>>
>>>> I believe the current approach is acceptable. However, we could also
>>>> incorporate union callbacks: if registration only supports text, we would
>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>> suggestion, as I'm ok with the current approach. :)
>>>
>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>
>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>
>> Or have a utility function to return the format, which keeps the structure invisible.
> 
> +1, just add some access methods.
> 
>>
>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>
>> I’ll wait for others to chime in...
> 
> I don't think changing all the callbacks is a good thing to do.
> i.e. this patch should not exist or be very small.

* or it should be very small.

> 
>>
>>>> Some small style comments highlighted below.
>>>
>>> I would like to have your eagle eyes...👀 Fixed them!
>>>
>>> Thanks,
>>> Jakob
>>
>

On 19.03.24 13:22, Ilya Maximets wrote:
> On 3/19/24 13:21, Ilya Maximets wrote:
>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>
>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>
>>>> Hi!
>>>>
>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>
>>>>>> ...
>>>>> Thank for the patch! What a beast to go trough ;)
>>>> Thank you for doing it anyway ☺️
>>>>
>>>>> I believe the current approach is acceptable. However, we could also
>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>> suggestion, as I'm ok with the current approach. :)
>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>
>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>> Or have a utility function to return the format, which keeps the structure invisible.
>> +1, just add some access methods.
>>
>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>> I’ll wait for others to chime in...
>> I don't think changing all the callbacks is a good thing to do.
>> i.e. this patch should not exist or be very small.
> * or it should be very small.

How about unixctl_command_register?

Should we change the existing function signature (like I did)?
Or add a second function, like unixctl_command_register_fmt?
Or should we add another function to set/change supported formats of an already registered command?

Example:

// Get output format which the user has choosen (defaults to txt)
enum ovs_output_fmt
unixctl_command_get_output_format(struct unixctl_conn *);

// Set output formats that a command supports
int
unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);

Wdyt?



>
>>>>> Some small style comments highlighted below.
>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>
>>>> Thanks,
>>>> Jakob

On 3/19/24 14:41, Jakob Meng wrote:
> 
> 
> On 19.03.24 13:22, Ilya Maximets wrote:
>> On 3/19/24 13:21, Ilya Maximets wrote:
>>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>>
>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>>
>>>>> Hi!
>>>>>
>>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>>
>>>>>>> ...
>>>>>> Thank for the patch! What a beast to go trough ;)
>>>>> Thank you for doing it anyway ☺️
>>>>>
>>>>>> I believe the current approach is acceptable. However, we could also
>>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>>> suggestion, as I'm ok with the current approach. :)
>>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>>
>>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>>> Or have a utility function to return the format, which keeps the structure invisible.
>>> +1, just add some access methods.
>>>
>>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>>> I’ll wait for others to chime in...
>>> I don't think changing all the callbacks is a good thing to do.
>>> i.e. this patch should not exist or be very small.
>> * or it should be very small.
> 
> How about unixctl_command_register?
> 
> Should we change the existing function signature (like I did)?
> Or add a second function, like unixctl_command_register_fmt?
> Or should we add another function to set/change supported formats of an already registered command?
> 
> Example:
> 
> // Get output format which the user has choosen (defaults to txt)
> enum ovs_output_fmt
> unixctl_command_get_output_format(struct unixctl_conn *);
> 
> // Set output formats that a command supports
> int
> unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);
> 
> Wdyt?

I'm actually not sure why we need to register them differently in the
first palace.  Make the callback check what was the requested format
and use appropriate reply function.  Why we need to fail the requests
for callbacks that don't support JSON replies?  Why can't we just
pack the plain reply into a JSON string?
We could also invent a new format for the case where JSON format is
requested, but the callback doesn't support it.  E.g. by creating a
reply like this: {"reply-format": "plain", "reply": "<plain text>"}
(for the plain text replies only).

> 
> 
> 
>>
>>>>>> Some small style comments highlighted below.
>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>
>>>>> Thanks,
>>>>> Jakob
>

On 19 Mar 2024, at 15:00, Ilya Maximets wrote:

> On 3/19/24 14:41, Jakob Meng wrote:
>>
>>
>> On 19.03.24 13:22, Ilya Maximets wrote:
>>> On 3/19/24 13:21, Ilya Maximets wrote:
>>>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>>>
>>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>>>
>>>>>> Hi!
>>>>>>
>>>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>>>
>>>>>>>> ...
>>>>>>> Thank for the patch! What a beast to go trough ;)
>>>>>> Thank you for doing it anyway ☺️
>>>>>>
>>>>>>> I believe the current approach is acceptable. However, we could also
>>>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>>>> suggestion, as I'm ok with the current approach. :)
>>>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>>>
>>>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>>>> Or have a utility function to return the format, which keeps the structure invisible.
>>>> +1, just add some access methods.
>>>>
>>>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>>>> I’ll wait for others to chime in...
>>>> I don't think changing all the callbacks is a good thing to do.
>>>> i.e. this patch should not exist or be very small.
>>> * or it should be very small.
>>
>> How about unixctl_command_register?
>>
>> Should we change the existing function signature (like I did)?
>> Or add a second function, like unixctl_command_register_fmt?
>> Or should we add another function to set/change supported formats of an already registered command?
>>
>> Example:
>>
>> // Get output format which the user has choosen (defaults to txt)
>> enum ovs_output_fmt
>> unixctl_command_get_output_format(struct unixctl_conn *);
>>
>> // Set output formats that a command supports
>> int
>> unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);
>>
>> Wdyt?
>
> I'm actually not sure why we need to register them differently in the
> first palace.  Make the callback check what was the requested format
> and use appropriate reply function.

I guess this can be done with the unixctl_command_get_output_format() function?

> Why we need to fail the requests
> for callbacks that don't support JSON replies?  Why can't we just
> pack the plain reply into a JSON string?

I don’t like this, this way we will not know if the command actually supports native JSON or not. Else we might confuse scripts in the future if we start sending some JSONized responses. returning an error might also trigger people to submit a patch to convert more commands to native JSON support :)

> We could also invent a new format for the case where JSON format is
> requested, but the callback doesn't support it.  E.g. by creating a
> reply like this: {"reply-format": "plain", "reply": "<plain text>"}
> (for the plain text replies only).

>>
>>
>>
>>>
>>>>>>> Some small style comments highlighted below.
>>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>>
>>>>>> Thanks,
>>>>>> Jakob
>>

On 3/19/24 15:17, Eelco Chaudron wrote:
> 
> 
> On 19 Mar 2024, at 15:00, Ilya Maximets wrote:
> 
>> On 3/19/24 14:41, Jakob Meng wrote:
>>>
>>>
>>> On 19.03.24 13:22, Ilya Maximets wrote:
>>>> On 3/19/24 13:21, Ilya Maximets wrote:
>>>>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>>>>
>>>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>>>>
>>>>>>> Hi!
>>>>>>>
>>>>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>>>>
>>>>>>>>> ...
>>>>>>>> Thank for the patch! What a beast to go trough ;)
>>>>>>> Thank you for doing it anyway ☺️
>>>>>>>
>>>>>>>> I believe the current approach is acceptable. However, we could also
>>>>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>>>>> suggestion, as I'm ok with the current approach. :)
>>>>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>>>>
>>>>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>>>>> Or have a utility function to return the format, which keeps the structure invisible.
>>>>> +1, just add some access methods.
>>>>>
>>>>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>>>>> I’ll wait for others to chime in...
>>>>> I don't think changing all the callbacks is a good thing to do.
>>>>> i.e. this patch should not exist or be very small.
>>>> * or it should be very small.
>>>
>>> How about unixctl_command_register?
>>>
>>> Should we change the existing function signature (like I did)?
>>> Or add a second function, like unixctl_command_register_fmt?
>>> Or should we add another function to set/change supported formats of an already registered command?
>>>
>>> Example:
>>>
>>> // Get output format which the user has choosen (defaults to txt)
>>> enum ovs_output_fmt
>>> unixctl_command_get_output_format(struct unixctl_conn *);
>>>
>>> // Set output formats that a command supports
>>> int
>>> unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);
>>>
>>> Wdyt?
>>
>> I'm actually not sure why we need to register them differently in the
>> first palace.  Make the callback check what was the requested format
>> and use appropriate reply function.
> 
> I guess this can be done with the unixctl_command_get_output_format() function?
> 
>> Why we need to fail the requests
>> for callbacks that don't support JSON replies?  Why can't we just
>> pack the plain reply into a JSON string?
> 
> I don’t like this, this way we will not know if the command actually supports native JSON or not. Else we might confuse scripts in the future if we start sending some JSONized responses. returning an error might also trigger people to submit a patch to convert more commands to native JSON support :)

The main problem is that we shouldn't fail commands after they are
already executed.  It may work for 'show' commands, but it's not
a thing that can be done for 'set' commands.

The suggestion below shouldn't be confusing for scripts.

> 
>> We could also invent a new format for the case where JSON format is
>> requested, but the callback doesn't support it.  E.g. by creating a
>> reply like this: {"reply-format": "plain", "reply": "<plain text>"}
>> (for the plain text replies only).
> 
>>>
>>>
>>>
>>>>
>>>>>>>> Some small style comments highlighted below.
>>>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jakob
>>>
>

On 19 Mar 2024, at 15:46, Ilya Maximets wrote:

> On 3/19/24 15:17, Eelco Chaudron wrote:
>>
>>
>> On 19 Mar 2024, at 15:00, Ilya Maximets wrote:
>>
>>> On 3/19/24 14:41, Jakob Meng wrote:
>>>>
>>>>
>>>> On 19.03.24 13:22, Ilya Maximets wrote:
>>>>> On 3/19/24 13:21, Ilya Maximets wrote:
>>>>>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>>>>>
>>>>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>>>>>
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>> Thank for the patch! What a beast to go trough ;)
>>>>>>>> Thank you for doing it anyway ☺️
>>>>>>>>
>>>>>>>>> I believe the current approach is acceptable. However, we could also
>>>>>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>>>>>> suggestion, as I'm ok with the current approach. :)
>>>>>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>>>>>
>>>>>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>>>>>> Or have a utility function to return the format, which keeps the structure invisible.
>>>>>> +1, just add some access methods.
>>>>>>
>>>>>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>>>>>> I’ll wait for others to chime in...
>>>>>> I don't think changing all the callbacks is a good thing to do.
>>>>>> i.e. this patch should not exist or be very small.
>>>>> * or it should be very small.
>>>>
>>>> How about unixctl_command_register?
>>>>
>>>> Should we change the existing function signature (like I did)?
>>>> Or add a second function, like unixctl_command_register_fmt?
>>>> Or should we add another function to set/change supported formats of an already registered command?
>>>>
>>>> Example:
>>>>
>>>> // Get output format which the user has choosen (defaults to txt)
>>>> enum ovs_output_fmt
>>>> unixctl_command_get_output_format(struct unixctl_conn *);
>>>>
>>>> // Set output formats that a command supports
>>>> int
>>>> unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);
>>>>
>>>> Wdyt?
>>>
>>> I'm actually not sure why we need to register them differently in the
>>> first palace.  Make the callback check what was the requested format
>>> and use appropriate reply function.
>>
>> I guess this can be done with the unixctl_command_get_output_format() function?
>>
>>> Why we need to fail the requests
>>> for callbacks that don't support JSON replies?  Why can't we just
>>> pack the plain reply into a JSON string?
>>
>> I don’t like this, this way we will not know if the command actually supports native JSON or not. Else we might confuse scripts in the future if we start sending some JSONized responses. returning an error might also trigger people to submit a patch to convert more commands to native JSON support :)
>
> The main problem is that we shouldn't fail commands after they are
> already executed.  It may work for 'show' commands, but it's not
> a thing that can be done for 'set' commands.

I guess this means I would prefer to take a ‘bigger’ change hit and change the register procedure (not the actual callback), to include the supported output formats. This way we can still error out all the show, set, etc. commands that do not support the JSON format before we execute them.

> The suggestion below shouldn't be confusing for scripts.
>
>>
>>> We could also invent a new format for the case where JSON format is
>>> requested, but the callback doesn't support it.  E.g. by creating a
>>> reply like this: {"reply-format": "plain", "reply": "<plain text>"}
>>> (for the plain text replies only).
>>
>>>>
>>>>
>>>>
>>>>>
>>>>>>>>> Some small style comments highlighted below.
>>>>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jakob
>>>>
>>

On 3/19/24 15:54, Eelco Chaudron wrote:
> 
> 
> On 19 Mar 2024, at 15:46, Ilya Maximets wrote:
> 
>> On 3/19/24 15:17, Eelco Chaudron wrote:
>>>
>>>
>>> On 19 Mar 2024, at 15:00, Ilya Maximets wrote:
>>>
>>>> On 3/19/24 14:41, Jakob Meng wrote:
>>>>>
>>>>>
>>>>> On 19.03.24 13:22, Ilya Maximets wrote:
>>>>>> On 3/19/24 13:21, Ilya Maximets wrote:
>>>>>>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>>>>>>
>>>>>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>>>>>>
>>>>>>>>> Hi!
>>>>>>>>>
>>>>>>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>>>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>>>>>>
>>>>>>>>>>> ...
>>>>>>>>>> Thank for the patch! What a beast to go trough ;)
>>>>>>>>> Thank you for doing it anyway ☺️
>>>>>>>>>
>>>>>>>>>> I believe the current approach is acceptable. However, we could also
>>>>>>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>>>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>>>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>>>>>>> suggestion, as I'm ok with the current approach. :)
>>>>>>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>>>>>>
>>>>>>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>>>>>>> Or have a utility function to return the format, which keeps the structure invisible.
>>>>>>> +1, just add some access methods.
>>>>>>>
>>>>>>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>>>>>>> I’ll wait for others to chime in...
>>>>>>> I don't think changing all the callbacks is a good thing to do.
>>>>>>> i.e. this patch should not exist or be very small.
>>>>>> * or it should be very small.
>>>>>
>>>>> How about unixctl_command_register?
>>>>>
>>>>> Should we change the existing function signature (like I did)?
>>>>> Or add a second function, like unixctl_command_register_fmt?
>>>>> Or should we add another function to set/change supported formats of an already registered command?
>>>>>
>>>>> Example:
>>>>>
>>>>> // Get output format which the user has choosen (defaults to txt)
>>>>> enum ovs_output_fmt
>>>>> unixctl_command_get_output_format(struct unixctl_conn *);
>>>>>
>>>>> // Set output formats that a command supports
>>>>> int
>>>>> unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);
>>>>>
>>>>> Wdyt?
>>>>
>>>> I'm actually not sure why we need to register them differently in the
>>>> first palace.  Make the callback check what was the requested format
>>>> and use appropriate reply function.
>>>
>>> I guess this can be done with the unixctl_command_get_output_format() function?
>>>
>>>> Why we need to fail the requests
>>>> for callbacks that don't support JSON replies?  Why can't we just
>>>> pack the plain reply into a JSON string?
>>>
>>> I don’t like this, this way we will not know if the command actually supports native JSON or not. Else we might confuse scripts in the future if we start sending some JSONized responses. returning an error might also trigger people to submit a patch to convert more commands to native JSON support :)
>>
>> The main problem is that we shouldn't fail commands after they are
>> already executed.  It may work for 'show' commands, but it's not
>> a thing that can be done for 'set' commands.
> 
> I guess this means I would prefer to take a ‘bigger’ change hit and change the register procedure (not the actual callback), to include the supported output formats. This way we can still error out all the show, set, etc. commands that do not support the JSON format before we execute them.

Failing the request because it doesn't support the output format is
also ugly.  For example, the list-commands doesn't support it, so
the user that enabled JSON format on the socket will not be able to
even list the commands.  I don't think that's a good interface.
I'd rather have just a new command then, e.g. 'dpif/show+json' that
would reply in a JSON format and void all the questionable API changes.

> 
>> The suggestion below shouldn't be confusing for scripts.
>>
>>>
>>>> We could also invent a new format for the case where JSON format is
>>>> requested, but the callback doesn't support it.  E.g. by creating a
>>>> reply like this: {"reply-format": "plain", "reply": "<plain text>"}
>>>> (for the plain text replies only).
>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>>>>> Some small style comments highlighted below.
>>>>>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Jakob
>>>>>
>>>
>

On 19 Mar 2024, at 16:19, Ilya Maximets wrote:

> On 3/19/24 15:54, Eelco Chaudron wrote:
>>
>>
>> On 19 Mar 2024, at 15:46, Ilya Maximets wrote:
>>
>>> On 3/19/24 15:17, Eelco Chaudron wrote:
>>>>
>>>>
>>>> On 19 Mar 2024, at 15:00, Ilya Maximets wrote:
>>>>
>>>>> On 3/19/24 14:41, Jakob Meng wrote:
>>>>>>
>>>>>>
>>>>>> On 19.03.24 13:22, Ilya Maximets wrote:
>>>>>>> On 3/19/24 13:21, Ilya Maximets wrote:
>>>>>>>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>>>>>>>
>>>>>>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>>>>>>>
>>>>>>>>>> Hi!
>>>>>>>>>>
>>>>>>>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>>>>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>>>>>>>
>>>>>>>>>>>> ...
>>>>>>>>>>> Thank for the patch! What a beast to go trough ;)
>>>>>>>>>> Thank you for doing it anyway ☺️
>>>>>>>>>>
>>>>>>>>>>> I believe the current approach is acceptable. However, we could also
>>>>>>>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>>>>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>>>>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>>>>>>>> suggestion, as I'm ok with the current approach. :)
>>>>>>>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>>>>>>>
>>>>>>>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>>>>>>>> Or have a utility function to return the format, which keeps the structure invisible.
>>>>>>>> +1, just add some access methods.
>>>>>>>>
>>>>>>>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>>>>>>>> I’ll wait for others to chime in...
>>>>>>>> I don't think changing all the callbacks is a good thing to do.
>>>>>>>> i.e. this patch should not exist or be very small.
>>>>>>> * or it should be very small.
>>>>>>
>>>>>> How about unixctl_command_register?
>>>>>>
>>>>>> Should we change the existing function signature (like I did)?
>>>>>> Or add a second function, like unixctl_command_register_fmt?
>>>>>> Or should we add another function to set/change supported formats of an already registered command?
>>>>>>
>>>>>> Example:
>>>>>>
>>>>>> // Get output format which the user has choosen (defaults to txt)
>>>>>> enum ovs_output_fmt
>>>>>> unixctl_command_get_output_format(struct unixctl_conn *);
>>>>>>
>>>>>> // Set output formats that a command supports
>>>>>> int
>>>>>> unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);
>>>>>>
>>>>>> Wdyt?
>>>>>
>>>>> I'm actually not sure why we need to register them differently in the
>>>>> first palace.  Make the callback check what was the requested format
>>>>> and use appropriate reply function.
>>>>
>>>> I guess this can be done with the unixctl_command_get_output_format() function?
>>>>
>>>>> Why we need to fail the requests
>>>>> for callbacks that don't support JSON replies?  Why can't we just
>>>>> pack the plain reply into a JSON string?
>>>>
>>>> I don’t like this, this way we will not know if the command actually supports native JSON or not. Else we might confuse scripts in the future if we start sending some JSONized responses. returning an error might also trigger people to submit a patch to convert more commands to native JSON support :)
>>>
>>> The main problem is that we shouldn't fail commands after they are
>>> already executed.  It may work for 'show' commands, but it's not
>>> a thing that can be done for 'set' commands.
>>
>> I guess this means I would prefer to take a ‘bigger’ change hit and change the register procedure (not the actual callback), to include the supported output formats. This way we can still error out all the show, set, etc. commands that do not support the JSON format before we execute them.
>
> Failing the request because it doesn't support the output format is
> also ugly.  For example, the list-commands doesn't support it, so
> the user that enabled JSON format on the socket will not be able to
> even list the commands.  I don't think that's a good interface.
> I'd rather have just a new command then, e.g. 'dpif/show+json' that
> would reply in a JSON format and void all the questionable API changes.

I was looking at this from a single transaction perspective, and then I’m fine with it failing. If you ask for json on the list-command and it does not support it, the error would be clear enough. Not sure why you would feel like this is ugly.

But you are right, in the context of keeping the socket open and doing multiple transactions, this might not be desirable. If we go for the below option, we can have a WARNING on stderr that the command does not support JSON output, so it uses the “plain” output method.

>>
>>> The suggestion below shouldn't be confusing for scripts.
>>>
>>>>
>>>>> We could also invent a new format for the case where JSON format is
>>>>> requested, but the callback doesn't support it.  E.g. by creating a
>>>>> reply like this: {"reply-format": "plain", "reply": "<plain text>"}
>>>>> (for the plain text replies only).
>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>>>> Some small style comments highlighted below.
>>>>>>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Jakob
>>>>>>
>>>>
>>

On 19.03.24 16:41, Eelco Chaudron wrote:
>
> On 19 Mar 2024, at 16:19, Ilya Maximets wrote:
>
>> On 3/19/24 15:54, Eelco Chaudron wrote:
>>>
>>> On 19 Mar 2024, at 15:46, Ilya Maximets wrote:
>>>
>>>> On 3/19/24 15:17, Eelco Chaudron wrote:
>>>>>
>>>>> On 19 Mar 2024, at 15:00, Ilya Maximets wrote:
>>>>>
>>>>>> On 3/19/24 14:41, Jakob Meng wrote:
>>>>>>>
>>>>>>> On 19.03.24 13:22, Ilya Maximets wrote:
>>>>>>>> On 3/19/24 13:21, Ilya Maximets wrote:
>>>>>>>>> On 3/19/24 13:17, Eelco Chaudron wrote:
>>>>>>>>>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi!
>>>>>>>>>>>
>>>>>>>>>>> On 15.03.24 11:19, Eelco Chaudron wrote:
>>>>>>>>>>>> On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> ...
>>>>>>>>>>>> Thank for the patch! What a beast to go trough ;)
>>>>>>>>>>> Thank you for doing it anyway ☺️
>>>>>>>>>>>
>>>>>>>>>>>> I believe the current approach is acceptable. However, we could also
>>>>>>>>>>>> incorporate union callbacks: if registration only supports text, we would
>>>>>>>>>>>> use callback A, and if multiple formats exist, we could employ the new style
>>>>>>>>>>>> callback. This would mitigate the need for a significant overhaul. Just a
>>>>>>>>>>>> suggestion, as I'm ok with the current approach. :)
>>>>>>>>>>> This trades many-initial-changes-across-a-lot-of-files for a more-complex-registration-and-callback-logic.
>>>>>>>>>>>
>>>>>>>>>>> We could also make struct unixctl_conn visible (move it from lib/unixctl.c to lib/unixctl.h) and let callees read the output format from its 'fmt' member. Then we would not have to change the callback signature at all.
>>>>>>>>>> Or have a utility function to return the format, which keeps the structure invisible.
>>>>>>>>> +1, just add some access methods.
>>>>>>>>>
>>>>>>>>>>> I do not have a strong opinion here. What do you, Ilya and the others think?
>>>>>>>>>> I’ll wait for others to chime in...
>>>>>>>>> I don't think changing all the callbacks is a good thing to do.
>>>>>>>>> i.e. this patch should not exist or be very small.
>>>>>>>> * or it should be very small.
>>>>>>> How about unixctl_command_register?
>>>>>>>
>>>>>>> Should we change the existing function signature (like I did)?
>>>>>>> Or add a second function, like unixctl_command_register_fmt?
>>>>>>> Or should we add another function to set/change supported formats of an already registered command?
>>>>>>>
>>>>>>> Example:
>>>>>>>
>>>>>>> // Get output format which the user has choosen (defaults to txt)
>>>>>>> enum ovs_output_fmt
>>>>>>> unixctl_command_get_output_format(struct unixctl_conn *);
>>>>>>>
>>>>>>> // Set output formats that a command supports
>>>>>>> int
>>>>>>> unixctl_command_set_output_formats(const char *name, unsigned int output_fmts);
>>>>>>>
>>>>>>> Wdyt?
>>>>>> I'm actually not sure why we need to register them differently in the
>>>>>> first palace.  Make the callback check what was the requested format
>>>>>> and use appropriate reply function.
>>>>> I guess this can be done with the unixctl_command_get_output_format() function?
>>>>>
>>>>>> Why we need to fail the requests
>>>>>> for callbacks that don't support JSON replies?  Why can't we just
>>>>>> pack the plain reply into a JSON string?
>>>>> I don’t like this, this way we will not know if the command actually supports native JSON or not. Else we might confuse scripts in the future if we start sending some JSONized responses. returning an error might also trigger people to submit a patch to convert more commands to native JSON support :)
>>>> The main problem is that we shouldn't fail commands after they are
>>>> already executed.  It may work for 'show' commands, but it's not
>>>> a thing that can be done for 'set' commands.
>>> I guess this means I would prefer to take a ‘bigger’ change hit and change the register procedure (not the actual callback), to include the supported output formats. This way we can still error out all the show, set, etc. commands that do not support the JSON format before we execute them.
>> Failing the request because it doesn't support the output format is
>> also ugly.  For example, the list-commands doesn't support it, so
>> the user that enabled JSON format on the socket will not be able to
>> even list the commands.  I don't think that's a good interface.
>> I'd rather have just a new command then, e.g. 'dpif/show+json' that
>> would reply in a JSON format and void all the questionable API changes.
> I was looking at this from a single transaction perspective, and then I’m fine with it failing. If you ask for json on the list-command and it does not support it, the error would be clear enough. Not sure why you would feel like this is ugly.

Ilya's 'dpif/show+json' approach has the benefit of a less invasive change. Both 'dpif/show' and 'dpif/show+json' will call different code paths (regardless of the approach we choose), often in form of different functions. Passing the output format as another function argument (like I currently do) requires us to put additional branching code in front of these two implementations. In worst case we have three functions. With Ilya's approach, we can get rid of this branching code, reducing it to two functions(==commands).

IIuc one downside is that we would no longer provide a global ovs-appctl flag for choosing the output format. Offering a global flag that would cause ovs-appctl to magically choose the '+json' commands would be a hack!?!

Also, how do we handle the pretty flag? Eelco suggested earlier that a '--pretty' flag should fail if something other than JSON output had been selected. Without a global "--format" flag, we would not know what kind of output a command would return. Except if we parse the command name and derive the output format from the '+json' postfix (what a hack..).

Probably less relevant: This approach does not scale well if we had to decide to offer more behavior changing flags. Consider a '--dry-run' flag (just made up). Would we offer four commands? 'dpif/show', 'dpif/show+json', 'dpif/show+dry-run' and 'dpif/show+dry-run+json'?

Difficult tradeoffs 🤔 I do not have a strong preference..

> But you are right, in the context of keeping the socket open and doing multiple transactions, this might not be desirable. If we go for the below option, we can have a WARNING on stderr that the command does not support JSON output, so it uses the “plain” output method.

The socket will not be closed if the user asked for JSON but the requested command does not support it. The user would simply get an error message.

It the user's responsibility to call functions in the correct order with correct arguments, isn't it?

In a multiple transactions scenario, the user could call "set-options --format json", then call a command which supports JSON output, then use "set-options --format text" to switch back to the old mode before calling "list-commands". If the user would forget the second "set-options" call, a JSON error would be returned but the socket would still be open.

When commands would register their supported output formats upfront (like I do atm or with discussed unixctl_command_set_output_formats), then "list-commands" (or any other command) would not be executed. The JSON error would be returned early, the commands themselves would not have to fail (which would not be feasible for "set" commands iiuc).

>>>> The suggestion below shouldn't be confusing for scripts.
>>>>
>>>>>> We could also invent a new format for the case where JSON format is
>>>>>> requested, but the callback doesn't support it.  E.g. by creating a
>>>>>> reply like this: {"reply-format": "plain", "reply": "<plain text>"}
>>>>>> (for the plain text replies only).
>>>>>>>
>>>>>>>
>>>>>>>>>>>> Some small style comments highlighted below.
>>>>>>>>>>> I would like to have your eagle eyes...👀 Fixed them!
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Jakob