On 30.10.23 11:19, Eelco Chaudron wrote: > 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?
You discovered an bug here. I addressed it by fixing the ovs_output_fmt_to_json() function instead: It will check for a NULL value properly now. ovs_output_fmt_to_string() continues to report NULL, because I do not want to silently discard errors. >> Anyway, will do a full review of the next version after the discussion with >> Ilya finishes. Please gimme time to send a new patch series first. It will be less tedious for you to review, hopefully.. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
