| Message ID | 20240118152657.2816536-6-jmeng@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add global option to output JSON from ovs-appctl cmds. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote: > From: Jakob Meng <code@jakobmeng.de> > > Signed-off-by: Jakob Meng <code@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
On 3/15/24 11:27, Eelco Chaudron wrote: > On 18 Jan 2024, at 16:26, jmeng@redhat.com wrote: > >> From: Jakob Meng <code@jakobmeng.de> >> 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. Keeping the test is a good thing, but I'd suggest to wrap the lines. You could define the output once with m4_define and then compare the non-pretty output with the result of m4_normalize and the pretty one with the original. Should be done in the previous patch and expanded in this one. BTW, is this output stable? It doesn't seem to be sorted. May cause random test failures and different architectures or with different compiler flags. Best regards, Ilya Maximets.
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) { 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"}}]]]) +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; 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; + break; + case 'T': if (!str_to_uint(optarg, 10, &timeout) || !timeout) { ovs_fatal(0, "value %s on -T or --timeout is invalid", optarg);