[ovs-dev,RFC,v3,1/4] Add global option for JSON output to ovs-appctl/dpctl.

From: Jakob Meng <code@jakobmeng.de>

For monitoring systems such as Prometheus it would be beneficial if
OVS and OVS-DPDK 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 it is necessary to change the JSON-RPC API lib/unixctl.*
which ovs-xxx tools use to communicate with ovs-vswitchd across unix
sockets. It now allows to transport the requested output format
besides the command and its args. This change has been implemented in
a backward compatible way. One can use an updated client
(ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
Of course, JSON output only works when both sides have been updated.

Previously, the command was copied to the 'method' parameter in
JSON-RPC while the args were copied to the 'params' parameter. Without
any other means to transport parameters via JSON-RPC left, the meaning
of 'method' and 'params' had to be changed: 'method' will now be
'execute/v1' when an output format other than 'text' is requested. In
that case, the first parameter of the JSON array 'params' will now be
the designated command, the second one the output format and the rest
will be command args.

unixctl_command_register() in lib/unixctl.* has been cloned as
unixctl_command_register_fmt() in order to demonstrate the new API.
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 is passed via unix socket 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>
---
 lib/command-line.c     |  36 ++++++
 lib/command-line.h     |  10 ++
 lib/dpctl.h            |   4 +
 lib/unixctl.c          | 260 ++++++++++++++++++++++++++++++++---------
 lib/unixctl.h          |  17 ++-
 tests/pmd.at           |   5 +
 utilities/ovs-appctl.c |  65 ++++++++---
 utilities/ovs-dpctl.c  |  12 ++
 8 files changed, 337 insertions(+), 72 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
#696 FILE: utilities/ovs-appctl.c:112:
  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\

Lines checked: 842, 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 25.10.23 11:59, 0-day Robot wrote:
> 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
> #696 FILE: utilities/ovs-appctl.c:112:
>   -f, --format=FMT   Output format. One of: 'json', or 'text'\n\
>
> Lines checked: 842, Warnings: 2, Errors: 0
>
>
> Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Shall we fix utilities/checkpatch.py or simply ignore this message?

Jakob Meng <jmeng@redhat.com> writes:

> On 25.10.23 11:59, 0-day Robot wrote:
>
>>  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
>> #696 FILE: utilities/ovs-appctl.c:112:
>>   -f, --format=FMT   Output format. One of: 'json', or 'text'\n\
>>
>> Lines checked: 842, Warnings: 2, Errors: 0
>>
>>
>> Please check this out.  If you feel there has been an error, please email aconole@redhat.com
>>
> Shall we fix utilities/checkpatch.py or simply ignore this message?

Ideally both - since we should be able to catch that we're in a string
in many instances.  That said, it gets a bit more complicated in
practice because inserting without a leading '"' or trailing '"' means
we don't have the context (unless we do something like apply the patch
and then scan the lines keeping the context).

We usually these kinds of errors since they are obviously tool
limitations.

On 25.10.23 11:37, jmeng@redhat.com wrote:
> From: Jakob Meng <code@jakobmeng.de>
>
> For monitoring systems such as Prometheus it would be beneficial if
> OVS and OVS-DPDK 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 it is necessary to change the JSON-RPC API lib/unixctl.*
> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
> sockets. It now allows to transport the requested output format
> besides the command and its args. This change has been implemented in
> a backward compatible way. One can use an updated client
> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
> Of course, JSON output only works when both sides have been updated.
>
> Previously, the command was copied to the 'method' parameter in
> JSON-RPC while the args were copied to the 'params' parameter. Without
> any other means to transport parameters via JSON-RPC left, the meaning
> of 'method' and 'params' had to be changed: 'method' will now be
> 'execute/v1' when an output format other than 'text' is requested. In
> that case, the first parameter of the JSON array 'params' will now be
> the designated command, the second one the output format and the rest
> will be command args.

Ilya brought up the question why I changed the meaning of 'method' and 'params' instead of adding the output format as an addition argument to the command arguments in 'params'. The server side would then interpret and filter out this argument before passing the remaining arguments to the command callbacks.

I decided against this approach because the code would get more involved, in particular we would have to implement option/argument parsing inside process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a safe way would be difficult in general because their global state.)
The current implementation is based purely on JSON objects/arrays which are nicely supported by OVS with functions from lib/json.*.

Ilya also voiced concerns about the limited extensibility of the proposed API. To fix this, this patch series could be tweaked in the follow way:

(1.) Instead of passing the output format as second entry in the JSON array 'params', turn the second entry into a JSON object (shash). The output format would be one entry in this JSON object, e.g. called 'format'.

The server (process_command() from lib/unixctl.c) will parse the content of this JSON object into known options. Clients would only add non-default options to this JSON object to keep compatibility with older servers. Options which are supported by the server but not transferred via this JSON object would be initialized with default values to keep compatibility with older clients. Unknown entries would cause the server to return an error.

For (2.) see below.

>
> unixctl_command_register() in lib/unixctl.* has been cloned as
> unixctl_command_register_fmt() in order to demonstrate the new API.
> 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 is passed via unix socket 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.

