On 30 Oct 2023, at 11:07, Jakob Meng wrote:
> On 27.10.23 16:27, Eelco Chaudron wrote: >> On 25 Oct 2023, at 11:37, [email protected] wrote: >>> From: Jakob Meng <[email protected]> >>> >>> Add global option to output JSON from ovs-appctl cmds. >>> >>> This patch is an update of [0] with the following major changes: >>> * The JSON-RPC API change is now backward compatible. 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 are >>> updated. >>> * tests/pmd.at from forth patch now features an example of how the >>> output looks like when a command does not support JSON output. >>> * The patch has been split into a series of four. The first patch >>> introduces the '-f,--format' option for ovs-appctl/ovs-dpctl and >>> necessary changes to the JSON-RPC API. It does not yet pass the >>> output format to individual commands because that requires a lot >>> of changes. Those changes have been split out into the third patch >>> to increase readability of the series. >>> * The second patch introduces equivalent changes to the Python files. >>> * The third patch moves all commands to the updated functions in >>> lib/unixctl.*, in particular unixctl_command_register() and the >>> unixctl_cb_func type, as well as their Python counterparts. The >>> output is still text-only (no json) for all commands. >>> * The forth patch shows how JSON output could be implemented using >>> 'dpif/show' as an example. >>> >>> The following paragraphs are taken from the previous patch revision >>> and have been updated to changes mentioned above. >>> >>> For monitoring systems such as Prometheus it would be beneficial if OVS >>> and OVS-DPDK would expose statistics in a machine-readable format. >>> Several approaches like UNIX socket, OVSDB queries and JSON output from >>> ovs-xxx tools have been proposed [2],[3]. This proof of concept >>> describes one way how ovs-xxx tools could output JSON in addition to >>> plain-text for humans. >>> >>> This patch follows an alternative approach to RFC [1] which >>> implemented JSON output as a separate option for each command like >>> 'dpif/show'. The option was called '-o|--output' in the latter. It >>> has been renamed to '-f,--format' because ovs-appctl already has a >>> short option '-o' which prints the available ovs-appctl options >>> ('--option'). The new option name '-f,--format' is in line with >>> ovsdb-client where it controls output formatting, too. >>> >>> An example call would be 'ovs-appctl --format json dpif/show' as >>> shown in tests/pmd.at of the forth patch. By default, the output >>> format is plain-text as before. >>> >>> With this patch, all commands announce their support for output >>> formats when being registered with unixctl_command_register() from >>> lib/unixctl.*, e.g. OVS_OUTPUT_FMT_TEXT and/or OVS_OUTPUT_FMT_JSON. >>> When a requested output format is not supported by a command, then >>> process_command() in lib/unixctl.c will return an error. This is an >>> advantage over the previous approach [1] where each command would have >>> to parse the output format option and handle requests for unsupported >>> output formats on its own. >>> >>> The question whether JSON output should be pretty printed and sorted >>> remains. In most cases it would be unnecessary because machines >>> consume the output or humans could use jq for pretty printing. However, >>> it would make tests more readable (for humans) without having to use jq >>> (which would require us to introduce a dependency on jq). >> Hi Jakob, >> >> I had some code-related comments on V1, which I do not see addressed or >> replied to. Did you miss them? Anyway, I’ll go over my old review notes, and >> add them to the split-up patch. > > Which one did I miss? > > The note on JSON pretty printing I left here as a remainder for the > discussion on last thursday. With the consensus on adding an extra flag for > pretty printing I will remove it in the next patch version. > > You asked about freeing args->target which is done a couple of lines above in > "cmdl_args_destroy()". > > The rest of your comments has been incorporated in v3 in one way or another 😉 Maybe I did not go over it in to much detail (it was Friday afternoon), but I missed the first comment: >> +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; > > Returning NULL might cause a problem in the json_string_create() below, how > about returning “text” as this is the default value? > >> + } >> +} Anyway, will do a full review of the next version after the discussion with Ilya finishes. //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
