On 18 Jan 2024, at 16:26, [email protected] wrote:
> From: Jakob Meng <[email protected]>
>
> Signed-off-by: Jakob Meng <[email protected]>
Some comments below and you might want to add a commit message to this patch.
//Eelco
> ---
> NEWS | 3 +++
> lib/unixctl.c | 4 ++--
> lib/unixctl.h | 1 +
> tests/pmd.at | 29 +++++++++++++++++++++++++++--
> utilities/ovs-appctl.c | 22 +++++++++++++++++++---
> 5 files changed, 52 insertions(+), 7 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 94a347246..12905ac86 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -35,6 +35,9 @@ v3.3.0 - xx xxx xxxx
> on mark and labels.
> * Added new option [-f|--format] to choose the output format, e.g.
> 'json'
> or 'text' (by default).
> + * Added new option [--pretty] to print JSON output in a readable
> fashion.
> + E.g. members of objects and elements of arrays are printed one per
> line,
> + with indentation.
> - Python:
> * Added support for choosing the output format, e.g. 'json' or 'text'.
> - ovs-vsctl:
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index 3b9f300ba..1b216795d 100644
> --- a/lib/unixctl.c
> +++ b/lib/unixctl.c
> @@ -553,7 +553,7 @@ 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[], int fmt_flags, char **result, char
> **err)
Perhaps it would be more consistent to use format_flags here as well, mirroring
the structure definition. Additionally, using an unsigned int might be
preferable, as outlined below.
> {
> struct jsonrpc_msg *request, *reply;
> struct json **json_args, *params;
> @@ -590,7 +590,7 @@ unixctl_client_transact(struct jsonrpc *client, const
> char *command, int argc,
> *result = xstrdup(json_string(reply->result));
> } else if (reply->result->type == JSON_OBJECT ||
> reply->result->type == JSON_ARRAY) {
> - *result = json_to_string(reply->result, 0);
> + *result = json_to_string(reply->result, fmt_flags);
> } 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 35ef6a548..fe9160894 100644
> --- a/lib/unixctl.h
> +++ b/lib/unixctl.h
> @@ -39,6 +39,7 @@ int unixctl_client_create(const char *path, struct jsonrpc
> **client);
> int unixctl_client_transact(struct jsonrpc *client,
> const char *command,
> int argc, char *argv[],
> + int fmt_flags,
> char **result, char **error);
>
> /* Command registration. */
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 57660c407..cff80da15 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -112,8 +112,33 @@ dummy@ovs-dummy: hit:0 missed:0
> p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
> ])
>
> -AT_CHECK([ovs-appctl --format json dpif/show], [0], [dnl
> -[[{"ofprotos":[{"name":"br0","ports":[{"netdev_type":"dummy-internal","ofp_port":"65534","odp_port":"100","netdev_config":{},"netdev_name":"br0"},{"netdev_type":"dummy-pmd","ofp_port":"1","odp_port":"1","netdev_config":{"numa_id":"0","n_txq":"1","n_rxq":"1"},"netdev_name":"p0"}]}],"name":"dummy@ovs-dummy","stats":{"n_hit":"0","n_missed":"0"}}]]])
It might be worth considering retaining this test to ensure it functions
properly even without the pretty options. Additionally, we could think about
adding a separate test (not in pmd.at) to be executed with make check,
specifically testing the JSON format with both pretty and non-pretty options in
the final patch.
> +AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
> +[[
> + {
> + "name": "dummy@ovs-dummy",
> + "ofprotos": [
> + {
> + "name": "br0",
> + "ports": [
> + {
> + "netdev_config": {
> + },
> + "netdev_name": "br0",
> + "netdev_type": "dummy-internal",
> + "odp_port": "100",
> + "ofp_port": "65534"},
> + {
> + "netdev_config": {
> + "n_rxq": "1",
> + "n_txq": "1",
> + "numa_id": "0"},
> + "netdev_name": "p0",
> + "netdev_type": "dummy-pmd",
> + "odp_port": "1",
> + "ofp_port": "1"}]}],
> + "stats": {
> + "n_hit": "0",
> + "n_missed": "0"}}]]])
>
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index 02df8ba97..03c71ffad 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -26,6 +26,7 @@
> #include "daemon.h"
> #include "dirs.h"
> #include "openvswitch/dynamic-string.h"
> +#include "openvswitch/json.h"
> #include "jsonrpc.h"
> #include "process.h"
> #include "timeval.h"
> @@ -39,6 +40,7 @@ static void usage(void);
> /* Parsed command line args. */
> struct cmdl_args {
> enum ovs_output_fmt format;
> + int format_flags;
Considering that this involves combining bit-masks, I suggest converting this
to an unsigned int.
> char *target;
> };
>
> @@ -73,7 +75,7 @@ main(int argc, char *argv[])
>
> if (opt_argv.n > 0) {
> error = unixctl_client_transact(client, "set-options",
> - opt_argv.n, opt_argv.names,
> + opt_argv.n, opt_argv.names, 0,
> &cmd_result, &cmd_error);
>
> if (error) {
> @@ -97,7 +99,9 @@ main(int argc, char *argv[])
> 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_flags, &cmd_result,
> + &cmd_error);
> +
> if (error) {
> ovs_fatal(error, "%s: transaction error", args->target);
> }
> @@ -143,6 +147,11 @@ 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\
> + --pretty By default, JSON in output is printed as compactly as\n\
> + possible. This option causes JSON in output to be\n\
> + printed in a more readable fashion. Members of
> objects\n\
> + and elements of arrays are printed one per line, with\n\
> + indentation.\n\
> -h, --help Print this helpful information\n\
> -V, --version Display ovs-appctl version information\n",
> program_name, program_name);
> @@ -154,6 +163,7 @@ cmdl_args_create(void) {
> struct cmdl_args *args = xmalloc(sizeof *args);
>
> args->format = OVS_OUTPUT_FMT_TEXT;
> + args->format_flags = 0;
> args->target = NULL;
>
> return args;
> @@ -173,7 +183,8 @@ parse_command_line(int argc, char *argv[])
> {
> enum {
> OPT_START = UCHAR_MAX + 1,
> - VLOG_OPTION_ENUMS
> + OPT_PRETTY,
> + VLOG_OPTION_ENUMS,
> };
> static const struct option long_options[] = {
> {"target", required_argument, NULL, 't'},
> @@ -181,6 +192,7 @@ parse_command_line(int argc, char *argv[])
> {"format", required_argument, NULL, 'f'},
> {"help", no_argument, NULL, 'h'},
> {"option", no_argument, NULL, 'o'},
> + {"pretty", no_argument, NULL, OPT_PRETTY},
> {"version", no_argument, NULL, 'V'},
> {"timeout", required_argument, NULL, 'T'},
> VLOG_LONG_OPTIONS,
> @@ -233,6 +245,10 @@ parse_command_line(int argc, char *argv[])
> ovs_cmdl_print_options(long_options);
> exit(EXIT_SUCCESS);
>
> + case OPT_PRETTY:
> + args->format_flags |= JSSF_PRETTY | JSSF_SORT;
Since these flags are only relevant for the JSON format, perhaps we should only
set them if the requested format was JSON.
We could utilize a boolean variable here and do the verification/setting
outside the loop. We might even consider raising an error if set with any mode
other than JSON.
> + break;
> +
> case 'T':
> if (!str_to_uint(optarg, 10, &timeout) || !timeout) {
> ovs_fatal(0, "value %s on -T or --timeout is invalid",
> optarg);
> --
> 2.39.2
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
