On 4/2/26 1:30 PM, Eelco Chaudron via dev wrote:
>
>
> 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",
I'd say the name should be the key. Not sure what's the best name
for the port number would be though. We can keep it as "port-no",
I guess.
> "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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
