[ovs-dev,v7,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. They gain a global option '-f,--format' which allows users to
request JSON instead of plain-text for humans. An example call
implemented in a later patch is 'ovs-appctl --format json dpif/show'.

For that a new 'set-options' command has been added to lib/unixctl.c
which allows to change the output format of the following commands.
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 then 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.

To demonstrate the necessary API changes for supporting output formats,
unixctl_command_register() in lib/unixctl.* has been cloned to
unixctl_command_register_fmt(). The latter will be replaced with the
former in a later patch. The new function gained an int argument called
'output_fmts' with which commands have to announce their support for
output formats.

Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
'enum ovs_output_fmt fmt'. For now, it has been added as a comment
only for the huge changes reason mentioned earlier. The output format
which has been passed via 'set-options' to ovs-vswitchd will be
converted into a value of type 'enum ovs_output_fmt' in lib/unixctl.c
and then passed to said 'fmt' arg of the choosen command handler (in a
future patch). When a requested output format is not supported by a
command, then process_command() in lib/unixctl.c will return an error.

This patch does not yet pass the choosen output format to commands.
Doing so requires changes to all unixctl_command_register() calls and
all command callbacks. To improve readibility those changes have been
split out into a follow up patch. Respectively, whenever an output
format other than 'text' is choosen for ovs-xxx tools, they will fail.
By default, the output format is plain-text as before.

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 alines 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                   |   2 +
 lib/command-line.c     |  36 +++++++++++
 lib/command-line.h     |  10 +++
 lib/dpctl.h            |   4 ++
 lib/unixctl.c          | 142 ++++++++++++++++++++++++++++++++++-------
 lib/unixctl.h          |  16 ++++-
 utilities/ovs-appctl.c |  97 ++++++++++++++++++++++++----
 utilities/ovs-dpctl.c  |   1 +
 8 files changed, 271 insertions(+), 37 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
#592 FILE: utilities/ovs-appctl.c:144:
  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\

Lines checked: 699, 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 Thu, Jan 18, 2024 at 04:26:52PM +0100, 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.

...

Recheck-request: github-robot

On Thu, Jan 18, 2024 at 04:39:56PM +0000, Simon Horman wrote:
> On Thu, Jan 18, 2024 at 04:26:52PM +0100, 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.
> 
> ...
> 
> Recheck-request: github-robot

Rerun was successful :)

https://github.com/ovsrobot/ovs/actions/runs/7572473760

On 18 Jan 2024, at 16: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. They gain a global option '-f,--format' which allows users to
> request JSON instead of plain-text for humans. An example call
> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>
> For that a new 'set-options' command has been added to lib/unixctl.c
> which allows to change the output format of the following commands.
> 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 then 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.
>
> To demonstrate the necessary API changes for supporting output formats,
> unixctl_command_register() in lib/unixctl.* has been cloned to
> unixctl_command_register_fmt(). The latter will be replaced with the
> former in a later patch. The new function gained an int argument called
> 'output_fmts' with which commands have to announce their support for
> output formats.
>
> Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
> 'enum ovs_output_fmt fmt'. For now, it has been added as a comment
> only for the huge changes reason mentioned earlier. The output format
> which has been passed via 'set-options' to ovs-vswitchd will be
> converted into a value of type 'enum ovs_output_fmt' in lib/unixctl.c
> and then passed to said 'fmt' arg of the choosen command handler (in a
> future patch). When a requested output format is not supported by a
> command, then process_command() in lib/unixctl.c will return an error.
>
> This patch does not yet pass the choosen output format to commands.
> Doing so requires changes to all unixctl_command_register() calls and
> all command callbacks. To improve readibility those changes have been
> split out into a follow up patch. Respectively, whenever an output
> format other than 'text' is choosen for ovs-xxx tools, they will fail.
> By default, the output format is plain-text as before.
>
> 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 alines with ovsdb-client
> where '-f,--format' controls output formatting.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <code@jakobmeng.de>

Hi Jakob,


Thank you for submitting this series; I believe it's a valuable addition to OVS! Apologies for the delayed response. I've reviewed the entire series, and most of the comments are minor change requests. I'll hold off on sending a new revision until Ilya has had a chance to review it, as he provided some comments on the previous revision.

