On 13 Mar 2026, at 21:51, Timothy Redaelli via dev wrote:
> When --format json is passed to ovs-appctl, dpctl/show returns a JSON
> array with one object per datapath. Each object contains the datapath
> name, lookup statistics, flow count, mask statistics, cache statistics,
> and a 'ports' array. Optional fields (masks, cache, port config and
> statistics) are omitted when the datapath does not support them or has
> no data.
>
> The output_format and conn fields are added to struct dpctl_params to
> pass the requested format and unixctl connection through the existing
> callback infrastructure. Setting conn to NULL after sending a JSON
> reply prevents the caller from sending a redundant text reply.
>
> Example output:
> [{"flows":0,"lookups":{"hit":0,"lost":0,"missed":0},
> "name":"ovs-system",
> "ports":[{"name":"br0","port_no":0,"type":"internal"}]}]
>
> Signed-off-by: Timothy Redaelli <[email protected]>
Hi Timothy,
This patch needs several structural changes to be consistent with the
other patches in the series and to provide a stable JSON schema.
First, please index the datapaths by name rather than using an array.
Also, index the ports by their port number. Here's the suggested
structure:
{
"ovs-system": {
"flows": 0,
"lookups": {
"hit": 1000,
"missed": 200,
"lost": 5
},
"masks": {
"hit": 800,
"total": 10,
"hit_per_pkt": 0.67
},
"cache": {
"hit": 600,
"hit_rate_pct": 50.0
},
"caches": [],
"ports": {
"0": {
"name": "ovs-system",
"type": "internal",
"config": {},
"statistics": {...}
},
"1": {
"name": "br0",
"type": "internal",
"config": {},
"statistics": {...}
}
}
}
}
The current code has nested conditionals where cache can only exist if
masks exists. These should be independent fields. Also, the schema should
not vary based on command-line flags like --statistics. Consider always
including these fields with empty objects or null when not applicable.
You might also want to check for memory leaks in error cases.
There is a lot of code added in dpctl_show() for the JSON case dumping
all datapaths. I suggest the show_dpif_json() function should be written
so you can simplify this with something like:
lasterror = dps_for_each(dpctl_p, show_dpif_json)
Finally, please split the show function into separate text and JSON
functions like patch 1, and update the unit test to check the full output
rather than using grep. Also, add a test to verify the complete
structure.
Cheers,
Eelco
[...]
> diff --git a/lib/dpctl.h b/lib/dpctl.h
> index 9d0052152..b8a2ee582 100644
> --- a/lib/dpctl.h
> +++ b/lib/dpctl.h
> @@ -51,6 +51,16 @@ struct dpctl_params {
>
> /* 'usage' (if != NULL) gets called for the "help" command. */
> void (*usage)(void *aux);
> +
> + /* Output format requested by the caller. One of UNIXCTL_OUTPUT_FMT_*.
> + * Use int to avoid pulling in unixctl.h from this header. */
> + int output_format;
Should this be enum unixctl_output_fmt ?
> +
> + /* Opaque pointer to struct unixctl_conn *, used to send JSON replies
> + * directly from command handlers. Set to NULL by a handler after it
> + * has already replied via unixctl_command_reply_json(), to prevent the
> + * caller from sending a second reply. */
> + void *conn;
> };
>
> int dpctl_run_command(int argc, const char *argv[],
> diff --git a/tests/dpctl.at b/tests/dpctl.at
> index a87f67f98..dd3d053a6 100644
> --- a/tests/dpctl.at
> +++ b/tests/dpctl.at
> @@ -25,6 +25,10 @@ dummy@br0:
> flows: 0
> port 0: br0 (dummy-internal)
> ])
> +dnl Check dpctl/show JSON output.
> +AT_CHECK([ovs-appctl --format json dpctl/show dummy@br0 | dnl
> + grep '"name"' | grep '"flows"' | dnl
> + grep '"lookups"' | grep '"ports"'], [0], [ignore])
> AT_CHECK([ovs-appctl dpctl/add-if dummy@br0 vif1.0,type=dummy,port_no=5])
> AT_CHECK([ovs-appctl dpctl/show dummy@br0], [0], [dnl
> dummy@br0:
> --
> 2.53.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
