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?
>> "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
