[ovs-dev,v9,1/6] Add global option for JSON output to ovs-appctl.

From: Jakob Meng <code@jakobmeng.de>

For monitoring systems such as Prometheus it would be beneficial if
OVS would expose statistics in a machine-readable format.

This patch introduces support for different output formats to ovs-xxx
tools, in particular ovs-appctl. The latter gains a global option
'-f,--format' which changes it to print a JSON document instead of
plain-text for humans. For example, a later patch implements support
for 'ovs-appctl --format json dpif/show'. By default, the output format
is plain-text as before.

A new 'set-options' command has been added to lib/unixctl.c which
allows to change the output format of the commands executed afterwards
on the same socket connection. It is supposed to be run by ovs-xxx
tools transparently for the user when a specific output format has been
requested.
For example, when a user calls 'ovs-appctl --format json dpif/show',
then ovs-appctl will call 'set-options' to set the output format as
requested by the user and afterwards it will call the actual command
'dpif/show'.
This ovs-appctl behaviour has been implemented in a backward compatible
way. One can use an updated client (ovs-appctl) with an old server
(ovs-vswitchd) and vice versa. Of course, JSON output only works when
both sides have been updated.

Two access functions unixctl_command_{get,set}_output_format() and
two unixctl_command_reply_{,error_}json functions have been added to
lib/unixctl.h:
unixctl_command_get_output_format() is supposed to be used in commands
like 'dpif/show' to query the requested output format. When JSON output
has been selected, both unixctl_command_reply_{,error_}json() functions
can be used to return JSON objects to the client (ovs-appctl) instead
of plain-text with the unixctl_command_reply{,_error}() functions.