Cheers,

Eelco

> ---
>  NEWS                   |   2 +
>  lib/command-line.c     |  36 +++++++++++
>  lib/command-line.h     |  10 +++
>  lib/dpctl.h            |   4 ++
>  lib/unixctl.c          | 142 ++++++++++++++++++++++++++++++++++-------
>  lib/unixctl.h          |  16 ++++-
>  utilities/ovs-appctl.c |  97 ++++++++++++++++++++++++----
>  utilities/ovs-dpctl.c  |   1 +
>  8 files changed, 271 insertions(+), 37 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 2153b4805..8631ea45e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -33,6 +33,8 @@ v3.3.0 - xx xxx xxxx
>         "ovs-appctl dpctl/ct-del-limits default".
>       * 'dpctl/flush-conntrack' is now capable of flushing connections based
>         on mark and labels.
> +     * Added new option [-f|--format] to choose the output format, e.g. 'json'
> +       or 'text' (by default).
>     - ovs-vsctl:
>       * New commands 'set-zone-limit', 'del-zone-limit' and 'list-zone-limits'
>         to manage the maximum number of connections in conntrack zones via
> 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
> @@ -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>
>
>  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 *);
> +
>  struct ovs_cmdl_parsed_option {
>      const struct option *o;
>      char *arg;
> diff --git a/lib/dpctl.h b/lib/dpctl.h
> index 9d0052152..1c9b2409f 100644
> --- a/lib/dpctl.h
> +++ b/lib/dpctl.h
> @@ -19,6 +19,7 @@
>  #include <stdbool.h>
>
>  #include "compiler.h"
> +#include "command-line.h"
>
>  struct dpctl_params {
>      /* True if it is called by ovs-appctl command. */
> @@ -42,6 +43,9 @@ struct dpctl_params {
>      /* --names: Use port names in output? */
>      bool names;
>
> +    /* Output format. */
> +    enum ovs_output_fmt format;
> +
>      /* Callback for printing.  This function is called from dpctl_run_command()
>       * to output data.  The 'aux' parameter is set to the 'aux'
>       * member.  The 'error' parameter is true if 'string' is an error
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index 103357ee9..dd1450f22 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"
> @@ -39,6 +38,7 @@ COVERAGE_DEFINE(unixctl_replied);
>  struct unixctl_command {
>      const char *usage;
>      int min_args, max_args;
> +    int output_fmts;

Considering that this involves combining bit-mask enums, I suggest converting this to an unsigned int.

>      unixctl_cb_func *cb;
>      void *aux;
>  };
> @@ -50,6 +50,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;
>  };
>
>  /* Server for control connection. */
> @@ -94,6 +96,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;
> +
> +        }
> +    }
> +    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.)
> @@ -109,6 +144,28 @@ 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.)
> + * 'output_fmts' is a bitmap that defines what output formats a command
> + * supports, e.g. OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON.
> + *
> + * '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_fmt(const char *name, const char *usage,
> +                             int min_args, int max_args, int output_fmts,

Unsigned int for output_fmts?