(2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type unixctl_cb_func, an indirection will be used:

The enum value will be put into a struct and the struct will be added to unixctl_cb_func. This would allow us to add new options easily without having to change all command callbacks again.

Wdyt?

> 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>
> ---
>  lib/command-line.c     |  36 ++++++
>  lib/command-line.h     |  10 ++
>  lib/dpctl.h            |   4 +
>  lib/unixctl.c          | 260 ++++++++++++++++++++++++++++++++---------
>  lib/unixctl.h          |  17 ++-
>  tests/pmd.at           |   5 +
>  utilities/ovs-appctl.c |  65 ++++++++---
>  utilities/ovs-dpctl.c  |  12 ++
>  8 files changed, 337 insertions(+), 72 deletions(-)
>
> 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..d174815e7 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;
>  
> +    /* --format: 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..914c3f61f 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;
>      unixctl_cb_func *cb;
>      void *aux;
>  };
> @@ -63,6 +63,8 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>  
>  static struct shash commands = SHASH_INITIALIZER(&commands);
>  
> +static const char *rpc_marker = "execute/v1";
> +
>  static void
>  unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                        const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> @@ -109,6 +111,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,
> +                             unixctl_cb_func *cb, void *aux)
>  {
>      struct unixctl_command *command;
>      struct unixctl_command *lookup = shash_find_data(&commands, name);
> @@ -123,41 +147,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);
>      }
> +}
> +
> +/* 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 +201,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:
> @@ -266,6 +309,11 @@ process_command(struct unixctl_conn *conn, struct jsonrpc_msg *request)
>  
>      struct unixctl_command *command;
>      struct json_array *params;
> +    const char *method;
> +    enum ovs_output_fmt fmt;
> +    struct svec argv = SVEC_EMPTY_INITIALIZER;
> +    int args_offset;
> +    bool plain_rpc;
>  
>      COVERAGE_INC(unixctl_received);
>      conn->request_id = json_clone(request->id);
> @@ -279,45 +327,95 @@ process_command(struct unixctl_conn *conn, struct jsonrpc_msg *request)
>          free(id_s);
>      }
>  
> +    /* The JSON-RPC API requires an indirection in order to allow transporting
> +     * additional data like the output format besides command and args. For
> +     * backward compatibility with older clients the plain RPC is still
> +     * supported. */
> +    plain_rpc = strcmp(request->method, rpc_marker);
> +    args_offset = plain_rpc ? 0 : 2;
> +
>      params = json_array(request->params);
> -    command = shash_find_data(&commands, request->method);
> +    if (!plain_rpc && (params->n < 2)) {
> +        error = xasprintf("JSON-RPC API mismatch: Unexpected # of params:"\
> +                          " %"PRIuSIZE, params->n);
> +        goto error;
> +    }
> +
> +    for (size_t i = 0; i < params->n; i++) {
> +        if (params->elems[i]->type != JSON_STRING) {
> +            error = xasprintf("command has non-string argument: %s",
> +                              json_to_string(params->elems[i], 0));
> +            goto error;
> +        }
> +    }
> +
> +    /* extract method name */
> +    method = plain_rpc ? request->method : json_string(params->elems[0]);
> +
> +    /* extract output format */
> +    if (plain_rpc) {
> +        fmt = OVS_OUTPUT_FMT_TEXT;
> +    } else {
> +        if (!ovs_output_fmt_from_string(json_string(params->elems[1]), &fmt)) {
> +            error = xasprintf("invalid output format: %s",
> +                              json_string(params->elems[1]));
> +            goto error;
> +        }
> +    }
> +
> +    /* find command with method name */
> +    command = shash_find_data(&commands, method);
> +
> +    /* verify that method call complies with command requirements */
>      if (!command) {
>          error = xasprintf("\"%s\" is not a valid command (use "
>                            "\"list-commands\" to see a list of valid commands)",
> -                          request->method);
> -    } else if (params->n < command->min_args) {
> +                          method);
> +        goto error;
> +    } else if ((params->n - args_offset) < command->min_args) {
>          error = xasprintf("\"%s\" command requires at least %d arguments",
> -                          request->method, command->min_args);
> -    } else if (params->n > command->max_args) {
> +                          method, command->min_args);
> +        goto error;
> +    } else if ((params->n - args_offset) > command->max_args) {
>          error = xasprintf("\"%s\" command takes at most %d arguments",
> -                          request->method, command->max_args);
> -    } else {
> -        struct svec argv = SVEC_EMPTY_INITIALIZER;
> -        int  i;
> -
> -        svec_add(&argv, request->method);
> -        for (i = 0; i < params->n; i++) {
> -            if (params->elems[i]->type != JSON_STRING) {
> -                error = xasprintf("\"%s\" command has non-string argument",
> -                                  request->method);
> -                break;
> -            }
> -            svec_add(&argv, json_string(params->elems[i]));
> -        }
> -        svec_terminate(&argv);
> -
> -        if (!error) {
> -            command->cb(conn, argv.n, (const char **) argv.names,
> -                        command->aux);
> -        }
> +                          method, command->max_args);
> +        goto error;
> +    } else if ((!command->output_fmts && fmt != OVS_OUTPUT_FMT_TEXT) ||
> +               (command->output_fmts && !(fmt & command->output_fmts)))
> +    {
> +        error = xasprintf("\"%s\" command does not support output format"\
> +                          " \"%s\" %d %d", method,
> +                          ovs_output_fmt_to_string(fmt), command->output_fmts,
> +                          fmt);
> +        goto error;
> +    }
>  
> -        svec_destroy(&argv);
> +    /* TODO: Remove this check once output format will be passed to the command
> +     *       handler below. */
> +    if (fmt != OVS_OUTPUT_FMT_TEXT) {
> +        error = xasprintf("output format \"%s\" has not been implemented yet",
> +                          ovs_output_fmt_to_string(fmt));
> +        goto error;
>      }
>  
> -    if (error) {
> -        unixctl_command_reply_error(conn, error);
> -        free(error);
> +    /* extract command args */
> +    svec_add(&argv, method);
> +    for (size_t i = args_offset; i < params->n; i++) {
> +        svec_add(&argv, json_string(params->elems[i]));
>      }
> +    svec_terminate(&argv);
> +
> +    /* TODO: Output format will be passed as 'fmt' to the command in later
> +     *       patch. */
> +    command->cb(conn, argv.n, (const char **) argv.names, /* fmt, */
> +                command->aux);
> +
> +    svec_destroy(&argv);
> +
> +    return;
> +error:
> +    unixctl_command_reply_error(conn, error);
> +    free(error);
>  }
>  
>  static int
> @@ -483,21 +581,43 @@ 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;
>      int error, i;
> +    /* The JSON-RPC API requires an indirection in order to allow transporting
> +     * additional data like the output format besides command and args. For
> +     * backward compatibility with older servers the plain RPC is still
> +     * supported. */
> +    bool plain_rpc = (fmt == OVS_OUTPUT_FMT_TEXT);
>  
>      *result = NULL;
>      *err = NULL;
>  
> -    json_args = xmalloc(argc * sizeof *json_args);
> -    for (i = 0; i < argc; i++) {
> -        json_args[i] = json_string_create(argv[i]);
> +    if (plain_rpc) {
> +        json_args = xmalloc(argc * sizeof *json_args);
> +        for (i = 0; i < argc; i++) {
> +            json_args[i] = json_string_create(argv[i]);
> +        }
> +
> +        params = json_array_create(json_args, argc);
> +        request = jsonrpc_create_request(command, params, NULL);
> +    } else {
> +        json_args = xmalloc((argc + 2) * sizeof *json_args);
> +        json_args[0] = json_string_create(command);
> +        json_args[1] = ovs_output_fmt_to_json(fmt);
> +        for (i = 0; i < argc; i++) {
> +            json_args[i + 2] = json_string_create(argv[i]);
> +        }
> +
> +        params = json_array_create(json_args, argc + 2);
> +
> +        /* Use a versioned command to ensure that both server and client
> +         * use the same JSON-RPC API. */
> +        request = jsonrpc_create_request(rpc_marker, params, NULL);
>      }
> -    params = json_array_create(json_args, argc);
> -    request = jsonrpc_create_request(command, params, NULL);
>  
>      error = jsonrpc_transact_block(client, request, &reply);
>      if (error) {
> @@ -508,7 +628,17 @@ unixctl_client_transact(struct jsonrpc *client, const char *command, int argc,
>  
>      if (reply->error) {
>          if (reply->error->type == JSON_STRING) {
> -            *err = xstrdup(json_string(reply->error));
> +            /* catch incompatible server and return helpful error msg */
> +            char *plain_rpc_error = xasprintf("\"%s\" is not a valid command",
> +                                              rpc_marker);
> +            if (!strncmp(plain_rpc_error, json_string(reply->error),
> +                         strlen(plain_rpc_error))) {
> +                *err = xstrdup("JSON RPC reply indicates incompatible server. "
> +                               "Please upgrade server side to newer version.");
> +            } else {
> +                *err = xstrdup(json_string(reply->error));
> +            }
> +            free(plain_rpc_error);
>          } else {
>              VLOG_WARN("%s: unexpected error type in JSON RPC reply: %s",
>                        jsonrpc_get_name(client),
> @@ -518,6 +648,20 @@ 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) {
> +            /* TODO: How about other result types? */
> +
> +            /* TODO: Do we really want to prettyfy and sort the output?
> +             * The benefit for users is probably minimal because they could
> +             * simply use jq to format the output if needed. Since JSON output
> +             * is meant to be consumed by machines, this pretty-printing is
> +             * probably unnecessary in most cases.
> +             * However, it might have its use in our unit tests because it
> +             * allows us to make readable checks without having to introduce a
> +             * dependency on jq.
> +             */
> +            *result = json_to_string(reply->result, JSSF_PRETTY | JSSF_SORT);
>          } 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..45b16157c 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
> @@ -36,17 +39,29 @@ 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. */
>  struct unixctl_conn;
> +/* TODO: 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);
> +/* TODO: unixctl_command_register() will be replaced with
> + *       unixctl_command_register_fmt() in a later patch of this series. It is
> + *       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/tests/pmd.at b/tests/pmd.at
> index 7bdaca9e7..89c392451 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -93,6 +93,11 @@ pmd thread numa_id <cleared> core_id <cleared>:
>    overhead: NOT AVAIL
>  ])
>  
> +AT_CHECK([ovs-appctl --format json dpif-netdev/pmd-rxq-show], [2], [], [dnl
> +"dpif-netdev/pmd-rxq-show" command does not support output format "json" 1 2
> +ovs-appctl: ovs-vswitchd: server returned an error
> +])
> +
>  AT_CHECK([ovs-appctl dpif/show | sed 's/\(tx_queues=\)[[0-9]]*/\1<cleared>/g'], [0], [dnl
>  dummy@ovs-dummy: hit:0 missed:0
>    br0:
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index ba0c172e6..3a70408e5 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -34,7 +34,14 @@
>  #include "openvswitch/vlog.h"
>  
>  static void usage(void);
> -static const char *parse_command_line(int argc, char *argv[]);
> +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 +50,30 @@ 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;
>  
>      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. */
>      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 +81,7 @@ main(int argc, char *argv[])
>          OVS_NOT_REACHED();
>      }
>  
> +    cmdl_args_destroy(args);
>      jsonrpc_close(client);
>      free(cmd_result);
>      free(cmd_error);
> @@ -101,13 +109,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) {
> +        free(args->target);
> +    }
> +
> +    free(args);
> +}
> +
> +static struct cmdl_args *
>  parse_command_line(int argc, char *argv[])
>  {
>      enum {
> @@ -117,6 +146,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 +156,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;
> +
> +    struct cmdl_args *args = cmdl_args_create();
>      int e_options;
>      unsigned int timeout = 0;
>  
> -    target = NULL;
>      e_options = 0;
>      for (;;) {
>          int option;
> @@ -141,10 +171,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 +187,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 +230,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..19bc80417 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -91,6 +91,7 @@ parse_options(int argc, char *argv[])
>          {"names", no_argument, NULL, OPT_NAMES},
>          {"no-names", no_argument, NULL, OPT_NO_NAMES},
>          {"timeout", required_argument, NULL, 't'},
> +        {"format", required_argument, NULL, 'f'},
>          {"help", no_argument, NULL, 'h'},
>          {"option", no_argument, NULL, 'o'},
>          {"version", no_argument, NULL, 'V'},
> @@ -101,6 +102,7 @@ parse_options(int argc, char *argv[])
>  
>      bool set_names = false;
>      unsigned int timeout = 0;
> +    enum ovs_output_fmt fmt = OVS_OUTPUT_FMT_TEXT;
>  
>      for (;;) {
>          int c;
> @@ -141,6 +143,12 @@ parse_options(int argc, char *argv[])
>              set_names = true;
>              break;
>  
> +        case 'f':
> +            if (!ovs_output_fmt_from_string(optarg, &fmt)) {
> +                ovs_fatal(0, "value %s on -f or --format is invalid", optarg);
> +            }
> +            break;
> +
>          case 't':
>              if (!str_to_uint(optarg, 10, &timeout) || !timeout) {
>                  ovs_fatal(0, "value %s on -t or --timeout is invalid", optarg);
> @@ -174,6 +182,8 @@ parse_options(int argc, char *argv[])
>      if (!set_names) {
>          dpctl_p.names = dpctl_p.verbosity > 0;
>      }
> +
> +    dpctl_p.format = fmt;
>  }
>  
>  static void
> @@ -228,6 +238,8 @@ usage(void *userdata OVS_UNUSED)
>             "  --clear                     reset existing stats to zero\n"
>             "\nOther options:\n"
>             "  -t, --timeout=SECS          give up after SECS seconds\n"
> +           "  -f, --format=FMT            Output format. One of: 'json', "
> +           "'text'\n                             ('text', by default)\n"
>             "  -h, --help                  display this help message\n"
>             "  -V, --version               display version information\n");
>      exit(EXIT_SUCCESS);

On 10/26/23 13:44, Jakob Meng wrote:
> On 25.10.23 11:37, jmeng@redhat.com wrote:
>> From: Jakob Meng <code@jakobmeng.de>
>>
>> For monitoring systems such as Prometheus it would be beneficial if
>> OVS and OVS-DPDK 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 it is necessary to change the JSON-RPC API lib/unixctl.*
>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>> sockets. It now allows to transport the requested output format
>> besides the command and its args. This change has been implemented in
>> a backward compatible way. One can use an updated client
>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>> Of course, JSON output only works when both sides have been updated.
>>
>> Previously, the command was copied to the 'method' parameter in
>> JSON-RPC while the args were copied to the 'params' parameter. Without
>> any other means to transport parameters via JSON-RPC left, the meaning
>> of 'method' and 'params' had to be changed: 'method' will now be
>> 'execute/v1' when an output format other than 'text' is requested. In
>> that case, the first parameter of the JSON array 'params' will now be
>> the designated command, the second one the output format and the rest
>> will be command args.
> 
> Ilya brought up the question why I changed the meaning of 'method' and 'params' instead of adding the output format as an addition argument to the command arguments in 'params'. The server side would then interpret and filter out this argument before passing the remaining arguments to the command callbacks.
> 
> I decided against this approach because the code would get more involved, in particular we would have to implement option/argument parsing inside process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a safe way would be difficult in general because their global state.)
> The current implementation is based purely on JSON objects/arrays which are nicely supported by OVS with functions from lib/json.*.

I'm not sure I got this point, but see below.

> 
> Ilya also voiced concerns about the limited extensibility of the proposed API. To fix this, this patch series could be tweaked in the follow way:
> 
> (1.) Instead of passing the output format as second entry in the JSON array 'params', turn the second entry into a JSON object (shash). The output format would be one entry in this JSON object, e.g. called 'format'.
> 
> The server (process_command() from lib/unixctl.c) will parse the content of this JSON object into known options. Clients would only add non-default options to this JSON object to keep compatibility with older servers. Options which are supported by the server but not transferred via this JSON object would be initialized with default values to keep compatibility with older clients. Unknown entries would cause the server to return an error.

This kind of implements what I had in mind, but in a slightly different
manner.  How about something like this:

 * ovs-appctl has a global parameter --format passed to it, not part of
   the requested command paramaters.  (I think this is implemented in the
   code, below, but I didn't read very carefully.)

 * ovs-appctl adds this paramater as an argument to a 'params' JSON array.
   i.e. separate element in the array to every command it executes, if
   provided.

 * Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
   that argument, removes it from the list of parameters before passing
   to the actual handler.

 * Before calling a handler, server stores these special parameters
   into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
   really matter).

 * Handler always has the conn pointer.  Handler can ask in which format
   the user prefers the output and generate one.  E.g.:

     if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
         unixctl_command_reply_json(conn, ...);
     } else {
         unixctl_command_reply(conn, ...);
     }

   Notice that nothing bad happens if command is not aware of the formats,
   because unixctl_command_reply() is a JSON reply, but with a simple
   JSON string instead of any fancy structure.

 * Result is sent back to the ovs-appctl.

 * If the --format=json was originally requested, just dump the result
   as JSON to the output in the unixctl_client_transact().  If format
   wasn't requested, parse it and print out the same way as it is done
   right now, if the result is not a simple JSON string - error.

This approach requires no changes in the code that doesn't want to provide
JSON output and no changes to the command registering API.

All commands support JSON output automatically.  But commands that do
not handle it specially will just return a simple JSON string.  We will
print it out as a JSON string, there is nothing bad in this.

Prettyfication can be done on the ovs-appctl side by an extra argument
that doesn't need to be transferred to the server (already done this way).

The only issue is that it's hard to tell what happened when a new
ovs-appctl --format=json communicates with the old server and gets a
number of arguments error.  But we could add a hint 'Can be caused by an
older <target> not supporting JSON format' to every such reply if format
was requested and let user try without format and get a proper error if
there is one.  In any case, that should not be a frequent case, as I
would expect the most users of JSON format will be scripts with their
own JSON-RPC implementation, not ovs-appctl.

Thoughts?

Some more comments below.

> 
> For (2.) see below.
> 
>>
>> unixctl_command_register() in lib/unixctl.* has been cloned as
>> unixctl_command_register_fmt() in order to demonstrate the new API.
>> 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 is passed via unix socket 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.
> 
> (2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type unixctl_cb_func, an indirection will be used:
> 
> The enum value will be put into a struct and the struct will be added to unixctl_cb_func. This would allow us to add new options easily without having to change all command callbacks again.

We already have strunc unixctl_conn where we can store generic things
and have functions-accessors.

> 
> Wdyt?
> 
>> 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>
>> ---
>>  lib/command-line.c     |  36 ++++++
>>  lib/command-line.h     |  10 ++
>>  lib/dpctl.h            |   4 +
>>  lib/unixctl.c          | 260 ++++++++++++++++++++++++++++++++---------
>>  lib/unixctl.h          |  17 ++-
>>  tests/pmd.at           |   5 +
>>  utilities/ovs-appctl.c |  65 ++++++++---
>>  utilities/ovs-dpctl.c  |  12 ++

Don't touch interface of the ovs-dpctl binary.  It doesn't need to have
JSON output.  ovs-appctl dpctl/ should pretty much always be used instead.

Best regards, Ilya Maximets.

On 10/27/23 23:51, Ilya Maximets wrote:
> On 10/26/23 13:44, Jakob Meng wrote:
>> On 25.10.23 11:37, jmeng@redhat.com wrote:
>>> From: Jakob Meng <code@jakobmeng.de>
>>>
>>> For monitoring systems such as Prometheus it would be beneficial if
>>> OVS and OVS-DPDK would expose statistics in a machine-readable format.

BTW, there is no such separate thing as OVS-DPDK, it's just OVS.

>>>
>>> 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 it is necessary to change the JSON-RPC API lib/unixctl.*
>>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>>> sockets. It now allows to transport the requested output format
>>> besides the command and its args. This change has been implemented in
>>> a backward compatible way. One can use an updated client
>>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>>> Of course, JSON output only works when both sides have been updated.
>>>
>>> Previously, the command was copied to the 'method' parameter in
>>> JSON-RPC while the args were copied to the 'params' parameter. Without
>>> any other means to transport parameters via JSON-RPC left, the meaning
>>> of 'method' and 'params' had to be changed: 'method' will now be
>>> 'execute/v1' when an output format other than 'text' is requested. In
>>> that case, the first parameter of the JSON array 'params' will now be
>>> the designated command, the second one the output format and the rest
>>> will be command args.
>>
>> Ilya brought up the question why I changed the meaning of 'method' and 'params' instead of adding the output format as an addition argument to the command arguments in 'params'. The server side would then interpret and filter out this argument before passing the remaining arguments to the command callbacks.
>>
>> I decided against this approach because the code would get more involved, in particular we would have to implement option/argument parsing inside process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a safe way would be difficult in general because their global state.)
>> The current implementation is based purely on JSON objects/arrays which are nicely supported by OVS with functions from lib/json.*.
> 
> I'm not sure I got this point, but see below.
> 
>>
>> Ilya also voiced concerns about the limited extensibility of the proposed API. To fix this, this patch series could be tweaked in the follow way:
>>
>> (1.) Instead of passing the output format as second entry in the JSON array 'params', turn the second entry into a JSON object (shash).

It has to be an array, not an object.  Parameters are positional.
Unless you want to change API for every existing command and give
each argument a unique name.

And that will not help with supporting older servers, because they
expect an array.

>> The output format would be one entry in this JSON object, e.g. called 'format'.
>>
>> The server (process_command() from lib/unixctl.c) will parse the content of this JSON object into known options. Clients would only add non-default options to this JSON object to keep compatibility with older servers. Options which are supported by the server but not transferred via this JSON object would be initialized with default values to keep compatibility with older clients. Unknown entries would cause the server to return an error.
> 
> This kind of implements what I had in mind, but in a slightly different
> manner.  How about something like this:
> 
>  * ovs-appctl has a global parameter --format passed to it, not part of
>    the requested command paramaters.  (I think this is implemented in the
>    code, below, but I didn't read very carefully.)
> 
>  * ovs-appctl adds this paramater as an argument to a 'params' JSON array.
>    i.e. separate element in the array to every command it executes, if
>    provided.
> 
>  * Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
>    that argument, removes it from the list of parameters before passing
>    to the actual handler.
> 
>  * Before calling a handler, server stores these special parameters
>    into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
>    really matter).
> 
>  * Handler always has the conn pointer.  Handler can ask in which format
>    the user prefers the output and generate one.  E.g.:
> 
>      if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
>          unixctl_command_reply_json(conn, ...);
>      } else {
>          unixctl_command_reply(conn, ...);
>      }
> 
>    Notice that nothing bad happens if command is not aware of the formats,
>    because unixctl_command_reply() is a JSON reply, but with a simple
>    JSON string instead of any fancy structure.
> 
>  * Result is sent back to the ovs-appctl.
> 
>  * If the --format=json was originally requested, just dump the result
>    as JSON to the output in the unixctl_client_transact().  If format
>    wasn't requested, parse it and print out the same way as it is done
>    right now, if the result is not a simple JSON string - error.
> 
> This approach requires no changes in the code that doesn't want to provide
> JSON output and no changes to the command registering API.
> 
> All commands support JSON output automatically.  But commands that do
> not handle it specially will just return a simple JSON string.  We will
> print it out as a JSON string, there is nothing bad in this.
> 
> Prettyfication can be done on the ovs-appctl side by an extra argument
> that doesn't need to be transferred to the server (already done this way).
> 
> The only issue is that it's hard to tell what happened when a new
> ovs-appctl --format=json communicates with the old server and gets a
> number of arguments error.  But we could add a hint 'Can be caused by an
> older <target> not supporting JSON format' to every such reply if format
> was requested and let user try without format and get a proper error if
> there is one.  In any case, that should not be a frequent case, as I
> would expect the most users of JSON format will be scripts with their
> own JSON-RPC implementation, not ovs-appctl.
> 
> Thoughts?
> 
> Some more comments below.
> 
>>
>> For (2.) see below.
>>
>>>
>>> unixctl_command_register() in lib/unixctl.* has been cloned as
>>> unixctl_command_register_fmt() in order to demonstrate the new API.
>>> 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 is passed via unix socket 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.
>>
>> (2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type unixctl_cb_func, an indirection will be used:
>>
>> The enum value will be put into a struct and the struct will be added to unixctl_cb_func. This would allow us to add new options easily without having to change all command callbacks again.
> 
> We already have strunc unixctl_conn where we can store generic things
> and have functions-accessors.
> 
>>
>> Wdyt?
>>
>>> 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>
>>> ---
>>>  lib/command-line.c     |  36 ++++++
>>>  lib/command-line.h     |  10 ++
>>>  lib/dpctl.h            |   4 +
>>>  lib/unixctl.c          | 260 ++++++++++++++++++++++++++++++++---------
>>>  lib/unixctl.h          |  17 ++-
>>>  tests/pmd.at           |   5 +
>>>  utilities/ovs-appctl.c |  65 ++++++++---
>>>  utilities/ovs-dpctl.c  |  12 ++
> 
> Don't touch interface of the ovs-dpctl binary.  It doesn't need to have
> JSON output.  ovs-appctl dpctl/ should pretty much always be used instead.
> 
> Best regards, Ilya Maximets.

On 27 Oct 2023, at 23:51, Ilya Maximets wrote:

> On 10/26/23 13:44, Jakob Meng wrote:
>> On 25.10.23 11:37, jmeng@redhat.com wrote:
>>> From: Jakob Meng <code@jakobmeng.de>
>>>
>>> For monitoring systems such as Prometheus it would be beneficial if
>>> OVS and OVS-DPDK 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'.

I started to look at some of the code, but I guess some of the comments from Ilya could change the implementation, so I’ll stop for now.

Please take a look at my commands from the v1 review, and incorporate them if still valid after potential changes (and flake8 warnings).

Also, instead of TODO use use 'XXX' or 'FIXME' as mentioned in the coding style.

On the ‘pretty print or not discussion’, I added the following:

I think we should use the same option as ovs-vsctl has for the dbase.

  -f, --format=FORMAT         set output formatting to FORMAT
                              ("table", "html", "csv", or "json")
  --pretty                    pretty-print JSON in output

This looks like the following:

   ovs-vsctl --format=json list Open_vSwitch
{"data":[[["uuid","8acf0aa8-7b3c-4b2a-9a15-6a4143ef63f2"],["uuid","630c4d69-c905-4645-ba59-66fd0bfa66c7"],32,["set",["netdev","system"]],["map",[]],"8.3.1",true,"DPDK 22.11.1",["map",[["hostname","wsfd-advnetlab224.anl.lab.eng.bos.redhat.com"],["rundir","/var/run/openvswitch"],["system-id","5ee96c43-4823-4456-9bc3-9616c1acdce3"]]],["set",["bareudp","dpdk","dpdkvhostuser","dpdkvhostuserclient","erspan","geneve","gre","gtpu","internal","ip6erspan","ip6gre","lisp","patch","stt","system","tap","vxlan"]],["set",[]],32,["map",[["dpdk-init","true"],["dpdk-socket-mem","2048"],["pmd-cpu-mask","0x4000000040000000"]]],"3.1.4",["set",[]],["map",[]],"rhel","9.2"]],"headings":["_uuid","bridges","cur_cfg","datapath_types","datapaths","db_version","dpdk_initialized","dpdk_version","external_ids","iface_types","manager_options","next_cfg","other_config","ovs_version","ssl","statistics","system_type","system_version"]}
[wsfd-advnetlab224:~]$ ovs-vsctl --format=json list Open_vSwitch

[wsfd-advnetlab224:~]$ ovs-vsctl --format=json --pretty list Open_vSwitch
{
  "data": [
  ...
}

>>> For that it is necessary to change the JSON-RPC API lib/unixctl.*
>>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>>> sockets. It now allows to transport the requested output format
>>> besides the command and its args. This change has been implemented in
>>> a backward compatible way. One can use an updated client
>>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>>> Of course, JSON output only works when both sides have been updated.
>>>
>>> Previously, the command was copied to the 'method' parameter in
>>> JSON-RPC while the args were copied to the 'params' parameter. Without
>>> any other means to transport parameters via JSON-RPC left, the meaning
>>> of 'method' and 'params' had to be changed: 'method' will now be
>>> 'execute/v1' when an output format other than 'text' is requested. In
>>> that case, the first parameter of the JSON array 'params' will now be
>>> the designated command, the second one the output format and the rest
>>> will be command args.
>>
>> Ilya brought up the question why I changed the meaning of 'method' and 'params' instead of adding the output format as an addition argument to the command arguments in 'params'. The server side would then interpret and filter out this argument before passing the remaining arguments to the command callbacks.
>>
>> I decided against this approach because the code would get more involved, in particular we would have to implement option/argument parsing inside process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a safe way would be difficult in general because their global state.)
>> The current implementation is based purely on JSON objects/arrays which are nicely supported by OVS with functions from lib/json.*.
>
> I'm not sure I got this point, but see below.
>
>>
>> Ilya also voiced concerns about the limited extensibility of the proposed API. To fix this, this patch series could be tweaked in the follow way:
>>
>> (1.) Instead of passing the output format as second entry in the JSON array 'params', turn the second entry into a JSON object (shash). The output format would be one entry in this JSON object, e.g. called 'format'.
>>
>> The server (process_command() from lib/unixctl.c) will parse the content of this JSON object into known options. Clients would only add non-default options to this JSON object to keep compatibility with older servers. Options which are supported by the server but not transferred via this JSON object would be initialized with default values to keep compatibility with older clients. Unknown entries would cause the server to return an error.
>
> This kind of implements what I had in mind, but in a slightly different
> manner.  How about something like this:
>
>  * ovs-appctl has a global parameter --format passed to it, not part of
>    the requested command paramaters.  (I think this is implemented in the
>    code, below, but I didn't read very carefully.)
>
>  * ovs-appctl adds this paramater as an argument to a 'params' JSON array.
>    i.e. separate element in the array to every command it executes, if
>    provided.
>
>  * Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
>    that argument, removes it from the list of parameters before passing
>    to the actual handler.
>
>  * Before calling a handler, server stores these special parameters
>    into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
>    really matter).
>
>  * Handler always has the conn pointer.  Handler can ask in which format
>    the user prefers the output and generate one.  E.g.:
>
>      if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
>          unixctl_command_reply_json(conn, ...);
>      } else {
>          unixctl_command_reply(conn, ...);
>      }
>
>    Notice that nothing bad happens if command is not aware of the formats,
>    because unixctl_command_reply() is a JSON reply, but with a simple
>    JSON string instead of any fancy structure.
>
>  * Result is sent back to the ovs-appctl.
>
>  * If the --format=json was originally requested, just dump the result
>    as JSON to the output in the unixctl_client_transact().  If format
>    wasn't requested, parse it and print out the same way as it is done
>    right now, if the result is not a simple JSON string - error.
>
> This approach requires no changes in the code that doesn't want to provide
> JSON output and no changes to the command registering API.
>
> All commands support JSON output automatically.  But commands that do
> not handle it specially will just return a simple JSON string.  We will
> print it out as a JSON string, there is nothing bad in this.

I think we should report an error in this case, as it will make it clear that the requested JSON format is not supported. This way it might be easy to differentiate between an implementation with or without explicit JSON output support.

> Prettyfication can be done on the ovs-appctl side by an extra argument
> that doesn't need to be transferred to the server (already done this way).
>
> The only issue is that it's hard to tell what happened when a new
> ovs-appctl --format=json communicates with the old server and gets a
> number of arguments error.  But we could add a hint 'Can be caused by an
> older <target> not supporting JSON format' to every such reply if format
> was requested and let user try without format and get a proper error if
> there is one.  In any case, that should not be a frequent case, as I
> would expect the most users of JSON format will be scripts with their
> own JSON-RPC implementation, not ovs-appctl.
>
> Thoughts?
>
> Some more comments below.
>
>>
>> For (2.) see below.
>>
>>>
>>> unixctl_command_register() in lib/unixctl.* has been cloned as
>>> unixctl_command_register_fmt() in order to demonstrate the new API.
>>> 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 is passed via unix socket 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.
>>
>> (2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type unixctl_cb_func, an indirection will be used:
>>
>> The enum value will be put into a struct and the struct will be added to unixctl_cb_func. This would allow us to add new options easily without having to change all command callbacks again.
>
> We already have strunc unixctl_conn where we can store generic things
> and have functions-accessors.
>
>>
>> Wdyt?
>>
>>> 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>
>>> ---
>>>  lib/command-line.c     |  36 ++++++
>>>  lib/command-line.h     |  10 ++
>>>  lib/dpctl.h            |   4 +
>>>  lib/unixctl.c          | 260 ++++++++++++++++++++++++++++++++---------
>>>  lib/unixctl.h          |  17 ++-
>>>  tests/pmd.at           |   5 +
>>>  utilities/ovs-appctl.c |  65 ++++++++---
>>>  utilities/ovs-dpctl.c  |  12 ++
>
> Don't touch interface of the ovs-dpctl binary.  It doesn't need to have
> JSON output.  ovs-appctl dpctl/ should pretty much always be used instead.
>
> Best regards, Ilya Maximets.

On 10/30/23 10:52, Eelco Chaudron wrote:
> 
> 
> On 27 Oct 2023, at 23:51, Ilya Maximets wrote:
> 
>> On 10/26/23 13:44, Jakob Meng wrote:
>>> On 25.10.23 11:37, jmeng@redhat.com wrote:
>>>> From: Jakob Meng <code@jakobmeng.de>
>>>>
>>>> For monitoring systems such as Prometheus it would be beneficial if
>>>> OVS and OVS-DPDK 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'.
> 
> I started to look at some of the code, but I guess some of the comments from Ilya could change the implementation, so I’ll stop for now.
> 
> Please take a look at my commands from the v1 review, and incorporate them if still valid after potential changes (and flake8 warnings).
> 
> Also, instead of TODO use use 'XXX' or 'FIXME' as mentioned in the coding style.
> 
> On the ‘pretty print or not discussion’, I added the following:
> 
> I think we should use the same option as ovs-vsctl has for the dbase.
> 
>   -f, --format=FORMAT         set output formatting to FORMAT
>                               ("table", "html", "csv", or "json")
>   --pretty                    pretty-print JSON in output
> 
> This looks like the following:
> 
>    ovs-vsctl --format=json list Open_vSwitch
> {"data":[[["uuid","8acf0aa8-7b3c-4b2a-9a15-6a4143ef63f2"],["uuid","630c4d69-c905-4645-ba59-66fd0bfa66c7"],32,["set",["netdev","system"]],["map",[]],"8.3.1",true,"DPDK 22.11.1",["map",[["hostname","wsfd-advnetlab224.anl.lab.eng.bos.redhat.com"],["rundir","/var/run/openvswitch"],["system-id","5ee96c43-4823-4456-9bc3-9616c1acdce3"]]],["set",["bareudp","dpdk","dpdkvhostuser","dpdkvhostuserclient","erspan","geneve","gre","gtpu","internal","ip6erspan","ip6gre","lisp","patch","stt","system","tap","vxlan"]],["set",[]],32,["map",[["dpdk-init","true"],["dpdk-socket-mem","2048"],["pmd-cpu-mask","0x4000000040000000"]]],"3.1.4",["set",[]],["map",[]],"rhel","9.2"]],"headings":["_uuid","bridges","cur_cfg","datapath_types","datapaths","db_version","dpdk_initialized","dpdk_version","external_ids","iface_types","manager_options","next_cfg","other_config","ovs_version","ssl","statistics","system_type","system_version"]}
> [wsfd-advnetlab224:~]$ ovs-vsctl --format=json list Open_vSwitch
> 
> [wsfd-advnetlab224:~]$ ovs-vsctl --format=json --pretty list Open_vSwitch
> {
>   "data": [
>   ...
> }
> 
>>>> For that it is necessary to change the JSON-RPC API lib/unixctl.*
>>>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>>>> sockets. It now allows to transport the requested output format
>>>> besides the command and its args. This change has been implemented in
>>>> a backward compatible way. One can use an updated client
>>>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>>>> Of course, JSON output only works when both sides have been updated.
>>>>
>>>> Previously, the command was copied to the 'method' parameter in
>>>> JSON-RPC while the args were copied to the 'params' parameter. Without
>>>> any other means to transport parameters via JSON-RPC left, the meaning
>>>> of 'method' and 'params' had to be changed: 'method' will now be
>>>> 'execute/v1' when an output format other than 'text' is requested. In
>>>> that case, the first parameter of the JSON array 'params' will now be
>>>> the designated command, the second one the output format and the rest
>>>> will be command args.
>>>
>>> Ilya brought up the question why I changed the meaning of 'method' and 'params' instead of adding the output format as an addition argument to the command arguments in 'params'. The server side would then interpret and filter out this argument before passing the remaining arguments to the command callbacks.
>>>
>>> I decided against this approach because the code would get more involved, in particular we would have to implement option/argument parsing inside process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a safe way would be difficult in general because their global state.)
>>> The current implementation is based purely on JSON objects/arrays which are nicely supported by OVS with functions from lib/json.*.
>>
>> I'm not sure I got this point, but see below.
>>
>>>
>>> Ilya also voiced concerns about the limited extensibility of the proposed API. To fix this, this patch series could be tweaked in the follow way:
>>>
>>> (1.) Instead of passing the output format as second entry in the JSON array 'params', turn the second entry into a JSON object (shash). The output format would be one entry in this JSON object, e.g. called 'format'.
>>>
>>> The server (process_command() from lib/unixctl.c) will parse the content of this JSON object into known options. Clients would only add non-default options to this JSON object to keep compatibility with older servers. Options which are supported by the server but not transferred via this JSON object would be initialized with default values to keep compatibility with older clients. Unknown entries would cause the server to return an error.
>>
>> This kind of implements what I had in mind, but in a slightly different
>> manner.  How about something like this:
>>
>>  * ovs-appctl has a global parameter --format passed to it, not part of
>>    the requested command paramaters.  (I think this is implemented in the
>>    code, below, but I didn't read very carefully.)
>>
>>  * ovs-appctl adds this paramater as an argument to a 'params' JSON array.
>>    i.e. separate element in the array to every command it executes, if
>>    provided.
>>
>>  * Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
>>    that argument, removes it from the list of parameters before passing
>>    to the actual handler.
>>
>>  * Before calling a handler, server stores these special parameters
>>    into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
>>    really matter).
>>
>>  * Handler always has the conn pointer.  Handler can ask in which format
>>    the user prefers the output and generate one.  E.g.:
>>
>>      if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
>>          unixctl_command_reply_json(conn, ...);
>>      } else {
>>          unixctl_command_reply(conn, ...);
>>      }
>>
>>    Notice that nothing bad happens if command is not aware of the formats,
>>    because unixctl_command_reply() is a JSON reply, but with a simple
>>    JSON string instead of any fancy structure.
>>
>>  * Result is sent back to the ovs-appctl.
>>
>>  * If the --format=json was originally requested, just dump the result
>>    as JSON to the output in the unixctl_client_transact().  If format
>>    wasn't requested, parse it and print out the same way as it is done
>>    right now, if the result is not a simple JSON string - error.
>>
>> This approach requires no changes in the code that doesn't want to provide
>> JSON output and no changes to the command registering API.
>>
>> All commands support JSON output automatically.  But commands that do
>> not handle it specially will just return a simple JSON string.  We will
>> print it out as a JSON string, there is nothing bad in this.
> 
> I think we should report an error in this case, as it will make it clear that the requested JSON format is not supported. This way it might be easy to differentiate between an implementation with or without explicit JSON output support.

I'm not sure it's actually possible to detect.  There is no difference
between JSON string as an actual JSON result and a JSON string created
from a simple output.  I guess, the only way could be is to require that
the argument of unixctl_command_reply_json() has to be a JSON_OBJECT.

> 
>> Prettyfication can be done on the ovs-appctl side by an extra argument
>> that doesn't need to be transferred to the server (already done this way).
>>
>> The only issue is that it's hard to tell what happened when a new
>> ovs-appctl --format=json communicates with the old server and gets a
>> number of arguments error.  But we could add a hint 'Can be caused by an
>> older <target> not supporting JSON format' to every such reply if format
>> was requested and let user try without format and get a proper error if
>> there is one.  In any case, that should not be a frequent case, as I
>> would expect the most users of JSON format will be scripts with their
>> own JSON-RPC implementation, not ovs-appctl.
>>
>> Thoughts?
>>
>> Some more comments below.
>>
>>>
>>> For (2.) see below.
>>>
>>>>
>>>> unixctl_command_register() in lib/unixctl.* has been cloned as
>>>> unixctl_command_register_fmt() in order to demonstrate the new API.
>>>> 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 is passed via unix socket 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.
>>>
>>> (2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type unixctl_cb_func, an indirection will be used:
>>>
>>> The enum value will be put into a struct and the struct will be added to unixctl_cb_func. This would allow us to add new options easily without having to change all command callbacks again.
>>
>> We already have strunc unixctl_conn where we can store generic things
>> and have functions-accessors.
>>
>>>
>>> Wdyt?
>>>
>>>> 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>
>>>> ---
>>>>  lib/command-line.c     |  36 ++++++
>>>>  lib/command-line.h     |  10 ++
>>>>  lib/dpctl.h            |   4 +
>>>>  lib/unixctl.c          | 260 ++++++++++++++++++++++++++++++++---------
>>>>  lib/unixctl.h          |  17 ++-
>>>>  tests/pmd.at           |   5 +
>>>>  utilities/ovs-appctl.c |  65 ++++++++---
>>>>  utilities/ovs-dpctl.c  |  12 ++
>>
>> Don't touch interface of the ovs-dpctl binary.  It doesn't need to have
>> JSON output.  ovs-appctl dpctl/ should pretty much always be used instead.
>>
>> Best regards, Ilya Maximets.
>

Hi Ilya,
thanks for sharing your thoughts, always appreciated! ☺️ Please find comments below.

On 28.10.23 00:05, Ilya Maximets wrote:
> On 10/27/23 23:51, Ilya Maximets wrote:
>> On 10/26/23 13:44, Jakob Meng wrote:
>>> On 25.10.23 11:37, jmeng@redhat.com wrote:
>>>> From: Jakob Meng <code@jakobmeng.de>
>>>>
>>>> For monitoring systems such as Prometheus it would be beneficial if
>>>> OVS and OVS-DPDK would expose statistics in a machine-readable format.
> BTW, there is no such separate thing as OVS-DPDK, it's just OVS.

With OVS-DPDK I wanted to highlight one of the potential use cases of this change. It came up in discussions and the OVS codebase. Will remove it, if it is frowned upon, no worry.

>
>>>> 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 it is necessary to change the JSON-RPC API lib/unixctl.*
>>>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>>>> sockets. It now allows to transport the requested output format
>>>> besides the command and its args. This change has been implemented in
>>>> a backward compatible way. One can use an updated client
>>>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>>>> Of course, JSON output only works when both sides have been updated.
>>>>
>>>> Previously, the command was copied to the 'method' parameter in
>>>> JSON-RPC while the args were copied to the 'params' parameter. Without
>>>> any other means to transport parameters via JSON-RPC left, the meaning
>>>> of 'method' and 'params' had to be changed: 'method' will now be
>>>> 'execute/v1' when an output format other than 'text' is requested. In
>>>> that case, the first parameter of the JSON array 'params' will now be
>>>> the designated command, the second one the output format and the rest
>>>> will be command args.
>>> Ilya brought up the question why I changed the meaning of 'method' and 'params' instead of adding the output format as an addition argument to the command arguments in 'params'. The server side would then interpret and filter out this argument before passing the remaining arguments to the command callbacks.
>>>
>>> I decided against this approach because the code would get more involved, in particular we would have to implement option/argument parsing inside process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a safe way would be difficult in general because their global state.)
>>> The current implementation is based purely on JSON objects/arrays which are nicely supported by OVS with functions from lib/json.*.
>> I'm not sure I got this point, but see below.
>>
>>> Ilya also voiced concerns about the limited extensibility of the proposed API. To fix this, this patch series could be tweaked in the follow way:
>>>
>>> (1.) Instead of passing the output format as second entry in the JSON array 'params', turn the second entry into a JSON object (shash).
> It has to be an array, not an object.  Parameters are positional.
> Unless you want to change API for every existing command and give
> each argument a unique name.
>
> And that will not help with supporting older servers, because they
> expect an array.

JSON-RPC 1.0 defines 'params' as a "Array of objects to pass as arguments to the method." [0]. Putting JSON objects into that array is valid.

Please have a look at the source code changes in unixctl.c. Hopefully it helps with understanding my approach:

Basically, I changed the meaning of the JSON-RPC API. Previously, there was a 1-1 mapping between command+args and JSON-RPC's method+params. Now, command+args are part of 'params'.

[0] https://www.jsonrpc.org/specification_v1

(After reading though your mail completely, I am thinking about reevaluating this decision as explained below.)

>
>>> The output format would be one entry in this JSON object, e.g. called 'format'.
>>>
>>> The server (process_command() from lib/unixctl.c) will parse the content of this JSON object into known options. Clients would only add non-default options to this JSON object to keep compatibility with older servers. Options which are supported by the server but not transferred via this JSON object would be initialized with default values to keep compatibility with older clients. Unknown entries would cause the server to return an error.
>> This kind of implements what I had in mind, but in a slightly different
>> manner.  How about something like this:
>>
>>  * ovs-appctl has a global parameter --format passed to it, not part of
>>    the requested command paramaters.  (I think this is implemented in the
>>    code, below, but I didn't read very carefully.)

Yes, --format is a global parameter.

>>
>>  * ovs-appctl adds this paramater as an argument to a 'params' JSON array.
>>    i.e. separate element in the array to every command it executes, if
>>    provided.
>>
>>  * Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
>>    that argument, removes it from the list of parameters before passing
>>    to the actual handler.

This would mean that ovs-appctl would parse the '--format' option and the server would have to parse it again, this time from the JSON array ('params'). This is what I tried to express above: The server would have to implement this option parsing in 'process_commands()' even though ovs-appctl already had parsed the user input. Besides duplicating work (parsing twice) you would have to duplicate code, because getopt* (which is used in appctl) cannot be used on the server side. It cannot be, because getopt* has global state and thus it is not safe in general.

>>
>>  * Before calling a handler, server stores these special parameters
>>    into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
>>    really matter).

struct unixctl_conn is only declared in unixctl.h, command callbacks do not have access to its definition. Which makes sense because it is an implementation detail which command callbacks do not have to care about. In contrast, adding a separate argument, whether it is an plain value or a wrapped in a (new) struct, highlights the intent (=> consumable by callback).

>>
>>  * Handler always has the conn pointer.  Handler can ask in which format
>>    the user prefers the output and generate one.  E.g.:
>>
>>      if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
>>          unixctl_command_reply_json(conn, ...);
>>      } else {
>>          unixctl_command_reply(conn, ...);
>>      }
>>
>>    Notice that nothing bad happens if command is not aware of the formats,
>>    because unixctl_command_reply() is a JSON reply, but with a simple
>>    JSON string instead of any fancy structure.

We talked about how to handle cases where a user asks for an unsupported format before. IIRC the consensus was that a command is supposed to fail if the user asks for JSON output but it has not been implemented (yet). Silently ignoring this and just returning text output instead is against common practice.

>>
>>  * Result is sent back to the ovs-appctl.
>>
>>  * If the --format=json was originally requested, just dump the result
>>    as JSON to the output in the unixctl_client_transact().  If format
>>    wasn't requested, parse it and print out the same way as it is done
>>    right now, if the result is not a simple JSON string - error.

Not sure what you mean with your second sentence. What I do is dump whatever is returned. If it is a simple JSON string, print it as we did before. If it is JSON object or JSON array, it is also just printed (in future revision: pretty-printed if requested by user). If it is anything else then fail as we did before.

>>
>> This approach requires no changes in the code that doesn't want to provide
>> JSON output and no changes to the command registering API.

Because you are cheating: Your approach turns struct unixctl_conn from an implementation detail into a part of the public API 😋 Suddenly the developer of the callback(s) also have access to the other parameters in unixctl_conn. I would prefer to keep unixctl_conn as is and instead introduce a new argument which makes our intent clear.

>> All commands support JSON output automatically.  But commands that do
>> not handle it specially will just return a simple JSON string.  We will
>> print it out as a JSON string, there is nothing bad in this.

In that sense, all commands already support JSON output! The result of unixctl_command_reply is a JSON string already.

>>
>> Prettyfication can be done on the ovs-appctl side by an extra argument
>> that doesn't need to be transferred to the server (already done this way).

Agree, will implement it in a follow up.

>>
>> The only issue is that it's hard to tell what happened when a new
>> ovs-appctl --format=json communicates with the old server and gets a
>> number of arguments error.  But we could add a hint 'Can be caused by an
>> older <target> not supporting JSON format' to every such reply if format
>> was requested and let user try without format and get a proper error if
>> there is one.  In any case, that should not be a frequent case, as I
>> would expect the most users of JSON format will be scripts with their
>> own JSON-RPC implementation, not ovs-appctl.
>>
>> Thoughts?

In my current implementation, ovs-appctl is able to detect when a (old) server does not support the new API. It will fail with a proper error message, telling the user to upgrade its server implementation.

If we keep the JSON-RPC API as it is today, there is no nice way of detecting old servers.



However, I am still thinking about your approach. Keeping the JSON-RPC API as it is today, has the downsides listed above (messier code, no nice way of handling backward compat to older servers), but the over-the-socket API would be cleaner.

Would be nice to hear some other people's opinion on the tradeoffs.

>> Some more comments below.
>>
>>> For (2.) see below.
>>>
>>>> unixctl_command_register() in lib/unixctl.* has been cloned as
>>>> unixctl_command_register_fmt() in order to demonstrate the new API.
>>>> 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 is passed via unix socket 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.
>>> (2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type unixctl_cb_func, an indirection will be used:
>>>
>>> The enum value will be put into a struct and the struct will be added to unixctl_cb_func. This would allow us to add new options easily without having to change all command callbacks again.
>> We already have strunc unixctl_conn where we can store generic things
>> and have functions-accessors.

Discussed above.

>>
>>> Wdyt?
>>>
>>>> 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>
>>>> ---
>>>>  lib/command-line.c     |  36 ++++++
>>>>  lib/command-line.h     |  10 ++
>>>>  lib/dpctl.h            |   4 +
>>>>  lib/unixctl.c          | 260 ++++++++++++++++++++++++++++++++---------
>>>>  lib/unixctl.h          |  17 ++-
>>>>  tests/pmd.at           |   5 +
>>>>  utilities/ovs-appctl.c |  65 ++++++++---
>>>>  utilities/ovs-dpctl.c  |  12 ++
>> Don't touch interface of the ovs-dpctl binary.  It doesn't need to have
>> JSON output.  ovs-appctl dpctl/ should pretty much always be used instead.
>>
>> Best regards, Ilya Maximets.

What interface should not be touched? CLI args of ovs-dpctl? Or ovs-dpctl in general?

Gruß,
Jakob