When JSON has been requested but a command has not implemented JSON
output the plain-text output will be wrapped in a provisional JSON
document with the following structure:

  {"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}

Thus commands which have been executed successfully will not fail when
they try to render the output at a later stage.

In popular tools like kubectl the option for output control is usually
called '-o|--output' instead of '-f,--format'. But ovs-appctl already
has an short option '-o' which prints the available ovs-appctl options
('--option'). The now choosen name also better alignes with
ovsdb-client where '-f,--format' controls output formatting.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng <code@jakobmeng.de>
---
 NEWS                   |   3 +
 lib/command-line.c     |  36 ++++++++++
 lib/command-line.h     |  10 +++
 lib/unixctl.c          | 158 ++++++++++++++++++++++++++++++++---------
 lib/unixctl.h          |  11 +++
 tests/ovs-vswitchd.at  |  11 +++
 utilities/ovs-appctl.c |  98 +++++++++++++++++++++----
 7 files changed, 277 insertions(+), 50 deletions(-)

Bleep bloop.  Greetings Jakob Meng, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#583 FILE: utilities/ovs-appctl.c:145:
  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\

Lines checked: 675, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On 4/12/24 09:26, jmeng@redhat.com wrote:
> From: Jakob Meng <code@jakobmeng.de>
> 
> For monitoring systems such as Prometheus it would be beneficial if
> OVS would expose statistics in a machine-readable format.
> 
> This patch introduces support for different output formats to ovs-xxx
> tools, in particular ovs-appctl.

What other tools you plan to add support to?
Database tools already have the machine-readable output from the database.
appctl should generelly be used instead of ovs-dpctl.  What else?

I'd suggest to not try to make this generic.

> The latter gains a global option
> '-f,--format' which changes it to print a JSON document instead of
> plain-text for humans. For example, a later patch implements support
> for 'ovs-appctl --format json dpif/show'. By default, the output format
> is plain-text as before.
> 
> A new 'set-options' command has been added to lib/unixctl.c which
> allows to change the output format of the commands executed afterwards
> on the same socket connection. It is supposed to be run by ovs-xxx

It's specific for appctl unility, nothing else is using unixctl.

> tools transparently for the user when a specific output format has been
> requested.
> For example, when a user calls 'ovs-appctl --format json dpif/show',
> then ovs-appctl will call 'set-options' to set the output format as
> requested by the user and afterwards it will call the actual command
> 'dpif/show'.
> This ovs-appctl behaviour has been implemented in a backward compatible
> way. One can use an updated client (ovs-appctl) with an old server
> (ovs-vswitchd) and vice versa. Of course, JSON output only works when
> both sides have been updated.
> 
> Two access functions unixctl_command_{get,set}_output_format() and
> two unixctl_command_reply_{,error_}json functions have been added to
> lib/unixctl.h:
> unixctl_command_get_output_format() is supposed to be used in commands
> like 'dpif/show' to query the requested output format. When JSON output
> has been selected, both unixctl_command_reply_{,error_}json() functions
> can be used to return JSON objects to the client (ovs-appctl) instead
> of plain-text with the unixctl_command_reply{,_error}() functions.
> 
> When JSON has been requested but a command has not implemented JSON
> output the plain-text output will be wrapped in a provisional JSON
> document with the following structure:
> 
>   {"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}
> 
> Thus commands which have been executed successfully will not fail when
> they try to render the output at a later stage.
> 
> In popular tools like kubectl the option for output control is usually
> called '-o|--output' instead of '-f,--format'. But ovs-appctl already
> has an short option '-o' which prints the available ovs-appctl options
> ('--option'). The now choosen name also better alignes with
> ovsdb-client where '-f,--format' controls output formatting.
> 
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <code@jakobmeng.de>
> ---
>  NEWS                   |   3 +
>  lib/command-line.c     |  36 ++++++++++
>  lib/command-line.h     |  10 +++
>  lib/unixctl.c          | 158 ++++++++++++++++++++++++++++++++---------
>  lib/unixctl.h          |  11 +++
>  tests/ovs-vswitchd.at  |  11 +++
>  utilities/ovs-appctl.c |  98 +++++++++++++++++++++----
>  7 files changed, 277 insertions(+), 50 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index b92cec532..03ef6486b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,9 @@ Post-v3.3.0
>     - The primary development branch has been renamed from 'master' to 'main'.
>       The OVS tree remains hosted on GitHub.
>       https://github.com/openvswitch/ovs.git
> +   - ovs-appctl:

'o' goes before 't' or 'u', so I'd put this entry to the top.

> +     * Added new option [-f|--format] to choose the output format, e.g. 'json'
> +       or 'text' (by default).

A man page update is missing:
  Documentation/ref/ovs-appctl.8.rst

>  
>  
>  v3.3.0 - 16 Feb 2024
> diff --git a/lib/command-line.c b/lib/command-line.c
> index 967f4f5d5..775e0430a 100644
> --- a/lib/command-line.c
> +++ b/lib/command-line.c

These changes should be moved to unixctl.[ch].

> @@ -24,6 +24,7 @@
>  #include "ovs-thread.h"
>  #include "util.h"
>  #include "openvswitch/vlog.h"
> +#include "openvswitch/json.h"
>  
>  VLOG_DEFINE_THIS_MODULE(command_line);
>  
> @@ -53,6 +54,41 @@ ovs_cmdl_long_options_to_short_options(const struct option options[])
>      return xstrdup(short_options);
>  }
>  
> +const char *
> +ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
> +{
> +    switch (fmt) {
> +    case OVS_OUTPUT_FMT_TEXT:
> +        return "text";
> +
> +    case OVS_OUTPUT_FMT_JSON:
> +        return "json";
> +
> +    default:
> +        return NULL;
> +    }
> +}
> +
> +struct json *
> +ovs_output_fmt_to_json(enum ovs_output_fmt fmt)
> +{
> +    const char *string = ovs_output_fmt_to_string(fmt);
> +    return string ? json_string_create(string) : NULL;
> +}
> +
> +bool
> +ovs_output_fmt_from_string(const char *string, enum ovs_output_fmt *fmt)
> +{
> +    if (!strcmp(string, "text")) {
> +        *fmt = OVS_OUTPUT_FMT_TEXT;
> +    } else if (!strcmp(string, "json")) {
> +        *fmt = OVS_OUTPUT_FMT_JSON;
> +    } else {
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static char * OVS_WARN_UNUSED_RESULT
>  build_short_options(const struct option *long_options)
>  {
> diff --git a/lib/command-line.h b/lib/command-line.h
> index fc2a2690f..494274cec 100644
> --- a/lib/command-line.h
> +++ b/lib/command-line.h
> @@ -20,6 +20,7 @@
>  /* Utilities for command-line parsing. */
>  
>  #include "compiler.h"
> +#include <openvswitch/list.h>

Seems unnecessary.

>  
>  struct option;
>  
> @@ -46,6 +47,15 @@ struct ovs_cmdl_command {
>  
>  char *ovs_cmdl_long_options_to_short_options(const struct option *options);
>  
> +enum ovs_output_fmt {
> +    OVS_OUTPUT_FMT_TEXT = 1 << 0,
> +    OVS_OUTPUT_FMT_JSON = 1 << 1
> +};
> +
> +const char *ovs_output_fmt_to_string(enum ovs_output_fmt);
> +struct json *ovs_output_fmt_to_json(enum ovs_output_fmt);
> +bool ovs_output_fmt_from_string(const char *, enum ovs_output_fmt *);

Move to unixctl.h.  And re-name accordingly.
And 'struct json;' forward declaration is missing.

> +
>  struct ovs_cmdl_parsed_option {
>      const struct option *o;
>      char *arg;
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index 103357ee9..18638d86a 100644
> --- a/lib/unixctl.c
> +++ b/lib/unixctl.c
> @@ -21,7 +21,6 @@
>  #include "coverage.h"
>  #include "dirs.h"
>  #include "openvswitch/dynamic-string.h"
> -#include "openvswitch/json.h"
>  #include "jsonrpc.h"
>  #include "openvswitch/list.h"
>  #include "openvswitch/poll-loop.h"
> @@ -50,6 +49,8 @@ struct unixctl_conn {
>      /* Only one request can be in progress at a time.  While the request is
>       * being processed, 'request_id' is populated, otherwise it is null. */
>      struct json *request_id;   /* ID of the currently active request. */
> +
> +    enum ovs_output_fmt fmt;

A comment is needed.

>  };
>  
>  /* Server for control connection. */
> @@ -94,6 +95,39 @@ unixctl_version(struct unixctl_conn *conn, int argc OVS_UNUSED,
>      unixctl_command_reply(conn, ovs_get_program_version());
>  }
>  
> +static void
> +unixctl_set_options(struct unixctl_conn *conn, int argc, const char *argv[],
> +                    void *aux OVS_UNUSED)
> +{
> +    char * error = NULL;
> +
> +    for (size_t i = 1; i < argc; i++) {
> +        if ((i + 1) == argc) {
> +            error = xasprintf("option requires an argument -- %s", argv[i]);
> +            goto error;
> +        } else if (!strcmp(argv[i], "--format")) {
> +            /* Move index to option argument. */
> +            i++;
> +
> +            /* Parse output format argument. */
> +            if (!ovs_output_fmt_from_string(argv[i], &conn->fmt)) {
> +                error = xasprintf("option %s has invalid value %s",
> +                                  argv[i - 1], argv[i]);
> +                goto error;
> +            }
> +        } else {
> +            error = xasprintf("invalid option -- %s", argv[i]);
> +            goto error;

There is no need for 'goto'.  We can just break and check if the
'error' is set after the loop.

But also,  This code is a little complex.  We only expect one option,
so we may just check for that instead.  If we want a generic option
handling, then we should probbaly use ovs_cmdl_parse_all() instead.

> +
> +        }
> +    }
> +    unixctl_command_reply(conn, NULL);
> +    return;
> +error:
> +    unixctl_command_reply_error(conn, 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.)
> @@ -128,36 +162,68 @@ unixctl_command_register(const char *name, const char *usage,
>      shash_add(&commands, name, command);
>  }
>  
> -static void
> -unixctl_command_reply__(struct unixctl_conn *conn,
> -                        bool success, const char *body)
> +enum ovs_output_fmt
> +unixctl_command_get_output_format(struct unixctl_conn *conn)
>  {
> -    struct json *body_json;
> -    struct jsonrpc_msg *reply;
> +    return conn->fmt;
> +}
>  
> -    COVERAGE_INC(unixctl_replied);
> -    ovs_assert(conn->request_id);
> +void
> +unixctl_command_set_output_format(struct unixctl_conn *conn,
> +                                  enum ovs_output_fmt fmt)
> +{
> +    conn->fmt = fmt;
> +}
> +
> +static struct json *
> +json_reply_create__(struct unixctl_conn *conn, const char *body)
> +{
> +    struct json * json_body;
>  
>      if (!body) {
>          body = "";
>      }
>  
>      if (body[0] && body[strlen(body) - 1] != '\n') {
> -        body_json = json_string_create_nocopy(xasprintf("%s\n", body));
> +        json_body = json_string_create_nocopy(xasprintf("%s\n", body));
>      } else {
> -        body_json = json_string_create(body);
> +        json_body = json_string_create(body);
>      }
>  
> +    if (conn->fmt == OVS_OUTPUT_FMT_TEXT) {
> +        return json_body;
> +    }
> +
> +    struct json *json_reply = json_object_create();
> +
> +    json_object_put_string(json_reply, "reply-format", "plain");
> +    json_object_put(json_reply, "reply", json_body);
> +
> +    return json_reply;
> +}
> +
> +/* Takes ownership of 'body'. */

of the

> +static void
> +unixctl_command_reply__(struct unixctl_conn *conn,
> +                        bool success, struct json *body)
> +{
> +    struct jsonrpc_msg *reply;
> +
> +    COVERAGE_INC(unixctl_replied);
> +    ovs_assert(conn->request_id);
> +
>      if (success) {
> -        reply = jsonrpc_create_reply(body_json, conn->request_id);
> +        reply = jsonrpc_create_reply(body, conn->request_id);
>      } else {
> -        reply = jsonrpc_create_error(body_json, conn->request_id);
> +        reply = jsonrpc_create_error(body, conn->request_id);
>      }
>  
>      if (VLOG_IS_DBG_ENABLED()) {
>          char *id = json_to_string(conn->request_id, 0);
> +        char *msg = json_to_string(body, 0);

JSSF_SORT is needed, otherwise it's hard to compare logs.
Wihtout it the same object can be serialized differently.

>          VLOG_DBG("replying with %s, id=%s: \"%s\"",
> -                 success ? "success" : "error", id, body);
> +                 success ? "success" : "error", id, msg);
> +        free(msg);
>          free(id);
>      }
>  
> @@ -170,20 +236,40 @@ unixctl_command_reply__(struct unixctl_conn *conn,
>  
>  /* Replies to the active unixctl connection 'conn'.  'result' is sent to the
>   * client indicating the command was processed successfully.  Only one call to
> - * unixctl_command_reply() or unixctl_command_reply_error() may be made per
> - * request. */
> + * the unixctl_command_reply*() functions may be made per request. */

'the' should not be here.

>  void
>  unixctl_command_reply(struct unixctl_conn *conn, const char *result)
>  {
> -    unixctl_command_reply__(conn, true, result);
> +    unixctl_command_reply__(conn, true, json_reply_create__(conn, result));
> +}
> +
> +/* Replies to the active unixctl connection 'conn'.  'result' is sent to the
> + * client indicating the command was processed successfully.  Only one call to
> + * the unixctl_command_reply*() functions may be made per request.

'the' should not be here.

> + *
> + * Takes ownership of 'body'. */

'of the'

But also the comment should explain the difference between '_json'
and the normal version of the function and when they need to be used.

> +void
> +unixctl_command_reply_json(struct unixctl_conn *conn, struct json *body)
> +{

Maybe assert the output format?

> +    unixctl_command_reply__(conn, true, body);
>  }
>  
>  /* Replies to the active unixctl connection 'conn'. 'error' is sent to the
> - * client indicating an error occurred processing the command.  Only one call to
> - * unixctl_command_reply() or unixctl_command_reply_error() may be made per
> - * request. */
> + * client indicating an error occurred processing the command.  Only one call
> + * to the unixctl_command_reply*() functions may be made per request. */

'the' should not be here.

>  void
>  unixctl_command_reply_error(struct unixctl_conn *conn, const char *error)
> +{
> +    unixctl_command_reply__(conn, false, json_reply_create__(conn, error));

This will wrap an error into a json object.  I don't think this is a
desireable behavior.

> +}
> +
> +/* Replies to the active unixctl connection 'conn'.  'error' is sent to the
> + * client indicating an error occurred processing the command.  Only one call
> + * to the unixctl_command_reply*() functions may be made per request.

'the' should not be here.

> + *
> + * Takes ownership of 'error'. */

'of the'

> +void
> +unixctl_command_reply_error_json(struct unixctl_conn *conn, struct json *error)
>  {
>      unixctl_command_reply__(conn, false, error);

I'm not sure we need this function.

>  }
> @@ -250,6 +336,8 @@ unixctl_server_create(const char *path, struct unixctl_server **serverp)
>      unixctl_command_register("list-commands", "", 0, 0, unixctl_list_commands,
>                               NULL);
>      unixctl_command_register("version", "", 0, 0, unixctl_version, NULL);
> +    unixctl_command_register("set-options", "[--format text|json]", 2, 2,

'[]' means optional, but the minimum number of args is set to 2.

> +                             unixctl_set_options, NULL);
>  
>      struct unixctl_server *server = xmalloc(sizeof *server);
>      server->listener = listener;
> @@ -381,6 +469,7 @@ unixctl_server_run(struct unixctl_server *server)
>              struct unixctl_conn *conn = xzalloc(sizeof *conn);
>              ovs_list_push_back(&server->conns, &conn->node);
>              conn->rpc = jsonrpc_open(stream);
> +            conn->fmt = OVS_OUTPUT_FMT_TEXT;
>          } else if (error == EAGAIN) {
>              break;
>          } else {
> @@ -483,10 +572,12 @@ unixctl_client_create(const char *path, struct jsonrpc **client)
>   * '*err' if not NULL. */
>  int
>  unixctl_client_transact(struct jsonrpc *client, const char *command, int argc,
> -                        char *argv[], char **result, char **err)
> +                        char *argv[], enum ovs_output_fmt fmt, char **result,
> +                        char **err)
>  {
>      struct jsonrpc_msg *request, *reply;
> -    struct json **json_args, *params;
> +    struct json **json_args, *params, *output_src;

Reverse x-mass tree.

> +    char **output_dst;
>      int error, i;
>  
>      *result = NULL;
> @@ -506,22 +597,19 @@ unixctl_client_transact(struct jsonrpc *client, const char *command, int argc,
>          return error;
>      }
>  
> -    if (reply->error) {
> -        if (reply->error->type == JSON_STRING) {
> -            *err = xstrdup(json_string(reply->error));
> +    output_src = reply->error ? reply->error : reply->result;
> +    output_dst = reply->error ? err : result;
> +
> +    if (output_src) {
> +        if (fmt == OVS_OUTPUT_FMT_TEXT && output_src->type == JSON_STRING) {
> +            *output_dst = xstrdup(json_string(output_src));
> +        } else if (fmt == OVS_OUTPUT_FMT_JSON) {
> +            *output_dst = json_to_string(output_src, 0);

It seems a little strange that this function is aware of the reply
type and the format.  It should not handle the data representation
for the the end user, it's not part of the transaction.  Can we just
return the json to the caller and make the caller decide what to do
with it?

>          } else {
> -            VLOG_WARN("%s: unexpected error type in JSON RPC reply: %s",
> +            VLOG_WARN("%s: unexpected %s type in JSON rpc reply: %s",
>                        jsonrpc_get_name(client),
> -                      json_type_to_string(reply->error->type));
> -            error = EINVAL;
> -        }
> -    } else if (reply->result) {
> -        if (reply->result->type == JSON_STRING) {
> -            *result = xstrdup(json_string(reply->result));
> -        } else {
> -            VLOG_WARN("%s: unexpected result type in JSON rpc reply: %s",
> -                      jsonrpc_get_name(client),
> -                      json_type_to_string(reply->result->type));
> +                      reply->error ? "error" : "result",
> +                      json_type_to_string(output_src->type));

There is no need to have non-string errors.

>              error = EINVAL;
>          }
>      }
> diff --git a/lib/unixctl.h b/lib/unixctl.h
> index 4562dbc49..b30bcf092 100644
> --- a/lib/unixctl.h
> +++ b/lib/unixctl.h
> @@ -17,6 +17,9 @@
>  #ifndef UNIXCTL_H
>  #define UNIXCTL_H 1
>  
> +#include "openvswitch/json.h"
> +#include "command-line.h"

Don't include headers into headers.  Forward-declare missing
types instead.

> +
>  #ifdef  __cplusplus
>  extern "C" {
>  #endif
> @@ -36,6 +39,7 @@ int unixctl_client_create(const char *path, struct jsonrpc **client);
>  int unixctl_client_transact(struct jsonrpc *client,
>                              const char *command,
>                              int argc, char *argv[],
> +                            enum ovs_output_fmt fmt,
>                              char **result, char **error);
>  
>  /* Command registration. */
> @@ -45,8 +49,15 @@ typedef void unixctl_cb_func(struct unixctl_conn *,
>  void unixctl_command_register(const char *name, const char *usage,
>                                int min_args, int max_args,
>                                unixctl_cb_func *cb, void *aux);
> +enum ovs_output_fmt unixctl_command_get_output_format(struct unixctl_conn *);
> +void unixctl_command_set_output_format(struct unixctl_conn *,
> +                                       enum ovs_output_fmt);
>  void unixctl_command_reply_error(struct unixctl_conn *, const char *error);
> +void unixctl_command_reply_error_json(struct unixctl_conn *,
> +                                      struct json *error);
>  void unixctl_command_reply(struct unixctl_conn *, const char *body);
> +void unixctl_command_reply_json(struct unixctl_conn *,
> +                                struct json *body);
>  
>  #ifdef  __cplusplus
>  }
> diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
> index 977b2eba1..32ca2c6e7 100644
> --- a/tests/ovs-vswitchd.at
> +++ b/tests/ovs-vswitchd.at
> @@ -265,3 +265,14 @@ OFPT_FEATURES_REPLY: dpid:$orig_dpid
>  
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([ovs-vswitchd version])
> +OVS_VSWITCHD_START
> +
> +AT_CHECK([ovs-appctl version], [0], [ignore])
> +ovs_version=$(ovs-appctl version)
> +
> +AT_CHECK_UNQUOTED([ovs-appctl --format json version], [0], [dnl
> +{"reply-format":"plain","reply":"$ovs_version\n"}])

If the output is not sorted, this check may fail.

Also, I'm not sure about the line break character in the JSON output.
We add it only because we want the final new line when printing the
output to the terminal.  It shoudln't be in the JSON reply itself.
I'd suggest to move the adition of the new line to the client in case
the output is text and it doesn't contain the new line already.
This should be backward compatible.

> +
> +AT_CLEANUP
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index ba0c172e6..4d4503597 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -29,44 +29,84 @@
>  #include "jsonrpc.h"
>  #include "process.h"
>  #include "timeval.h"
> +#include "svec.h"
>  #include "unixctl.h"
>  #include "util.h"
>  #include "openvswitch/vlog.h"
>  
>  static void usage(void);
> -static const char *parse_command_line(int argc, char *argv[]);
> +
> +/* Parsed command line args. */
> +struct cmdl_args {
> +    enum ovs_output_fmt format;
> +    char *target;
> +};
> +
> +static struct cmdl_args *cmdl_args_create(void);
> +static void cmdl_args_destroy(struct cmdl_args *);
> +static struct cmdl_args *parse_command_line(int argc, char *argv[]);
>  static struct jsonrpc *connect_to_target(const char *target);
>  
>  int
>  main(int argc, char *argv[])
>  {
> +    struct svec opt_argv = SVEC_EMPTY_INITIALIZER;
>      char *cmd_result, *cmd_error;
>      struct jsonrpc *client;
> +    struct cmdl_args *args;
>      char *cmd, **cmd_argv;
> -    const char *target;
>      int cmd_argc;
>      int error;
>  
>      set_program_name(argv[0]);
>  
>      /* Parse command line and connect to target. */
> -    target = parse_command_line(argc, argv);
> -    client = connect_to_target(target);
> +    args = parse_command_line(argc, argv);
> +    client = connect_to_target(args->target);
>  
> -    /* Transact request and process reply. */
> +    /* Transact options request (if required) and process reply */

Period at the end of the comment.

> +    if (args->format != OVS_OUTPUT_FMT_TEXT) {
> +        svec_add(&opt_argv, "--format");
> +        svec_add(&opt_argv, ovs_output_fmt_to_string(args->format));
> +    }
> +    svec_terminate(&opt_argv);
> +
> +    if (!svec_is_empty(&opt_argv)) {
> +        error = unixctl_client_transact(client, "set-options",
> +                                        opt_argv.n, opt_argv.names,
> +                                        args->format, &cmd_result,
> +                                        &cmd_error);
> +
> +        if (error) {
> +            ovs_fatal(error, "%s: transaction error", args->target);
> +        }
> +
> +        if (cmd_error) {
> +            jsonrpc_close(client);
> +            fputs(cmd_error, stderr);
> +            ovs_error(0, "%s: server returned an error", args->target);
> +            exit(2);
> +        }
> +
> +        free(cmd_result);
> +        free(cmd_error);
> +    }
> +    svec_destroy(&opt_argv);
> +
> +    /* Transact command request and process reply. */
>      cmd = argv[optind++];
>      cmd_argc = argc - optind;
>      cmd_argv = cmd_argc ? argv + optind : NULL;
>      error = unixctl_client_transact(client, cmd, cmd_argc, cmd_argv,
> -                                    &cmd_result, &cmd_error);
> +                                    args->format, &cmd_result, &cmd_error);
>      if (error) {
> -        ovs_fatal(error, "%s: transaction error", target);
> +        ovs_fatal(error, "%s: transaction error", args->target);
>      }
>  
>      if (cmd_error) {
>          jsonrpc_close(client);
>          fputs(cmd_error, stderr);
> -        ovs_error(0, "%s: server returned an error", target);
> +        ovs_error(0, "%s: server returned an error", args->target);
>          exit(2);
>      } else if (cmd_result) {
>          fputs(cmd_result, stdout);
> @@ -74,6 +114,7 @@ main(int argc, char *argv[])
>          OVS_NOT_REACHED();
>      }
>  
> +    cmdl_args_destroy(args);
>      jsonrpc_close(client);
>      free(cmd_result);
>      free(cmd_error);
> @@ -101,13 +142,31 @@ Common commands:\n\
>    vlog/reopen        Make the program reopen its log file\n\
>  Other options:\n\
>    --timeout=SECS     wait at most SECS seconds for a response\n\
> +  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\
> +                     ('text', by default)\n\

(default: text)

>    -h, --help         Print this helpful information\n\
>    -V, --version      Display ovs-appctl version information\n",
>             program_name, program_name);
>      exit(EXIT_SUCCESS);
>  }
>  
> -static const char *
> +static struct cmdl_args *
> +cmdl_args_create(void) {

'{' on a new line.

> +    struct cmdl_args *args = xmalloc(sizeof *args);
> +
> +    args->format = OVS_OUTPUT_FMT_TEXT;
> +    args->target = NULL;
> +
> +    return args;
> +}
> +
> +static void
> +cmdl_args_destroy(struct cmdl_args *args) {

Ditto.

> +    free(args->target);
> +    free(args);
> +}
> +
> +static struct cmdl_args *
>  parse_command_line(int argc, char *argv[])
>  {
>      enum {
> @@ -117,6 +176,7 @@ parse_command_line(int argc, char *argv[])
>      static const struct option long_options[] = {
>          {"target", required_argument, NULL, 't'},
>          {"execute", no_argument, NULL, 'e'},
> +        {"format", required_argument, NULL, 'f'},
>          {"help", no_argument, NULL, 'h'},
>          {"option", no_argument, NULL, 'o'},
>          {"version", no_argument, NULL, 'V'},
> @@ -126,11 +186,10 @@ parse_command_line(int argc, char *argv[])
>      };
>      char *short_options_ = ovs_cmdl_long_options_to_short_options(long_options);
>      char *short_options = xasprintf("+%s", short_options_);
> -    const char *target;
> -    int e_options;
> +    struct cmdl_args *args = cmdl_args_create();
>      unsigned int timeout = 0;
> +    int e_options;
>  
> -    target = NULL;
>      e_options = 0;
>      for (;;) {
>          int option;
> @@ -141,10 +200,10 @@ parse_command_line(int argc, char *argv[])
>          }
>          switch (option) {
>          case 't':
> -            if (target) {
> +            if (args->target) {
>                  ovs_fatal(0, "-t or --target may be specified only once");
>              }
> -            target = optarg;
> +            args->target = xstrdup(optarg);

Any reason to copy this data?

>              break;
>  
>          case 'e':
> @@ -157,6 +216,12 @@ parse_command_line(int argc, char *argv[])
>              }
>              break;
>  
> +        case 'f':
> +            if (!ovs_output_fmt_from_string(optarg, &args->format)) {
> +                ovs_fatal(0, "value %s on -f or --format is invalid", optarg);
> +            }
> +            break;
> +
>          case 'h':
>              usage();
>              break;
> @@ -194,7 +259,10 @@ parse_command_line(int argc, char *argv[])
>                    "(use --help for help)");
>      }
>  
> -    return target ? target : "ovs-vswitchd";
> +    if (!args->target) {
> +        args->target = xstrdup("ovs-vswitchd");
> +    }
> +    return args;
>  }
>  
>  static struct jsonrpc *