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