> +                             unixctl_cb_func *cb, void *aux)
>  {
>      struct unixctl_command *command;
>      struct unixctl_command *lookup = shash_find_data(&commands, name);
> @@ -123,41 +180,48 @@ unixctl_command_register(const char *name, const char *usage,
>      command->usage = usage;
>      command->min_args = min_args;
>      command->max_args = max_args;
> +    command->output_fmts = output_fmts;
>      command->cb = cb;
>      command->aux = aux;
>      shash_add(&commands, name, command);
>  }
>
> -static void
> -unixctl_command_reply__(struct unixctl_conn *conn,
> -                        bool success, const char *body)
> +static struct json *
> +json_string_create__(const char *body)
>  {
> -    struct json *body_json;
> -    struct jsonrpc_msg *reply;
> -
> -    COVERAGE_INC(unixctl_replied);
> -    ovs_assert(conn->request_id);
> -
>      if (!body) {
>          body = "";
>      }
>
>      if (body[0] && body[strlen(body) - 1] != '\n') {
> -        body_json = json_string_create_nocopy(xasprintf("%s\n", body));
> +        return json_string_create_nocopy(xasprintf("%s\n", body));
>      } else {
> -        body_json = json_string_create(body);
> +        return json_string_create(body);

We can remove the else clause here, so it will become:

static struct json *
json_string_create__(const char *body)
{
    if (!body) {
        body = "";
    }

    if (body[0] && body[strlen(body) - 1] != '\n') {
        return json_string_create_nocopy(xasprintf("%s\n", body));
    }

    return json_string_create(body);
}

>      }
> +}
> +
> +/* Takes ownership of 'body'. */
> +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);
>          VLOG_DBG("replying with %s, id=%s: \"%s\"",
> -                 success ? "success" : "error", id, body);
> +                 success ? "success" : "error", id, msg);
> +        free(msg);
>          free(id);
>      }
>
> @@ -170,22 +234,34 @@ 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. */
> + * unixctl_command_reply(), unixctl_command_reply_error() or
> + * unixctl_command_reply_json() may be made per request. */
>  void
>  unixctl_command_reply(struct unixctl_conn *conn, const char *result)
>  {
> -    unixctl_command_reply__(conn, true, result);
> +    unixctl_command_reply__(conn, true, json_string_create__(result));
>  }
>
>  /* 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 unixctl_command_reply(), unixctl_command_reply_error() or
> + * unixctl_command_reply_json() may be made per request. */
>  void
>  unixctl_command_reply_error(struct unixctl_conn *conn, const char *error)
>  {
> -    unixctl_command_reply__(conn, false, error);
> +    unixctl_command_reply__(conn, false, json_string_create__(error));
> +}
> +
> +/* 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(), unixctl_command_reply_error() or
> + * unixctl_command_reply_json() may be made per request.
> + *
> + * Takes ownership of 'body'. */
> +void
> +unixctl_command_reply_json(struct unixctl_conn *conn, struct json *body)
> +{
> +    unixctl_command_reply__(conn, true, body);
>  }
>
>  /* Creates a unixctl server listening on 'path', which for POSIX may be:
> @@ -250,6 +326,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,
> +                             unixctl_set_options, NULL);
>
>      struct unixctl_server *server = xmalloc(sizeof *server);
>      server->listener = listener;
> @@ -291,6 +369,18 @@ process_command(struct unixctl_conn *conn, struct jsonrpc_msg *request)
>      } else if (params->n > command->max_args) {
>          error = xasprintf("\"%s\" command takes at most %d arguments",
>                            request->method, command->max_args);
> +    } else if ((!command->output_fmts && conn->fmt != OVS_OUTPUT_FMT_TEXT) ||
> +               (command->output_fmts && !(conn->fmt & command->output_fmts)))
> +    {
> +        error = xasprintf("\"%s\" command does not support output format"\
> +                          " \"%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;
> @@ -307,8 +397,10 @@ 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,
> -                        command->aux);
> +                        /* conn->fmt, */ command->aux);
>          }
>
>          svec_destroy(&argv);
> @@ -381,6 +473,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 {
> @@ -518,6 +611,9 @@ unixctl_client_transact(struct jsonrpc *client, const char *command, int argc,
>      } else if (reply->result) {
>          if (reply->result->type == JSON_STRING) {
>              *result = xstrdup(json_string(reply->result));
> +        } else if (reply->result->type == JSON_OBJECT ||
> +                   reply->result->type == JSON_ARRAY) {
> +            *result = json_to_string(reply->result, 0);
>          } else {
>              VLOG_WARN("%s: unexpected result type in JSON rpc reply: %s",
>                        jsonrpc_get_name(client),
> diff --git a/lib/unixctl.h b/lib/unixctl.h
> index 4562dbc49..8200b9e11 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"
> +
>  #ifdef  __cplusplus
>  extern "C" {
>  #endif
> @@ -40,13 +43,24 @@ 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[], void *aux);
> +                             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. */
>  void unixctl_command_register(const char *name, const char *usage,
>                                int min_args, int max_args,
>                                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 *,
> +                                struct json *body);
>
>  #ifdef  __cplusplus
>  }
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index ba0c172e6..02df8ba97 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -29,12 +29,22 @@
>  #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
> @@ -43,30 +53,59 @@ main(int argc, char *argv[])
>      char *cmd_result, *cmd_error;
>      struct jsonrpc *client;
>      char *cmd, **cmd_argv;
> -    const char *target;
> +    struct cmdl_args *args;
>      int cmd_argc;
>      int error;
> +    struct svec opt_argv = SVEC_EMPTY_INITIALIZER;

Can we keep the variables in reversed Christmas tree order?

>
>      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 options request (if required) and process reply */
> +    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 (opt_argv.n > 0) {

You can use svec_is_empty() here.

> +        error = unixctl_client_transact(client, "set-options",
> +                                        opt_argv.n, opt_argv.names,
> +                                        &cmd_result, &cmd_error);
> +
> +        if (error) {
> +            ovs_fatal(error, "%s: transaction error", args->target);
> +        }
>
> -    /* Transact request and process reply. */
> +        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);

cmd_error will never end up here, as you call exit(2) above, so the free should also move up.

Should we not exit on a transaction error also? I think error handling might need some more cleanup.

> +    }
> +    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);
>      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 +113,7 @@ main(int argc, char *argv[])
>          OVS_NOT_REACHED();
>      }
>
> +    cmdl_args_destroy(args);
>      jsonrpc_close(client);
>      free(cmd_result);
>      free(cmd_error);
> @@ -101,13 +141,34 @@ 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\
>    -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) {
> +    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) {
> +    if (args->target) {

No need to check for NULL here, free() will do that for you.

> +        free(args->target);
> +    }
> +
> +    free(args);
> +}
> +
> +static struct cmdl_args *
>  parse_command_line(int argc, char *argv[])
>  {
>      enum {
> @@ -117,6 +178,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 +188,11 @@ 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;
> +

No need for extra newline.

> +    struct cmdl_args *args = cmdl_args_create();
>      int e_options;
>      unsigned int timeout = 0;

While your add it maybe swap the above two lines.

>
> -    target = NULL;
>      e_options = 0;
>      for (;;) {
>          int option;
> @@ -141,10 +203,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);
>              break;
>
>          case 'e':
> @@ -157,6 +219,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 +262,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 *
> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
> index 56d7a942b..30104ddb2 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -63,6 +63,7 @@ main(int argc, char *argv[])
>      fatal_ignore_sigpipe();
>
>      dpctl_p.is_appctl = false;
> +    dpctl_p.format = OVS_OUTPUT_FMT_TEXT;
>      dpctl_p.output = dpctl_print;
>      dpctl_p.usage = usage;
>
> -- 
> 2.39.2

On 15.03.24 11:15, Eelco Chaudron wrote:
> [...]
> Hi Jakob,
>
>
> Thank you for submitting this series; I believe it's a valuable addition to OVS! Apologies for the delayed response. I've reviewed the entire series, and most of the comments are minor change requests. I'll hold off on sending a new revision until Ilya has had a chance to review it, as he provided some comments on the previous revision.
>
> Cheers,
>
> Eelco

Thanks for your review ☺️ Some questions below..

Best,
Jakob

>> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
>> index ba0c172e6..02df8ba97 100644
>> --- a/utilities/ovs-appctl.c
>> +++ b/utilities/ovs-appctl.c
>> @@ -29,12 +29,22 @@
>>  #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
>> @@ -43,30 +53,59 @@ main(int argc, char *argv[])
>>      char *cmd_result, *cmd_error;
>>      struct jsonrpc *client;
>>      char *cmd, **cmd_argv;
>> -    const char *target;
>> +    struct cmdl_args *args;
>>      int cmd_argc;
>>      int error;
>> +    struct svec opt_argv = SVEC_EMPTY_INITIALIZER;
> Can we keep the variables in reversed Christmas tree order?
>
>>      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 options request (if required) and process reply */
>> +    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 (opt_argv.n > 0) {
> You can use svec_is_empty() here.
>
>> +        error = unixctl_client_transact(client, "set-options",
>> +                                        opt_argv.n, opt_argv.names,
>> +                                        &cmd_result, &cmd_error);
>> +
>> +        if (error) {
>> +            ovs_fatal(error, "%s: transaction error", args->target);
>> +        }
>>
>> -    /* Transact request and process reply. */
>> +        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);
> cmd_error will never end up here, as you call exit(2) above, so the free should also move up.

The error handling of this set-options call is intentionally kept very similar to the error handling of the later command call. Should I drop the "free(cmd_error);" in both sections because we exit anyway? Should I move it up in both cases?

> Should we not exit on a transaction error also? I think error handling might need some more cleanup.

We exit on transaction error with ovs_fatal?! Just like in the next command call..

What kind of cleanup do you have in mind?

>> +    }
>> +    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);
>>      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 +113,7 @@ main(int argc, char *argv[])
>>          OVS_NOT_REACHED();
>>      }
>>
>> +    cmdl_args_destroy(args);
>>      jsonrpc_close(client);
>>      free(cmd_result);
>>      free(cmd_error);
>> @@ -101,13 +141,34 @@ 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\
>>    -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) {
>> +    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) {
>> +    if (args->target) {

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

> On 15.03.24 11:15, Eelco Chaudron wrote:
>> [...]
>> Hi Jakob,
>>
>>
>> Thank you for submitting this series; I believe it's a valuable addition to OVS! Apologies for the delayed response. I've reviewed the entire series, and most of the comments are minor change requests. I'll hold off on sending a new revision until Ilya has had a chance to review it, as he provided some comments on the previous revision.
>>
>> Cheers,
>>
>> Eelco
>
> Thanks for your review ☺️ Some questions below..
>
> Best,
> Jakob
>
>>> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
>>> index ba0c172e6..02df8ba97 100644
>>> --- a/utilities/ovs-appctl.c
>>> +++ b/utilities/ovs-appctl.c
>>> @@ -29,12 +29,22 @@
>>>  #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
>>> @@ -43,30 +53,59 @@ main(int argc, char *argv[])
>>>      char *cmd_result, *cmd_error;
>>>      struct jsonrpc *client;
>>>      char *cmd, **cmd_argv;
>>> -    const char *target;
>>> +    struct cmdl_args *args;
>>>      int cmd_argc;
>>>      int error;
>>> +    struct svec opt_argv = SVEC_EMPTY_INITIALIZER;
>> Can we keep the variables in reversed Christmas tree order?
>>
>>>      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 options request (if required) and process reply */
>>> +    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 (opt_argv.n > 0) {
>> You can use svec_is_empty() here.
>>
>>> +        error = unixctl_client_transact(client, "set-options",
>>> +                                        opt_argv.n, opt_argv.names,
>>> +                                        &cmd_result, &cmd_error);
>>> +
>>> +        if (error) {
>>> +            ovs_fatal(error, "%s: transaction error", args->target);
>>> +        }
>>>
>>> -    /* Transact request and process reply. */
>>> +        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);
>> cmd_error will never end up here, as you call exit(2) above, so the free should also move up.
>
> The error handling of this set-options call is intentionally kept very similar to the error handling of the later command call. Should I drop the "free(cmd_error);" in both sections because we exit anyway? Should I move it up in both cases?
>
>> Should we not exit on a transaction error also? I think error handling might need some more cleanup.
>
> We exit on transaction error with ovs_fatal?! Just like in the next command call..
>
> What kind of cleanup do you have in mind?


I come from the pre-MMU era, where exiting without freeing memory meant losing it forever. With MMUs in place, I suppose it's acceptable to leave this code unchanged. My attempts to tidy it up only made it less readable.

>>> +    }
>>> +    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);
>>>      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 +113,7 @@ main(int argc, char *argv[])
>>>          OVS_NOT_REACHED();
>>>      }
>>>
>>> +    cmdl_args_destroy(args);
>>>      jsonrpc_close(client);
>>>      free(cmd_result);
>>>      free(cmd_error);
>>> @@ -101,13 +141,34 @@ 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\
>>>    -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) {
>>> +    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) {
>>> +    if (args->target) {