On 18 Jan 2024, at 16:26, jm...@redhat.com wrote: > From: Jakob Meng <c...@jakobmeng.de> > > Signed-off-by: Jakob Meng <c...@jakobmeng.de>
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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev