On 4/12/24 09:26, jm...@redhat.com wrote:
> From: Jakob Meng <c...@jakobmeng.de>
>
> The 'dpif/show' command now supports machine-readable JSON output in
> addition to the plain-text output for humans. An example would be:
>
> ovs-appctl --format json dpif/show
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <c...@jakobmeng.de>
> ---
> NEWS | 1 +
> ofproto/ofproto-dpif.c | 124 +++++++++++++++++++++++++++++++++++++----
> tests/pmd.at | 28 ++++++++++
> 3 files changed, 142 insertions(+), 11 deletions(-)
>
Hi, Jakob. Thanks for v9!
The general approach in the set seems reasonable, however I didn't
read the code carefully enough. I hope to do that once I'm back
from PTO in one week.
I didn't read the code in this patch either, but I have a couple
of general comments for the formatting below.
> diff --git a/NEWS b/NEWS
> index af83623b3..97b30233c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,7 @@ Post-v3.3.0
> E.g. members of objects and elements of arrays are printed one per
> line,
> with indentation.
> * 'list-commands' now supports output in JSON format.
> + * 'dpif/show' now supports output in JSON format.
> - Python:
> * Added support for choosing the output format, e.g. 'json' or 'text'.
> * Added new option [--pretty] to print JSON output in a readable
> fashion.
These NEWS entries look strange. "Output format for python" sounds
strange. Is it for every python library we have? The section should
be more specific on what it means.
<snip>
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 35a44b4df..cff80da15 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -112,6 +112,34 @@ dummy@ovs-dummy: hit:0 missed:0
> p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
> ])
>
> +AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
> +[[
> + {
> + "name": "dummy@ovs-dummy",
> + "ofprotos": [
> + {
> + "name": "br0",
> + "ports": [
> + {
> + "netdev_config": {
> + },
> + "netdev_name": "br0",
> + "netdev_type": "dummy-internal",
> + "odp_port": "100",
> + "ofp_port": "65534"},
> + {
> + "netdev_config": {
> + "n_rxq": "1",
> + "n_txq": "1",
> + "numa_id": "0"},
> + "netdev_name": "p0",
> + "netdev_type": "dummy-pmd",
> + "odp_port": "1",
> + "ofp_port": "1"}]}],
> + "stats": {
> + "n_hit": "0",
> + "n_missed": "0"}}]]])
I'd suggest a different format for this command, e.g.:
{
"dummy@ovs-dummy": {
"bridges": {
"br0": {
"br0": {
"config": {},
"type": "dummy-internal",
"port_no": "100",
"ofport": "65534"},
"p0": {
"config": {
"n_rxq": "1",
"n_txq": "1",
"numa_id": "0"},
"type": "dummy-pmd",
"port_no": "1",
"ofport": "1"}}},
"stats": {
"n_hit": "0",
"n_missed": "0"}}
}
Using dictionaries with names as keys saves us from some of the strange
naming. "ofprotos" is a strange word and is not understandable by users.
It's an internal word. The user-facing entity is a bridge, not ofproto.
We don't need to have "netdev_" prefixes, it's already clear that it is
a port configuration if it is a part of port values. All datapaths,
bridges and ports have unique names, so they should be keys in JSON objects,
no need to use arrays. "ofp_port" means "OpenFlow port port", no need to
repeat. In the database we have "ofport" name, so we can use it here as
well. "dpif" commands operate with "port_no" as a datapath port number,
so that should be used instead of "odp_port".
Does that make sense?
Also, I noticed that non-pretty output is not sorted, this may cause
random CI failures, because the order depends on a hash function for
smap. I'd say the output should always be sorted, or we'll have to
somehow sort the values in tests.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
