On 4/2/26 3:58 PM, Eelco Chaudron wrote:
>
>
> On 2 Apr 2026, at 15:54, Ilya Maximets wrote:
>
>> 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.
>
> The name might make more sense. 'port-number', no need to shorten it?
Sounds fine.
>
>>> "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
