Re: [ovs-dev] [PATCH ovn 0/5] Fix I+P versus recompute differences.

[Ales Musil]

On Tue, Apr 23, 2024 at 1:54 PM Xavier Simonart  wrote:

> Comparing I+P flows versus flows after recompute highlighted a few
> issues.
>
> Xavier Simonart (5):
>   controller: Fix iface-id-ver handling.
>   controller: Nonvif related lports handling.
>   controller: Fix deletion of container parent port.
>   controller: Handle postponed ports claims.
>   controller: Handle postponed ports release.
>
>  controller/binding.c  |  68 ++---
>  controller/physical.c |   3 +-
>  tests/ovn.at  | 115 --
>  3 files changed, 174 insertions(+), 12 deletions(-)
>
> --
> 2.31.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
The whole series looks good to me, thanks.

Acked-by: Ales Musil 
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2] northd: Fix an issue wrt mac binding aging.

[Indrajitt Valsaraj]

Issue:
In case of a Logical_Router without mac_binding_age_threshold set or a
Logical_Router with an incorrectly formatted mac_binding_threshold
option, entries were not being purged from the Mac Binding table in
SouthBound.

This was because in the function `en_mac_binding_aging_run` in case of
an invalid mac_binding_threshold entry or if mac_binding_threshold is
not set we are returning from the loop instead of iterating through the
remaining LRs. As a result, subsequent runs of the aging_waker node are
also not scehduled and we end up not purging any MAC Bindings.

Fix:
This patch fixes this issue by changing the return to a continue so that
we skip the current LR but continue processing for the remaining LRs.

Fixes: 78851b6ffb58 ("Support CIDR-based MAC binding aging threshold.")
Signed-off-by: Indrajitt Valsaraj 
Acked-by: Naveen Yerramneni 

---
v1:
  - Addressed review comment from Ales
v2:
  - Fix test failure
---
 northd/aging.c |   2 +-
 tests/ovn.at   | 107 +++--
 2 files changed, 77 insertions(+), 32 deletions(-)

diff --git a/northd/aging.c b/northd/aging.c
index b76963a2d..9685044e7 100644
--- a/northd/aging.c
+++ b/northd/aging.c
@@ -421,7 +421,7 @@ en_mac_binding_aging_run(struct engine_node *node, void 
*data OVS_UNUSED)
 if (!parse_aging_threshold(smap_get(>nbr->options,
 "mac_binding_age_threshold"),
_config)) {
-return;
+continue;
 }

 aging_context_set_threshold(, _config);
diff --git a/tests/ovn.at b/tests/ovn.at
index 486680649..5ab64ae9b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -34414,10 +34414,15 @@ AT_CHECK([ovn-nbctl lsp-set-addresses ln_port 
unknown])
 AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
 AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])

-AT_CHECK([ovn-nbctl lsp-add public public-gw])
-AT_CHECK([ovn-nbctl lsp-set-type public-gw router])
-AT_CHECK([ovn-nbctl lsp-set-addresses public-gw 00:00:00:00:10:00 router])
-AT_CHECK([ovn-nbctl lsp-set-options public-gw router-port=gw-public])
+AT_CHECK([ovn-nbctl lsp-add public public-gw-1])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-1 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-1 00:00:00:00:10:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-1 router-port=gw-1-public])
+
+AT_CHECK([ovn-nbctl lsp-add public public-gw-2])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-2 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-2 00:00:00:00:30:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-2 router-port=gw-2-public])

 AT_CHECK([ovn-nbctl lsp-add internal internal-gw])
 AT_CHECK([ovn-nbctl lsp-set-type internal-gw router])
@@ -34430,9 +34435,12 @@ AT_CHECK([ovn-nbctl lsp-set-addresses vif1 
"00:00:00:00:20:10 192.168.20.10"])
 AT_CHECK([ovn-nbctl lsp-add internal vif2])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif2 "00:00:00:00:20:20 192.168.20.20"])

-AT_CHECK([ovn-nbctl lr-add gw])
-AT_CHECK([ovn-nbctl lrp-add gw gw-public 00:00:00:00:10:00 192.168.10.1/24])
-AT_CHECK([ovn-nbctl lrp-add gw gw-internal 00:00:00:00:20:00 192.168.20.1/24])
+AT_CHECK([ovn-nbctl lr-add gw-1])
+AT_CHECK([ovn-nbctl lrp-add gw-1 gw-1-public 00:00:00:00:10:00 
192.168.10.1/24])
+AT_CHECK([ovn-nbctl lrp-add gw-1 gw-internal 00:00:00:00:20:00 
192.168.20.1/24])
+
+AT_CHECK([ovn-nbctl lr-add gw-2])
+AT_CHECK([ovn-nbctl lrp-add gw-2 gw-2-public 00:00:00:00:30:00 
192.168.10.2/24])

 sim_add hv1
 as hv1
@@ -34500,21 +34508,27 @@ send_udp() {
 as $hv ovs-appctl netdev-dummy/receive $dev $packet
 }
 # Check if the option is not present by default
-AT_CHECK([fetch_column nb:logical_router options name="gw" | grep -q 
mac_binding_age_threshold], [1])
+AT_CHECK([fetch_column nb:logical_router options name="gw-1" | grep -q 
mac_binding_age_threshold], [1])
+AT_CHECK([fetch_column nb:logical_router options name="gw-2" | grep -q 
mac_binding_age_threshold], [1])

 # Send GARP to populate MAC binding table records
 send_garp hv1 ext1 10
 send_garp hv2 ext2 20

-wait_row_count mac_binding 1 ip="192.168.10.10"
-wait_row_count mac_binding 1 ip="192.168.10.20"
+# Two rows present for each IP, one corresponding to each logical_port
+wait_row_count mac_binding 2 ip="192.168.10.10"
+wait_row_count mac_binding 2 ip="192.168.10.20"

-dp_key=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key 
external_ids:name=gw))
-port_key=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key 
logical_port=gw-public))
+dp_key_1=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key 
external_ids:name=gw-1))
+port_key_1=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key 
logical_port=gw-1-public))
+dp_key_2=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key 
external_ids:name=gw-2))
+port_key_2=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key 
logical_port=gw-2-public))

 AT_CHECK_UNQUOTED([as hv1 ovs-ofctl 

Re: [ovs-dev] [v2] odp-execute: Fix AVX checksum calculation.

[Ilya Maximets]

On 5/15/24 12:12, Eelco Chaudron wrote:
> 
> 
> On 14 May 2024, at 15:48, Emma Finn wrote:
> 
>> The AVX implementation for calcualting checksums was not
>> handling carry-over addition correctly in some cases.
>> This patch adds an additional shuffle to add 16-bit padding
>> to the final part of the calculation to handle such cases.
>> This commit also adds a unit test to fuzz test the actions
>> autovalidator.
>>
>> Signed-off-by: Emma Finn 
>> Reported-by: Eelco Chaudron 
> 
> Hi Emma,
> 
> Thanks for also fixing the IPv6 case, however, the test you added does
> not seem to catch the issue. See notes below.
> 
> Cheers,
> 
> Eelco
> 
>> ---
>>  lib/odp-execute-avx512.c |  5 +
>>  tests/dpif-netdev.at | 26 ++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
>> index 50c48bfd4..a74a85dc1 100644
>> --- a/lib/odp-execute-avx512.c
>> +++ b/lib/odp-execute-avx512.c
>> @@ -366,6 +366,8 @@ avx512_get_delta(__m256i old_header, __m256i new_header)
>>0xF, 0xF, 0xF, 0xF);
>>  v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
>>
>> +v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
>> +v_delta = _mm256_shuffle_epi8(v_delta, v_swap16a);
>>  v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
>>  v_delta = _mm256_hadd_epi16(v_delta, v_zeros);
>>
>> @@ -575,6 +577,9 @@ avx512_ipv6_sum_header(__m512i ip6_header)
>>0xF, 0xF, 0xF, 0xF);
>>
>>  v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
>> +
>> +v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
>> +v_delta = _mm256_shuffle_epi8(v_delta, v_swap16a);
>>  v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
>>  v_delta = _mm256_hadd_epi16(v_delta, v_zeros);
>>
>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
>> index 790b5a43a..4db6a99e1 100644
>> --- a/tests/dpif-netdev.at
>> +++ b/tests/dpif-netdev.at
>> @@ -1091,3 +1091,29 @@ OVS_VSWITCHD_STOP(["dnl
>>  /Error: unknown miniflow extract implementation superstudy./d
>>  /Error: invalid study_pkt_cnt value: -pmd./d"])
>>  AT_CLEANUP
>> +
>> +AT_SETUP([datapath - Actions Autovalidator Fuzzy])
> 
> This is not a Fuzzy test, but a normal Actions Autovalidator.

FWIW, even if it was, I don't think we should add any more fuzzy tests
in a general testsuite.  And we should find a way to get rid of the
existing ones.  Having non-reproducible tests is not good.

> 
> However, the main problem with this test is that it does not find the problem.
> Even without the C code changes, it’s passing the test.
> 
> Maybe it will be better to add a specific test to capture checksum wrapping 
> for
> IPv4 and 6. In addition, you should also make sure the received packet is ok.
> You can use options:pcap=p1.pcap for this, see other test cases.

I'd suggest to model the test after 'userspace offload - ip csum offload'
test case we have in tests/dpif-netdev.at.  It does very similar checks.

> 
>> +AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
>> +AT_SKIP_IF([! $PYTHON3 $srcdir/genpkts.py 2000 > packets])
>> +
>> +OVS_VSWITCHD_START(add-port br0 p0 -- set Interface p0 type=dummy \
>> +   -- add-port br0 p1 -- set Interface p1 type=dummy)
>> +
>> +AT_CHECK([ovs-appctl odp-execute/action-impl-set autovalidator], [0], [dnl
>> +Action implementation set to autovalidator.
>> +])
>> +
>> +AT_DATA([flows.txt], [dnl
>> +  in_port=p0,ip,actions=mod_nw_src=10.1.1.1,p1
>> +  in_port=p0,ipv6,actions=set_field:fc00::100->ipv6_src,p1
>> +])
>> +
>> +AT_CHECK([ovs-ofctl del-flows br0])
>> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt])
>> +
>> +cat packets | while read line; do
>> +  AT_CHECK([ovs-appctl netdev-dummy/receive p0 $line], [0], [ignore])
>> +done
>> +
>> +OVS_TRAFFIC_VSWITCHD_STOP
>> +AT_CLEANUP
>> -- 
>> 2.25.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] fix ct tp policy when create zone.

[Ilya Maximets]

On 5/14/24 09:50, Chandler Wu wrote:
> Hi, IIya, thanks for the attention. 
> 
> If I create a zone for the first time, the new tp_cfg will be copied
> to the zone, see `ct_zone_alloc`. Then update_timeout_policy will check
> that the new copied tp equals to tp_cfg, so ofproto_ct_set_zone_timeout_policy
> will not got called. Or you can check tag v3.0.0 or before. There's no such
> issue for these versions.

Ah, that makes sense.  Please, add this information to the commit message
before sending v2.

Please, also add the following tag:

Fixes: 1b3557f53dbc ("vswitchd, ofproto-dpif: Propagate the CT limit from 
database.")

For the code itself:

Maybe we should remove the get_timeout_policy_from_ovsrec() call from
ct_zone_alloc() instead of adding ofproto_ct_set_zone_timeout_policy()?
If we do not initialize it, the update_timeout_policy() later should
sync it correctly.  What do you think?

Best regards, Ilya Maximets.

> 
> Best regards.
> 
> On Tue, May 14, 2024 at 5:11 AM Ilya Maximets  > wrote:
> 
> On 5/6/24 06:05, Chandler Wu wrote:
> > From 5b4d479a9083272e56c51ef9521e6ef665dd534d Mon Sep 17 00:00:00 2001
> > From: chandlerwu  >
> > Date: Mon, 6 May 2024 11:58:21 +0800
> > Subject: [PATCH] Subject:[PATCH] fix ct tp policy when create zone.
> >
> > Signed-off-by: chandlerwu  >
> > ---
> >  vswitchd/bridge.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 95a65fcdc..e60359b59 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -766,6 +766,8 @@ ct_zones_reconfigure(struct datapath *dp, struct 
> ovsrec_datapath *dp_cfg)
> >          if (!ct_zone) {
> >              ct_zone = ct_zone_alloc(zone_id, tp_cfg);
> >              hmap_insert(>ct_zones, _zone->node, 
> hash_int(zone_id, 0));
> > +            ofproto_ct_set_zone_timeout_policy(dp->type, 
> ct_zone->zone_id,
> > +                                               _zone->tp);
> >          }
> > 
> >          struct simap new_tp = SIMAP_INITIALIZER(_tp);
> 
> Hi, Chandler Wu.  Thanks for the patch!
> 
> But could you, please, explain what is the issue you're trying to fix?
> 
> Reading the code, it seems that update_timeout_policy() call later in
> the function should correctly set the policy.  Is that not happening?
> 
> Best regards, Ilya Maximets.
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] route-table: Add support for v4 via v6 route.

[Ilya Maximets]

On 5/14/24 00:42, Ilya Maximets wrote:
> On 5/14/24 00:11, William Tu wrote:
>> Add route-table support for ipv4 dst via ipv6. One use case is BGP
>> unnumbered, a mechanism that establishes peering sessions without the
>> need to explicitly configure IPv4 addresses on the interfaces involved
>> in the peering. Without using IPv4 address assignments, it uses
>> link-local IPv6 addresses of the directly connected neighbors for
>> peering purposes. For example, BGP might install the following route:
>> $ ip route get 100.87.18.3
>> 100.87.18.3 via inet6 fe80::920a:84ff:fe9e:9570 \
>> dev br-phy src 100.87.18.6
>>
>> Note that the v6 addr fe80::920a:84ff:fe9e:9570 is not being used in
>> the packet header, but only used for lookup the out dev br-phy.
>> Currently OVS can only support either all-ipv4 or all-ipv6, the patch
>> adds support for such use case.
>>
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2024-January/052908.html
>> Acked-by: Simon Horman 
>> Signed-off-by: William Tu 
>> ---
>> v3: add vxlan test, remove rfc
>> v2: fix CI error
>> ---
>>  lib/ovs-router.c | 34 ++--
>>  lib/route-table.c| 21 ++
>>  tests/ovs-router.at  | 33 +++
>>  tests/system-route.at| 24 
>>  tests/tunnel-push-pop.at | 48 
>>  5 files changed, 142 insertions(+), 18 deletions(-)

I didn't look at the code changes too closely, but in case you
wan't to re-spin the patch, I added a few comments for the
test cases below.

Best regards, Ilya Maximets.



>> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
>> index b3314b3dff0d..f34dd3c99f5d 100644
>> --- a/tests/ovs-router.at
>> +++ b/tests/ovs-router.at
>> @@ -109,6 +109,39 @@ User: 2001:db8:beef::14/128 MARK 14 dev br0 GW 
>> 2001:db8:cafe::1 SRC 2001:db8:caf
>>  OVS_VSWITCHD_STOP
>>  AT_CLEANUP
>>  
>> +AT_SETUP([appctl - route/add with ipv4 via ipv6])

We should probably also add a few cases for the lookup and del,
not just the add.

>> +AT_KEYWORDS([ovs_router])
>> +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy])
>> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.2/24], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.3/24], [0], [OK
>> +])
>> +
>> +# defualt pick the first IP, 192.168.9.2, as src
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.10/32 br0 
>> fe80::920a:84ff:beef:9510], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.11/32 br0 
>> fe80::920a:84ff:beef:9511 src=192.168.9.2], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.12/32 br0 
>> fe80::920a:84ff:beef:9512 src=192.168.9.3], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.13/32 br0 
>> fe80::920a:84ff:beef:9513 pkt_mark=13 src=192.168.9.3], [0], [OK
>> +])

Might be better to wrap some of the longer lines, e.g. after the bridge name.

>> +
>> +AT_CHECK([ovs-appctl ovs/route/show | grep User | grep beef | sort], [0], 
>> [dnl
>> +User: 192.168.9.10/32 dev br0 GW fe80::920a:84ff:beef:9510 SRC 192.168.9.2
>> +User: 192.168.9.11/32 dev br0 GW fe80::920a:84ff:beef:9511 SRC 192.168.9.2
>> +User: 192.168.9.12/32 dev br0 GW fe80::920a:84ff:beef:9512 SRC 192.168.9.3
>> +User: 192.168.9.13/32 MARK 13 dev br0 GW fe80::920a:84ff:beef:9513 SRC 
>> 192.168.9.3
>> +])
>> +
>> +# DST and SRC must be in the same subnet domain
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.12/32 br0 
>> fe80::920a:84ff:beef:9512 src=192.168.9.3], [2], [], [dnl
>> +Error while inserting route.
>> +ovs-appctl: ovs-vswitchd: server returned an error
>> +])
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>>  AT_SETUP([appctl - route/lookup])
>>  AT_KEYWORDS([ovs_router])
>>  OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy])
>> diff --git a/tests/system-route.at b/tests/system-route.at
>> index c0ecad6cfb49..fd698321b401 100644
>> --- a/tests/system-route.at
>> +++ b/tests/system-route.at
>> @@ -65,6 +65,30 @@ Cached: fc00:db8:beef::13/128 dev br0 GW fc00:db8:cafe::1 
>> SRC fc00:db8:cafe::2])
>>  OVS_TRAFFIC_VSWITCHD_STOP
>>  AT_CLEANUP
>>  
>> +AT_SETUP([ovs-route - add system route ipv4 via ipv6])
>> +AT_KEYWORDS([route])
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +AT_CHECK([ip link set br0 up])
>> +
>> +AT_CHECK([ip addr add 192.168.9.2/24 dev br0], [0], [stdout])
>> +AT_CHECK([ip addr add 192.168.9.3/24 dev br0], [0], [stdout])
>> +
>> +AT_CHECK([ip -6 addr add fc00:db8:cafe::2/64 dev br0], [0], [stdout])
>> +AT_CHECK([ip -6 addr add fc00:db8:cafe::3/64 dev br0], [0], [stdout])
>> +
>> +AT_CHECK([ip route add 192.168.9.12/32 dev br0 via inet6 
>> fe80::920a:84ff:fe9e:9512 src 192.168.9.2], [0], [stdout])
>> +AT_CHECK([ip route add 192.168.9.13/32 dev br0 via inet6 
>> fe80::920a:84ff:fe9e:9513 src 192.168.9.3], [0], [stdout])

maybe wrap these lines.

>> +
>> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl ovs/route/show | grep -E 
>> 

Re: [ovs-dev] [PATCH 5/5] netdev-linux: Fix Clang's static analyzer uninitialized values warnings.

[0-day Robot]

Bleep bloop.  Greetings Mike Pattrick, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: The subject, ': ', is over 70 characters, i.e., 72.
Subject: netdev-linux: Fix Clang's static analyzer uninitialized values 
warnings.
Lines checked: 53, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 net] openvswitch: Set the skbuff pkt_type for proper pmtud support.

[Aaron Conole]

Open vSwitch is originally intended to switch at layer 2, only dealing with
Ethernet frames.  With the introduction of l3 tunnels support, it crossed
into the realm of needing to care a bit about some routing details when
making forwarding decisions.  If an oversized packet would need to be
fragmented during this forwarding decision, there is a chance for pmtu
to get involved and generate a routing exception.  This is gated by the
skbuff->pkt_type field.

When a flow is already loaded into the openvswitch module this field is
set up and transitioned properly as a packet moves from one port to
another.  In the case that a packet execute is invoked after a flow is
newly installed this field is not properly initialized.  This causes the
pmtud mechanism to omit sending the required exception messages across
the tunnel boundary and a second attempt needs to be made to make sure
that the routing exception is properly setup.  To fix this, we set the
outgoing packet's pkt_type to PACKET_OUTGOING, since it can only get
to the openvswitch module via a port device or packet command.

Even for bridge ports as users, the pkt_type needs to be reset when
doing the transmit as the packet is truly outgoing and routing needs
to get involved post packet transformations, in the case of
VXLAN/GENEVE/udp-tunnel packets.  In general, the pkt_type on output
gets ignored, since we go straight to the driver, but in the case of
tunnel ports they go through IP routing layer.

This issue is periodically encountered in complex setups, such as large
openshift deployments, where multiple sets of tunnel traversal occurs.
A way to recreate this is with the ovn-heater project that can setup
a networking environment which mimics such large deployments.  We need
larger environments for this because we need to ensure that flow
misses occur.  In these environment, without this patch, we can see:

  ./ovn_cluster.sh start
  podman exec ovn-chassis-1 ip r a 170.168.0.5/32 dev eth1 mtu 1200
  podman exec ovn-chassis-1 ip netns exec sw01p1 ip r flush cache
  podman exec ovn-chassis-1 ip netns exec sw01p1 \
 ping 21.0.0.3 -M do -s 1300 -c2
  PING 21.0.0.3 (21.0.0.3) 1300(1328) bytes of data.
  From 21.0.0.3 icmp_seq=2 Frag needed and DF set (mtu = 1142)

  --- 21.0.0.3 ping statistics ---
  ...

Using tcpdump, we can also see the expected ICMP FRAG_NEEDED message is not
sent into the server.

With this patch, setting the pkt_type, we see the following:

  podman exec ovn-chassis-1 ip netns exec sw01p1 \
 ping 21.0.0.3 -M do -s 1300 -c2
  PING 21.0.0.3 (21.0.0.3) 1300(1328) bytes of data.
  From 21.0.0.3 icmp_seq=1 Frag needed and DF set (mtu = 1222)
  ping: local error: message too long, mtu=1222

  --- 21.0.0.3 ping statistics ---
  ...

In this case, the first ping request receives the FRAG_NEEDED message and
a local routing exception is created.

Tested-by: Jaime Caamano 
Reported-at: https://issues.redhat.com/browse/FDP-164
Fixes: 58264848a5a7 ("openvswitch: Add vxlan tunneling support.")
Signed-off-by: Aaron Conole 
---
v1->v2: Include a comment as requested by Eelco, and add some details about
bridge port packets.

 net/openvswitch/actions.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 6fcd7e2ca81fe..9642255808247 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -936,6 +936,12 @@ static void do_output(struct datapath *dp, struct sk_buff 
*skb, int out_port,
pskb_trim(skb, ovs_mac_header_len(key));
}
 
+   /* Need to set the pkt_type to involve the routing layer.  The
+* packet movement through the OVS datapath doesn't generally
+* use routing, but this is needed for tunnel cases.
+*/
+   skb->pkt_type = PACKET_OUTGOING;
+
if (likely(!mru ||
   (skb->len <= mru + vport->dev->hard_header_len))) {
ovs_vport_send(vport, skb, ovs_key_mac_proto(key));
-- 
2.45.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/5] netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' warnings.

[0-day Robot]

Bleep bloop.  Greetings Mike Pattrick, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: The subject, ': ', is over 70 characters, i.e., 78.
Subject: netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' 
warnings.
Lines checked: 35, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] netdev-offload: Fix Clang's static analyzer 'null pointer dereference' warnings.

[0-day Robot]

Bleep bloop.  Greetings Mike Pattrick, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: The subject, ': ', is over 70 characters, i.e., 80.
Subject: netdev-offload: Fix Clang's static analyzer 'null pointer dereference' 
warnings.
Lines checked: 41, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 3/5] dpctl: Fix Clang's static analyzer 'garbage value' warnings.

[Mike Pattrick]

Clang's static analyzer will complain about an uninitialized value
because we weren't setting a value for ufid_generated in all code paths.

Now we initialize this on declaration.

Signed-off-by: Mike Pattrick 
---
 lib/dpctl.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 3c555a559..9c287d060 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1366,12 +1366,11 @@ dpctl_del_flow_dpif(struct dpif *dpif, const char 
*key_s,
 struct ofpbuf mask; /* To be ignored. */
 
 ovs_u128 ufid;
-bool ufid_generated;
-bool ufid_present;
+bool ufid_generated = false;
+bool ufid_present = false;
 struct simap port_names;
 int n, error;
 
-ufid_present = false;
 n = odp_ufid_from_string(key_s, );
 if (n < 0) {
 dpctl_error(dpctl_p, -n, "parsing flow ufid");
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/5] netdev-offload: Fix Clang's static analyzer 'null pointer dereference' warnings.

[Mike Pattrick]

Clang's static analyzer will complain about a null pointer dereference
because dumps can be set to null and then there is a loop where it could
have been written to.

Instead, return early from the netdev_ports_flow_dump_create function if
dumps is NULL.

Signed-off-by: Mike Pattrick 
---
 lib/netdev-offload.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
index 931d634e1..02b1cf203 100644
--- a/lib/netdev-offload.c
+++ b/lib/netdev-offload.c
@@ -638,7 +638,14 @@ netdev_ports_flow_dump_create(const char *dpif_type, int 
*ports, bool terse)
 }
 }
 
-dumps = count ? xzalloc(sizeof *dumps * count) : NULL;
+if (count == 0) {
+*ports = 0;
+ovs_rwlock_unlock(_to_netdev_rwlock);
+
+return NULL;
+}
+
+dumps = xzalloc(sizeof *dumps * count);
 
 HMAP_FOR_EACH (data, portno_node, _to_netdev) {
 if (netdev_get_dpif_type(data->netdev) == dpif_type) {
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 5/5] netdev-linux: Fix Clang's static analyzer uninitialized values warnings.

[Mike Pattrick]

Clang's static analyzer will complain about an uninitialized value
because in some error conditions we wouldn't set all values that are
used later.

Now we initialize more values that are needed later even in error
conditions.

Signed-off-by: Mike Pattrick 
---
 lib/netdev-linux.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 25349c605..66dae3e1a 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2439,7 +2439,9 @@ netdev_linux_read_definitions(struct netdev_linux *netdev,
 int error = 0;
 
 error = netdev_linux_read_stringset_info(netdev, );
-if (error || !len) {
+if (!len) {
+return -EOPNOTSUPP;
+} else if (error) {
 return error;
 }
 strings = xzalloc(sizeof *strings + len * ETH_GSTRING_LEN);
@@ -2724,6 +2726,7 @@ netdev_linux_get_speed_locked(struct netdev_linux *netdev,
   uint32_t *current, uint32_t *max)
 {
 if (netdev_linux_netnsid_is_remote(netdev)) {
+*current = 0;
 return EOPNOTSUPP;
 }
 
@@ -2733,6 +2736,8 @@ netdev_linux_get_speed_locked(struct netdev_linux *netdev,
? 0 : netdev->current_speed;
 *max = MIN(UINT32_MAX,
netdev_features_to_bps(netdev->supported, 0) / 100ULL);
+} else {
+*current = 0;
 }
 return netdev->get_features_error;
 }
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 4/5] socket: Fix Clang's static analyzer 'garbage value' warnings.

[Mike Pattrick]

Clang's static analyzer will complain about an uninitialized value
because we weren't setting a value for dns_failure in all code paths.

Now we initialize this on declaration.

Signed-off-by: Mike Pattrick 
---
 lib/socket-util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/socket-util.c b/lib/socket-util.c
index 2d89fce85..1d21ce01c 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -711,7 +711,7 @@ inet_open_passive(int style, const char *target, int 
default_port,
 struct sockaddr_storage ss;
 int fd = 0, error;
 unsigned int yes = 1;
-bool dns_failure;
+bool dns_failure = false;
 
 if (!inet_parse_passive(target, default_port, , true, _failure)) {
 if (dns_failure) {
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 0/5] clang: Fix Clang's static analyzer detections.

[Mike Pattrick]

Clang's static analyzer has identified several instances of uninitialized
variable usage and null pointer dereferencing that while not likely, is
possible. These mostly included making sure that a variable is properly set
or error code properly returned in every error condition.

Signed-off-by: Mike Pattrick 


Mike Pattrick (5):
  netdev-offload: Fix Clang's static analyzer 'null pointer dereference'
warnings.
  netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value'
warnings.
  dpctl: Fix Clang's static analyzer 'garbage value' warnings.
  socket: Fix Clang's static analyzer 'garbage value' warnings.
  netdev-linux: Fix Clang's static analyzer uninitialized values
warnings.

 lib/dpctl.c | 5 ++---
 lib/netdev-linux.c  | 7 ++-
 lib/netdev-native-tnl.c | 4 +++-
 lib/netdev-offload.c| 9 -
 lib/socket-util.c   | 2 +-
 5 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/5] netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' warnings.

[Mike Pattrick]

Clang's static analyzer will complain about an uninitialized value
because we weren't properly checking the error code from a function that
would have initialized the value.

Instead, add a check for that return code.

Signed-off-by: Mike Pattrick 
---
 lib/netdev-native-tnl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index dee9ab344..6e6b15764 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet)
 }
 
 pkt_metadata_init_tnl(md);
-netdev_tnl_ip_extract_tnl_md(packet, tnl, );
+if (netdev_tnl_ip_extract_tnl_md(packet, tnl, ) == NULL) {
+goto err;
+}
 dp_packet_reset_packet(packet, hlen);
 
 return packet;
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC 00/11] Add psample support to NXAST_SAMPLE action.

[Adrian Moreno]

On 4/24/24 9:53 PM, Adrian Moreno wrote:

This is the userspace counterpart of the work being done in the kernel
[1]. Sending it as RFC to get some early feedback on the overall
solution.

** Problem description **
Currently, OVS supports several observability features, such as
per-bridge IPFIX, per-flow IPFIX and sFlow. However, given the
complexity of OVN-generated pipelines, a single sample is typically not
enough to troubleshoot what is OVN/OVS is doing with the packet. We need
highler level metadata alongside the packet sample.

This can be achieved by the means of per-flow IPFIX sampling and
NXAST_SAMPLE action, through which OVN can add two arbitrary 32-bit
values (observation_domain_id and observation_point_id) that are sent
alongside the packet information in the IPFIX frames.

However, there is a fundamental limitation of the existing sampling
infrastructure, specially in the kernel datapath: samples are handled by
ovs-vswitchd, forcing the need for an upcall (userspace action). The
fact that samples share the same netlink sockets and handler thread cpu
time with actual packet misse, can easily cause packet drops making this
solution unfit for use in highly loaded production systems.

Users are left with little option than guessing what sampling rate will
be OK for their traffic pattern and dealing with the lost accuracy.

** Feature Description **
In order to solve this situation and enable this feature to be safely
turned on in production, this RFC uses the psample support proposed in
[1] to send NXAST_SAMPLE samples to psample adding the observability
domain and point information as metadata.

~~ API ~~
The API is simply a new field called "psample_group" in the
Flow_Sample_Collector_Set (FSCS) table. Configuring this value is
orthogonal to also associating the FSCS entry to an entry in the IPFIX
table.

Apart from that configuration, the controller needs to add NXAST_SAMPLE
actions that refer the entry's id.

~~ HW Offload ~~
psample is already supported by tc using the act_sample action. The
metadata is taken from the actions' cookie.
This series also adds support for offloading the odp sample action,
only when it includes psample information but not nested actions.

A bit of special care has to be taken in the tc layer to not store the
ufid in the sample's cookie as it's used to carry action-specific data.

~~ Datapath support ~~
This new behavior of the datapth sample action is only supported on the
kernel datapath. A more detailed analysis is needed (and planned) to
find a way to also optimize the userspace datapath. However, although
potentially inefficient, there is not that much risk of dropping packets
with the existing IPFIX infrastructure.

~~ Testing ~~
The series includes an utility program called "ovs-psample" that listens
to packets multicasted by the psample module and prints them (also
printing the obs_domain and obs_point ids). In addition the kernel
series includes a tracepoint for easy testing and troubleshooting.

[1] https://patchwork.kernel.org/project/netdevbpf/list/?series=847473




Hi all,

I had an offline meeting with Eelco, Ilya and Aaron about this topic and wanted 
to put out what was discussed and hopefully get more feedback.


In general, 3 options were considered:

Option A: Reusing the sample action
===
This is essentially what this proposal (and it's kernel counterpart) consists 
in. The datapath action would look like this:


 sample(probability=50%, actions(...), group=10, cookie=0x123)

Pros

- In userspace, it fits nicely with the existing per-flow sampling 
infrastructure as this RFC exemplifies.


- The probability is present in the context of sending the packet to psample so 
it's easily carried to psample's rate, making the consumer aware of the accuracy 
of the sample.


- Relatively easy to implement in tc as a act_sample exists with similar 
semantics.

Cons

- It breaks the original design of the "sample" action. The "sample" action 
means: The packet is sampled (a probability is calculated) and, if the result is 
positive, a set of nested actions is executed. This follows the 
"building-blocks" approach of datapath action. This option breaks this 
assumption by adding more semantics and behavior to the "sample" action.


- If we want to add "trunc" to this sampling, the result would probably work but 
is not very nice because we need it outside of the sample() action, e.g:
   trunc(100),sample(probability=50%, actions(...), group=10, 
cookie=0x123),trunc(0)


- Makes the uAPI a bit clunky by adding more attributes to an existing, simple 
action. Also, new attributes and existing nested actions are completely 
orthogonal so code needs to exist in both userspace and kernel to properly split 
this behavior.


- When "trunc" is used, psample will report the original skb length, regardless 
of whether the "trunc" is associated to the sample action or not. Not sure this 
is a huge problem nor if it's easy 

Re: [ovs-dev] [PATCH v12 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[0-day Robot]

Bleep bloop.  Greetings Jakob Meng, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#113 FILE: utilities/ovs-appctl.c:157:
 Requires: --format=json.\n\

Lines checked: 196, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] compiler: Fix errors in Clang 17 ubsan checks.

[Mike Pattrick]

Recheck-request: github-robot

On Thu, May 16, 2024 at 9:58 AM Mike Pattrick  wrote:
>
> This patch attempts to fix a large number of ubsan error messages that
> take the following form:
>
> lib/netlink-notifier.c:237:13: runtime error: call to function
> route_table_change through pointer to incorrect function type
> 'void (*)(const void *, void *)'
>
> In Clang 17 the undefined behaviour sanatizer check for function
> pointers was enabled by default, whereas it was previously disabled
> while compiling C code. These warnings are a false positive in the case
> of OVS, as our macros already check to make sure the function parameter
> is the correct size.
>
> So that check is disabled in the single function that is causing all of
> the errors.
>
> Signed-off-by: Mike Pattrick 
> ---
> v2: Changed macro name
> ---
>  include/openvswitch/compiler.h | 11 +++
>  lib/ovs-rcu.c  |  1 +
>  2 files changed, 12 insertions(+)
>
> diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h
> index 878c5c6a7..f49b23683 100644
> --- a/include/openvswitch/compiler.h
> +++ b/include/openvswitch/compiler.h
> @@ -69,6 +69,17 @@
>  #define OVS_UNLIKELY(CONDITION) (!!(CONDITION))
>  #endif
>
> +/* Clang 17's implementation of ubsan enables checking that function pointers
> + * match the type of the called function. This currently breaks ovs-rcu, 
> which
> + * calls multiple different types of callbacks via a generic void *(void*)
> + * function pointer type. This macro enables disabling that check for 
> specific
> + * functions. */
> +#if __clang__ && __has_feature(undefined_behavior_sanitizer)
> +#define OVS_NO_SANITIZE_FUNCTION __attribute__((no_sanitize("function")))
> +#else
> +#define OVS_NO_SANITIZE_FUNCTION
> +#endif
> +
>  #if __has_feature(c_thread_safety_attributes)
>  /* "clang" annotations for thread safety check.
>   *
> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
> index 9e07d9bab..597fe6826 100644
> --- a/lib/ovs-rcu.c
> +++ b/lib/ovs-rcu.c
> @@ -327,6 +327,7 @@ ovsrcu_postpone__(void (*function)(void *aux), void *aux)
>  }
>
>  static bool
> +OVS_NO_SANITIZE_FUNCTION
>  ovsrcu_call_postponed(void)
>  {
>  struct ovsrcu_cbset *cbset;
> --
> 2.39.3
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 0/7] Bump of CI Ubuntu and Fedora versions

[Numan Siddique]

On Thu, May 16, 2024 at 12:01 PM Ales Musil  wrote:
>
> On Thu, May 16, 2024 at 5:57 PM Numan Siddique  wrote:
>
> > On Tue, May 14, 2024 at 4:41 AM Ales Musil  wrote:
> > >
> > > The series is pretty small however it is required for the
> > > bump to Ubuntu 24.04 and Fedora 40. Both have newer GCC and
> > > Clang which brought up some issues that needed to be fixed.
> > >
> > > The series also includes fix for weekly runs to use Fedora
> > > because the cache string wasn't specific enough.
> > >
> > > Ales Musil (7):
> > >   ci: Pin Fedora version for the build-rpm job.
> > >   ovs: Bump the submodule to the tip of branch-3.3.
> > >   ci: Update the Ubuntu container to 24.04.
> > >   tests: Replace wget with curl for failing commands.
> > >   ci: Add missing packages to run Fedora image in GH CI.
> > >   ci: Make sure that we are using proper image.
> > >   ci: Bump the Fedora container to 40.
> >
> > Thanks for the patch series.
> >
> > I applied this patch series to the main.  Before applying I removed
> > "dnf update" from .ci/linux-build.sh  as per
> > the comments in the patch 5.
> >
>
>
> Thank you, would you mind backporting only the first patch to 24.03?

Sure.

Numan

>
> >
> > Thanks
> > Numan
> >
> > >
> > >  .ci/linux-build.sh |  3 +++
> > >  .github/workflows/test.yml |  7 ---
> > >  ovs|  2 +-
> > >  tests/system-ovn.at| 15 +++
> > >  utilities/containers/fedora/Dockerfile |  3 ++-
> > >  utilities/containers/ubuntu/Dockerfile | 11 ++-
> > >  6 files changed, 27 insertions(+), 14 deletions(-)
> > >
> > > --
> > > 2.44.0
> > >
> > > ___
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> >
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA 
>
> amu...@redhat.com
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v12 1/6] Add global option for JSON output to ovs-appctl.

[0-day Robot]

Bleep bloop.  Greetings Jakob Meng, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#678 FILE: utilities/ovs-appctl.c:152:
  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\

Lines checked: 793, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 0/7] Bump of CI Ubuntu and Fedora versions

[Ales Musil]

On Thu, May 16, 2024 at 5:57 PM Numan Siddique  wrote:

> On Tue, May 14, 2024 at 4:41 AM Ales Musil  wrote:
> >
> > The series is pretty small however it is required for the
> > bump to Ubuntu 24.04 and Fedora 40. Both have newer GCC and
> > Clang which brought up some issues that needed to be fixed.
> >
> > The series also includes fix for weekly runs to use Fedora
> > because the cache string wasn't specific enough.
> >
> > Ales Musil (7):
> >   ci: Pin Fedora version for the build-rpm job.
> >   ovs: Bump the submodule to the tip of branch-3.3.
> >   ci: Update the Ubuntu container to 24.04.
> >   tests: Replace wget with curl for failing commands.
> >   ci: Add missing packages to run Fedora image in GH CI.
> >   ci: Make sure that we are using proper image.
> >   ci: Bump the Fedora container to 40.
>
> Thanks for the patch series.
>
> I applied this patch series to the main.  Before applying I removed
> "dnf update" from .ci/linux-build.sh  as per
> the comments in the patch 5.
>


Thank you, would you mind backporting only the first patch to 24.03?

>
> Thanks
> Numan
>
> >
> >  .ci/linux-build.sh |  3 +++
> >  .github/workflows/test.yml |  7 ---
> >  ovs|  2 +-
> >  tests/system-ovn.at| 15 +++
> >  utilities/containers/fedora/Dockerfile |  3 ++-
> >  utilities/containers/ubuntu/Dockerfile | 11 ++-
> >  6 files changed, 27 insertions(+), 14 deletions(-)
> >
> > --
> > 2.44.0
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
Thanks,
Ales
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 5/7] ci: Add missing packages to run Fedora image in GH CI.

[Numan Siddique]

On Tue, May 14, 2024 at 9:46 AM Ales Musil  wrote:
>
> On Tue, May 14, 2024 at 3:19 PM Ilya Maximets  wrote:
>
> > On 5/14/24 10:38, Ales Musil wrote:
> > > There were two things missing for the Fedora builds, 32-bit
> > > version of glibc to allows the -m32 compilation on Fedora
> > > and numactl-devel package.
> > >
> > > Signed-off-by: Ales Musil 
> > > ---
> > >  .ci/linux-build.sh | 3 +++
> > >  utilities/containers/fedora/Dockerfile | 1 +
> > >  2 files changed, 4 insertions(+)
> > >
> > > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> > > index 78f17f8bd..12966f532 100755
> > > --- a/.ci/linux-build.sh
> > > +++ b/.ci/linux-build.sh
> > > @@ -83,6 +83,9 @@ function configure_gcc()
> > >  # do it directly because gcc-multilib is not available
> > >  # for arm64
> > >  sudo apt update && sudo apt install -y gcc-multilib
> > > +elif which dnf; then
> > > +# Install equivalent of gcc-multilib for Fedora.
> > > +sudo dnf -y update && sudo dnf -y install glibc-devel.i686
> >
> > dnf always refreshes package cache.  'dnf update' will actually update
> > all the packages to the latest versions.  I'm not sure it is an intended
> > behavior here.
> >
>
> Yeah good point, we shouldn't update the packages just install the missing
> one. So only the install part is needed, I'll wait for other reviews before
> posting v2.

I removed "dnf -u update" and applied this patch series to main.
I thought of applying instead of spinning v2 just for this change.

Numan

>
>
> >
> > >  fi
> > >  fi
> > >  }
> > > diff --git a/utilities/containers/fedora/Dockerfile
> > b/utilities/containers/fedora/Dockerfile
> > > index 9b8386aae..d40a7b31f 100755
> > > --- a/utilities/containers/fedora/Dockerfile
> > > +++ b/utilities/containers/fedora/Dockerfile
> > > @@ -28,6 +28,7 @@ RUN dnf -y update \
> > >  libtool \
> > >  net-tools \
> > >  nmap-ncat \
> > > +numactl-devel \
> > >  openssl \
> > >  openssl-devel \
> > >  procps-ng \
> >
> >
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA 
>
> amu...@redhat.com
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 0/7] Bump of CI Ubuntu and Fedora versions

[Numan Siddique]

On Tue, May 14, 2024 at 4:41 AM Ales Musil  wrote:
>
> The series is pretty small however it is required for the
> bump to Ubuntu 24.04 and Fedora 40. Both have newer GCC and
> Clang which brought up some issues that needed to be fixed.
>
> The series also includes fix for weekly runs to use Fedora
> because the cache string wasn't specific enough.
>
> Ales Musil (7):
>   ci: Pin Fedora version for the build-rpm job.
>   ovs: Bump the submodule to the tip of branch-3.3.
>   ci: Update the Ubuntu container to 24.04.
>   tests: Replace wget with curl for failing commands.
>   ci: Add missing packages to run Fedora image in GH CI.
>   ci: Make sure that we are using proper image.
>   ci: Bump the Fedora container to 40.

Thanks for the patch series.

I applied this patch series to the main.  Before applying I removed
"dnf update" from .ci/linux-build.sh  as per
the comments in the patch 5.

Thanks
Numan

>
>  .ci/linux-build.sh |  3 +++
>  .github/workflows/test.yml |  7 ---
>  ovs|  2 +-
>  tests/system-ovn.at| 15 +++
>  utilities/containers/fedora/Dockerfile |  3 ++-
>  utilities/containers/ubuntu/Dockerfile | 11 ++-
>  6 files changed, 27 insertions(+), 14 deletions(-)
>
> --
> 2.44.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] controller: Avoid use after free in LB I-P.

[Numan Siddique]

On Mon, May 6, 2024 at 6:37 AM Ales Musil  wrote:
>
> Avoid use after free in scenario when controller received LB deletion
> after the DB was reconnected. The reconnect led to idl clearing up
> the "old" structs, one of them being the LB. However, during recompute
> the struct was referenced when it was already gone.
>
> Clear the whole objdep_mgr instead of going one-by-one during recompute.
>
> ==143949==ERROR: AddressSanitizer: heap-use-after-free
> READ of size 4 at 0x513280d0 thread T0
> 0 0x61c3c9 in lb_data_local_lb_remove controller/ovn-controller.c:2978:5
> 1 0x5fd4df in en_lb_data_run controller/ovn-controller.c:3063:9
> 2 0x6fe0d9 in engine_recompute lib/inc-proc-eng.c:415:5
> 3 0x6fbdc2 in engine_run_node lib/inc-proc-eng.c:477:9
> 4 0x6fbdc2 in engine_run lib/inc-proc-eng.c:528:9
> 5 0x5f39a0 in main controller/ovn-controller.c
>
> Fixes: 8382127186bf ("controller: Store load balancer data in separate node")
> Reported-at: https://issues.redhat.com/browse/FDP-610
> Signed-off-by: Ales Musil 

Thanks.  Applied the patch to main and backported until 23.03

Numan

> ---
>  controller/ovn-controller.c | 20 +--
>  tests/ovn-controller.at | 38 +
>  2 files changed, 48 insertions(+), 10 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 23269af83..65b9ba8e5 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2972,7 +2972,7 @@ lb_data_local_lb_add(struct ed_type_lb_data *lb_data,
>
>  static void
>  lb_data_local_lb_remove(struct ed_type_lb_data *lb_data,
> -struct ovn_controller_lb *lb, bool tracked)
> +struct ovn_controller_lb *lb)
>  {
>  const struct uuid *uuid = >slb->header_.uuid;
>
> @@ -2981,12 +2981,8 @@ lb_data_local_lb_remove(struct ed_type_lb_data 
> *lb_data,
>
>  lb_data_removed_five_tuples_add(lb_data, lb);
>
> -if (tracked) {
> -hmap_insert(_data->old_lbs, >hmap_node, uuid_hash(uuid));
> -uuidset_insert(_data->deleted, uuid);
> -} else {
> -ovn_controller_lb_destroy(lb);
> -}
> +hmap_insert(_data->old_lbs, >hmap_node, uuid_hash(uuid));
> +uuidset_insert(_data->deleted, uuid);
>  }
>
>  static bool
> @@ -3011,7 +3007,7 @@ lb_data_handle_changed_ref(enum objdep_type type, const 
> char *res_name,
>  continue;
>  }
>
> -lb_data_local_lb_remove(lb_data, lb, true);
> +lb_data_local_lb_remove(lb_data, lb);
>
>  const struct sbrec_load_balancer *sbrec_lb =
>  sbrec_load_balancer_table_get_for_uuid(ctx_in->lb_table, uuid);
> @@ -3057,9 +3053,13 @@ en_lb_data_run(struct engine_node *node, void *data)
>  const struct sbrec_load_balancer_table *lb_table =
>  EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
>
> +objdep_mgr_clear(_data->deps_mgr);
> +
>  struct ovn_controller_lb *lb;
>  HMAP_FOR_EACH_SAFE (lb, hmap_node, _data->local_lbs) {
> -lb_data_local_lb_remove(lb_data, lb, false);
> +hmap_remove(_data->local_lbs, >hmap_node);
> +lb_data_removed_five_tuples_add(lb_data, lb);
> +ovn_controller_lb_destroy(lb);
>  }
>
>  const struct sbrec_load_balancer *sbrec_lb;
> @@ -3097,7 +3097,7 @@ lb_data_sb_load_balancer_handler(struct engine_node 
> *node, void *data)
>  continue;
>  }
>
> -lb_data_local_lb_remove(lb_data, lb, true);
> +lb_data_local_lb_remove(lb_data, lb);
>  }
>
>  if (sbrec_load_balancer_is_deleted(sbrec_lb) ||
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 27cec2aec..cecbc190b 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2973,3 +2973,41 @@ 
> priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4 
> actions=load:0x1->OXM_OF
>
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
> +
> +AT_SETUP([ovn-controller - LB remove after disconnect])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +check ovs-vsctl -- add-port br-int vif1 -- \
> +set interface vif1 external-ids:iface-id=lsp
> +
> +check ovs-vsctl set Open_vSwitch . 
> external-ids:ovn-remote-probe-interval="5000"
> +
> +check ovn-nbctl ls-add ls
> +check ovn-nbctl lsp-add ls lsp \
> +-- lsp-set-addresses lsp "f0:00:00:00:00:01 172.16.0.10"
> +
> +check ovn-nbctl lb-add lb 192.168.100.100 172.16.0.10
> +check ovn-nbctl ls-lb-add ls lb
> +
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +sleep_sb
> +OVS_WAIT_UNTIL([grep -q 'OVNSB commit failed' hv1/ovn-controller.log])
> +
> +sleep_controller hv1
> +wake_up_sb
> +
> +ovn-nbctl lb-del lb
> +
> +wake_up_controller hv1
> +check ovn-nbctl --wait=hv sync
> +
> +OVN_CLEANUP([hv1
> +/no response to inactivity probe after .* seconds, disconnecting/d])
> +AT_CLEANUP
> --
> 2.44.0
>
> 

[ovs-dev] [PATCH v12 6/6] ofproto: Add JSON output for 'dpif/show' command.

[jmeng]

From: Jakob Meng 

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 
---
 NEWS   |   1 +
 ofproto/ofproto-dpif.c | 120 +
 tests/pmd.at   |  23 
 3 files changed, 133 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 479310d49..c434b1497 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,7 @@ Post-v3.3.0
or 'text' (by default).
  * Added new option [--pretty] to print JSON output in a readable fashion.
  * 'list-commands' now supports output in JSON format.
+ * 'dpif/show' now supports output in JSON format.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 32d037be6..db0405886 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -28,6 +28,7 @@
 #include "fail-open.h"
 #include "guarded-list.h"
 #include "hmapx.h"
+#include "json.h"
 #include "lacp.h"
 #include "learn.h"
 #include "mac-learning.h"
@@ -6519,8 +6520,99 @@ done:
 return changed;
 }
 
+static struct json *
+dpif_show_backer_json(struct json *backers, const struct dpif_backer *backer)
+{
+struct json *json_backer = json_object_create();
+
+/* Add datapath as new JSON object using its name as key. */
+json_object_put(backers, dpif_name(backer->dpif), json_backer);
+
+/* Add datapath's stats under "stats" key. */
+struct dpif_dp_stats dp_stats;
+struct json *json_dp_stats = json_object_create();
+
+json_object_put(json_backer, "stats", json_dp_stats);
+dpif_get_dp_stats(backer->dpif, _stats);
+json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);
+json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
+   dp_stats.n_missed);
+
+/* Add datapath's bridges under "bridges" key. */
+struct json *json_dp_bridges = json_object_create();
+json_object_put(json_backer, "bridges", json_dp_bridges);
+
+struct shash ofproto_shash;
+shash_init(_shash);
+const struct shash_node **ofprotos = get_ofprotos(_shash);
+for (size_t i = 0; i < shash_count(_shash); i++) {
+struct ofproto_dpif *ofproto = ofprotos[i]->data;
+
+if (ofproto->backer != backer) {
+continue;
+}
+
+struct json *json_ofproto = json_object_create();
+/* Add bridge to "bridges" dictionary using its name as key. */
+json_object_put(json_dp_bridges, ofproto->up.name, json_ofproto);
+
+/* Add bridge ports to the current bridge dictionary. */
+const struct shash_node **ports;
+ports = shash_sort(>up.port_by_name);
+for (size_t j = 0; j < shash_count(>up.port_by_name); j++) {
+const struct shash_node *port = ports[j];
+struct ofport *ofport = port->data;
+
+struct json * json_ofproto_port = json_object_create();
+/* Add bridge port to a bridge's dict using port name as key. */
+json_object_put(json_ofproto, netdev_get_name(ofport->netdev),
+json_ofproto_port);
+/* Add OpenFlow port associated with a bridge port. */
+json_object_put_format(json_ofproto_port, "ofport", "%u",
+   ofport->ofp_port);
+
+/* Add bridge port number. */
+odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
+   ofport->ofp_port);
+if (odp_port != ODPP_NONE) {
+json_object_put_format(json_ofproto_port, "port_no",
+   "%"PRIu32, odp_port);
+} else {
+json_object_put_string(json_ofproto_port, "port_no", "none");
+}
+
+/* Add type of a bridge port. */
+json_object_put_string(json_ofproto_port, "type",
+   netdev_get_type(ofport->netdev));
+
+/* Add config entries for a bridge port. */
+struct json *json_port_config = json_object_create();
+struct smap config;
+smap_init();
+
+json_object_put(json_ofproto_port, "config", json_port_config);
+if (!netdev_get_config(ofport->netdev, )) {
+struct smap_node *node;
+
+SMAP_FOR_EACH (node, ) {
+json_object_put_string(json_port_config, node->key,
+   node->value);
+}
+}
+smap_destroy();
+
+} /* End of bridge port(s). */
+
+free(ports);
+} /* End of bridge(s). */
+shash_destroy(_shash);
+free(ofprotos);
+
+return json_backer;
+}
+
 

[ovs-dev] [PATCH v12 4/6] python: Add option for pretty-printing JSON output to appctl.py.

[jmeng]

From: Jakob Meng 

With the '--pretty' option, appctl.py will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys. The pretty-printed output from appctl.py is not
strictly the same as with ovs-appctl because of both use different
pretty-printing implementations.

Signed-off-by: Jakob Meng 
---
 tests/appctl.py | 13 ++---
 tests/unixctl-py.at |  5 +
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/tests/appctl.py b/tests/appctl.py
index cf3ea3642..b08bf9033 100644
--- a/tests/appctl.py
+++ b/tests/appctl.py
@@ -37,7 +37,7 @@ def connect_to_target(target):
 return client
 
 
-def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT):
+def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT, fmt_flags={}):
 if fmt == ovs.util.OutputFormat.TEXT:
 body = str(reply)
 
@@ -46,7 +46,7 @@ def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT):
 
 return body
 else:
-return ovs.json.to_string(reply)
+return ovs.json.to_string(reply, **fmt_flags)
 
 
 def main():
@@ -65,13 +65,20 @@ def main():
 help="Output format.", default="text",
 choices=[fmt.name.lower()
  for fmt in ovs.util.OutputFormat])
+parser.add_argument("--pretty", action="store_true",
+help="Format the output in a more readable fashion."
+ " Requires: --format json.")
 args = parser.parse_args()
 
+if args.format != ovs.util.OutputFormat.JSON.name.lower() and args.pretty:
+ovs.util.ovs_fatal(0, "--pretty is supported with --format json only")
+
 signal_alarm(int(args.timeout) if args.timeout else None)
 
 ovs.vlog.Vlog.init()
 target = args.target
 format = ovs.util.OutputFormat[args.format.upper()]
+format_flags = dict(pretty=True) if args.pretty else {}
 client = connect_to_target(target)
 
 if format != ovs.util.OutputFormat.TEXT:
@@ -96,7 +103,7 @@ def main():
 sys.exit(2)
 else:
 assert result is not None
-sys.stdout.write(reply_to_string(result, format))
+sys.stdout.write(reply_to_string(result, format, format_flags))
 
 
 if __name__ == '__main__':
diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
index 92f557b67..dffe40d2b 100644
--- a/tests/unixctl-py.at
+++ b/tests/unixctl-py.at
@@ -115,6 +115,11 @@ AT_CHECK([APPCTL -t test-unixctl.py version], [0], 
[expout])
 AT_CHECK([PYAPPCTL_PY -t test-unixctl.py version], [0], [expout])
 AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json version], [0], 
[dnl
 {"reply":"$(cat expout)","reply-format":"plain"}])
+AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json --pretty 
version], [0], [dnl
+{
+  "reply":"$(cat expout)",
+  "reply-format":"plain"
+}])
 
 AT_CHECK([APPCTL -t test-unixctl.py echo robot ninja], [0], [stdout])
 AT_CHECK([cat stdout | sed -e "s/u'/'/g"], [0], [dnl
-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v12 5/6] vswitchd: Add JSON output for 'list-commands' command.

[jmeng]

From: Jakob Meng 

The 'list-commands' command now supports machine-readable JSON output
in addition to the plain-text output for humans.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS  |  1 +
 lib/unixctl.c | 44 +--
 tests/ovs-vswitchd.at | 13 +
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/NEWS b/NEWS
index 0d41061d3..479310d49 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,7 @@ Post-v3.3.0
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
  * Added new option [--pretty] to print JSON output in a readable fashion.
+ * 'list-commands' now supports output in JSON format.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/lib/unixctl.c b/lib/unixctl.c
index c430eac0b..c84ca125f 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -107,24 +107,40 @@ static void
 unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
   const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
-struct ds ds = DS_EMPTY_INITIALIZER;
-const struct shash_node **nodes = shash_sort();
-size_t i;
+if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
+struct json *json_commands = json_object_create();
+const struct shash_node *node;
 
-ds_put_cstr(, "The available commands are:\n");
+SHASH_FOR_EACH (node, ) {
+const struct unixctl_command *command = node->data;
 
-for (i = 0; i < shash_count(); i++) {
-const struct shash_node *node = nodes[i];
-const struct unixctl_command *command = node->data;
-
-if (command->usage) {
-ds_put_format(, "  %-23s %s\n", node->name, command->usage);
+if (command->usage) {
+json_object_put_string(json_commands, node->name,
+   command->usage);
+}
 }
-}
-free(nodes);
+unixctl_command_reply_json(conn, json_commands);
+} else {
+struct ds ds = DS_EMPTY_INITIALIZER;
+const struct shash_node **nodes = shash_sort();
+size_t i;
 
-unixctl_command_reply(conn, ds_cstr());
-ds_destroy();
+ds_put_cstr(, "The available commands are:\n");
+
+for (i = 0; i < shash_count(); ++i) {
+const struct shash_node *node = nodes[i];
+const struct unixctl_command *command = node->data;
+
+if (command->usage) {
+ds_put_format(, "  %-23s %s\n", node->name,
+  command->usage);
+}
+}
+free(nodes);
+
+unixctl_command_reply(conn, ds_cstr());
+ds_destroy();
+}
 }
 
 static void
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 2db8138f1..4f909603a 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -281,3 +281,16 @@ AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty 
version], [0], [dnl
   "reply-format": "plain"}])
 
 AT_CLEANUP
+
+AT_SETUP([ovs-vswitchd list-commands])
+OVS_VSWITCHD_START
+
+AT_CHECK([ovs-appctl list-commands], [0], [ignore])
+AT_CHECK([ovs-appctl --format json list-commands], [0], [stdout])
+# Check that ovs-appctl prints a single line without a trailing newline.
+AT_CHECK([wc -l stdout], [0], [0 stdout
+])
+# Check that ovs-appctl prints text which roughly looks like a JSON object.
+AT_CHECK([grep -E '^\{"autoattach/show-isid":.*\}$' stdout], [0], [ignore])
+
+AT_CLEANUP
-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v12 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[jmeng]

From: Jakob Meng 

With the '--pretty' option, ovs-appctl will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys.

Signed-off-by: Jakob Meng 
---
 Documentation/ref/ovs-appctl.8.rst |  7 ++
 NEWS   |  1 +
 tests/ovs-vswitchd.at  |  5 +
 utilities/ovs-appctl.c | 34 --
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/Documentation/ref/ovs-appctl.8.rst 
b/Documentation/ref/ovs-appctl.8.rst
index 9619c1226..db6c52b42 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -79,6 +79,13 @@ In normal use only a single option is accepted:
 
   ``{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}``
 
+* ``--pretty``
+
+  By default, JSON output is printed as compactly as possible. This option
+  causes JSON in output to be printed in a more readable fashion. For example,
+  members of objects and elements of arrays are printed one per line, with
+  indentation. Requires ``--format=json``.
+
 Common Commands
 ===
 
diff --git a/NEWS b/NEWS
index 7076939c5..0d41061d3 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,7 @@ Post-v3.3.0
- ovs-appctl:
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+ * Added new option [--pretty] to print JSON output in a readable fashion.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 1ae7fcc32..2db8138f1 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -275,4 +275,9 @@ ovs_version=$(ovs-appctl version)
 AT_CHECK_UNQUOTED([ovs-appctl --format json version], [0], [dnl
 {"reply":"$ovs_version","reply-format":"plain"}])
 
+AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty version], [0], [dnl
+{
+  "reply": "$ovs_version",
+  "reply-format": "plain"}])
+
 AT_CLEANUP
diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
index 0e99259e8..3b76740ea 100644
--- a/utilities/ovs-appctl.c
+++ b/utilities/ovs-appctl.c
@@ -40,13 +40,15 @@ static void usage(void);
 /* Parsed command line args. */
 struct cmdl_args {
 enum unixctl_output_fmt format;
+unsigned int format_flags;
 char *target;
 };
 
 static struct cmdl_args *cmdl_args_create(void);
 static struct cmdl_args *parse_command_line(int argc, char *argv[]);
 static struct jsonrpc *connect_to_target(const char *target);
-static char * reply_to_string(struct json *reply, enum unixctl_output_fmt fmt);
+static char * reply_to_string(struct json *reply, enum unixctl_output_fmt fmt,
+  unsigned int fmt_flags);
 
 int
 main(int argc, char *argv[])
@@ -84,7 +86,7 @@ main(int argc, char *argv[])
 
 if (cmd_error) {
 jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
 free(msg);
 ovs_error(0, "%s: server returned an error", args->target);
@@ -108,13 +110,13 @@ main(int argc, char *argv[])
 
 if (cmd_error) {
 jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
 free(msg);
 ovs_error(0, "%s: server returned an error", args->target);
 exit(2);
 } else if (cmd_result) {
-msg = reply_to_string(cmd_result, args->format);
+msg = reply_to_string(cmd_result, args->format, args->format_flags);
 fputs(msg, stdout);
 free(msg);
 } else {
@@ -151,6 +153,8 @@ Other options:\n\
   --timeout=SECS wait at most SECS seconds for a response\n\
   -f, --format=FMT   Output format. One of: 'json', or 'text'\n\
  (default: text)\n\
+  --pretty   Format the output in a more readable fashion.\n\
+ Requires: --format=json.\n\
   -h, --help Print this helpful information\n\
   -V, --version  Display ovs-appctl version information\n",
program_name, program_name);
@@ -163,6 +167,7 @@ cmdl_args_create(void)
 struct cmdl_args *args = xmalloc(sizeof *args);
 
 args->format = UNIXCTL_OUTPUT_FMT_TEXT;
+args->format_flags = 0;
 args->target = NULL;
 
 return args;
@@ -173,7 +178,8 @@ parse_command_line(int argc, char *argv[])
 {
 enum {
 OPT_START = UCHAR_MAX + 1,
-VLOG_OPTION_ENUMS
+OPT_PRETTY,
+VLOG_OPTION_ENUMS,
 };
 static const struct option long_options[] = {
 {"target", required_argument, NULL, 't'},
@@ -181,6 +187,7 @@ parse_command_line(int argc, char *argv[])
 {"format", required_argument, NULL, 'f'},
 {"help", no_argument, 

[ovs-dev] [PATCH v12 1/6] Add global option for JSON output to ovs-appctl.

[jmeng]

From: Jakob Meng 

For monitoring systems such as Prometheus it would be beneficial if
OVS would expose statistics in a machine-readable format.

This patch introduces support for different output formats to
ovs-appctl. It gains a global option '-f,--format' which changes it to
print a JSON document instead of plain-text for humans. For example, a
later patch implements support for
'ovs-appctl --format json dpif/show'. By default, the output format
is plain-text as before.

A new 'set-options' command has been added to lib/unixctl.c which
allows to change the output format of the commands executed afterwards
on the same socket connection. It is supposed to be run by ovs-appctl
transparently for the user when a specific output format has been
requested.
For example, when a user calls 'ovs-appctl --format json dpif/show',
then ovs-appctl will call 'set-options' to set the output format as
requested by the user and afterwards it will call the actual command
'dpif/show'.
This ovs-appctl behaviour has been implemented in a backward compatible
way. One can use an updated client (ovs-appctl) with an old server
(ovs-vswitchd) and vice versa. Of course, JSON output only works when
both sides have been updated.

Two access functions unixctl_command_{get,set}_output_format() and a
unixctl_command_reply_json function have been added to lib/unixctl.h:
unixctl_command_get_output_format() is supposed to be used in commands
like 'dpif/show' to query the requested output format. When JSON output
has been selected, the unixctl_command_reply_json() function can be
used to return JSON objects to the client (ovs-appctl) instead of
plain-text with the unixctl_command_reply{,_error}() functions.

When JSON has been requested but a command has not implemented JSON
output the plain-text output will be wrapped in a provisional JSON
document with the following structure:

  {"reply":"$PLAIN_TEXT_HERE","reply-format":"plain"}

Thus commands which have been executed successfully will not fail when
they try to render the output at a later stage.

A test for the 'version' command has been implemented which shows how
the provisional JSON document looks like in practice. For a cleaner
JSON document, the trailing newline has been moved from the program
version string to function ovs_print_version(). This way, the
plain-text output of the 'version' command has not changed.

Output formatting has been moved from unixctl_client_transact() in
lib/unixctl.c to utilities/ovs-appctl.c. The former merely returns the
JSON objects returned from the server and the latter is now responsible
for printing it properly.

In popular tools like kubectl the option for output control is usually
called '-o|--output' instead of '-f,--format'. But ovs-appctl already
has an short option '-o' which prints the available ovs-appctl options
('--option'). The now choosen name also better alignes with
ovsdb-client where '-f,--format' controls output formatting.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 Documentation/ref/ovs-appctl.8.rst |  12 ++
 NEWS   |   3 +
 lib/unixctl.c  | 195 ++---
 lib/unixctl.h  |  20 ++-
 lib/util.c |   6 +-
 python/ovs/unixctl/server.py   |   3 -
 tests/appctl.py|   5 +
 tests/ovs-vswitchd.at  |  11 ++
 utilities/ovs-appctl.c | 132 ---
 9 files changed, 316 insertions(+), 71 deletions(-)

diff --git a/Documentation/ref/ovs-appctl.8.rst 
b/Documentation/ref/ovs-appctl.8.rst
index 3ce02e984..9619c1226 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -8,6 +8,7 @@ Synopsis
 ``ovs-appctl``
 [``--target=`` | ``-t`` ]
 [``--timeout=`` | ``-T`` ]
+[``--format=`` | ``-f`` ]
  [...]
 
 ``ovs-appctl --help``
@@ -67,6 +68,17 @@ In normal use only a single option is accepted:
   runtime to approximately  seconds.  If the timeout expires,
   ``ovs-appctl`` exits with a ``SIGALRM`` signal.
 
+* ``-f `` or ``--format=``
+
+  Tells ``ovs-appctl`` which output format to use. By default, or with a
+   of ``text``, ``ovs-appctl`` will print plain-text for humans.
+  When  is ``json``, ``ovs-appctl`` will return a JSON document.
+  When ``json`` is requested, but a command has not implemented JSON
+  output, the plain-text output will be wrapped in a provisional JSON
+  document with the following structure:
+
+  ``{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}``
+
 Common Commands
 ===
 
diff --git a/NEWS b/NEWS
index b92cec532..3c52e5ec1 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
 Post-v3.3.0
 
+   - ovs-appctl:
+ * Added new option [-f|--format] to choose the output format, e.g. 'json'
+   or 'text' (by default).
- Userspace datapath:
  * Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' 

[ovs-dev] [PATCH v12 0/6] Add global option to output JSON from ovs-appctl cmds.

[jmeng]

From: Jakob Meng 

v12 has one tiny change compared to v11 [0]:

diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
Signed-off-by: Jakob Meng 
index 2ade64336..3b76740ea 100644
--- a/utilities/ovs-appctl.c
+++ b/utilities/ovs-appctl.c
@@ -88,6 +88,7 @@ main(int argc, char *argv[])
 jsonrpc_close(client);
 msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
+free(msg);
 ovs_error(0, "%s: server returned an error", args->target);
 exit(2);
 }
@@ -111,11 +112,13 @@ main(int argc, char *argv[])
 jsonrpc_close(client);
 msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
+free(msg);
 ovs_error(0, "%s: server returned an error", args->target);
 exit(2);
 } else if (cmd_result) {
 msg = reply_to_string(cmd_result, args->format, args->format_flags);
 fputs(msg, stdout);
+free(msg);
 } else {
 OVS_NOT_REACHED();
 }
@@ -124,7 +127,6 @@ main(int argc, char *argv[])
 json_destroy(cmd_result);
 json_destroy(cmd_error);
 free(args);
-free(msg);
 return 0;
 }

It moves "free(msg);" up in order to work around an issue with 
AddressSanitizer/LeakSanitizer [1] in Ubuntu 22.04 which is not present in 
later releases.

[0] 
https://patchwork.ozlabs.org/project/openvswitch/list/?series=407037=both=*
[1] https://github.com/ovsrobot/ovs/actions/runs/9110821443/job/25046630474

Best regards,
Jakob


Jakob Meng (6):
  Add global option for JSON output to ovs-appctl.
  python: Add option for JSON output to unixctl classes and appctl.py.
  appctl: Add option '--pretty' for pretty-printing JSON output.
  python: Add option for pretty-printing JSON output to appctl.py.
  vswitchd: Add JSON output for 'list-commands' command.
  ofproto: Add JSON output for 'dpif/show' command.

 Documentation/ref/ovs-appctl.8.rst |  19 +++
 NEWS   |   9 ++
 lib/unixctl.c  | 239 ++---
 lib/unixctl.h  |  20 ++-
 lib/util.c |   6 +-
 ofproto/ofproto-dpif.c | 120 +--
 python/ovs/unixctl/client.py   |   5 +-
 python/ovs/unixctl/server.py   |  55 +--
 python/ovs/util.py |   8 +
 tests/appctl.py|  40 -
 tests/ovs-vswitchd.at  |  29 
 tests/pmd.at   |  23 +++
 tests/unixctl-py.at|   8 +
 utilities/ovs-appctl.c | 154 ---
 14 files changed, 627 insertions(+), 108 deletions(-)

-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v12 2/6] python: Add option for JSON output to unixctl classes and appctl.py.

[jmeng]

From: Jakob Meng 

This patch introduces support for different output formats to Python
Unixctl* classes and appctl.py, similar to what the previous commit did
for ovs-appctl.
In particular, tests/appctl.py gains a global option '-f,--format'
which allows users to request JSON instead of plain-text for humans.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS |  3 +++
 python/ovs/unixctl/client.py |  5 ++--
 python/ovs/unixctl/server.py | 52 +++-
 python/ovs/util.py   |  8 ++
 tests/appctl.py  | 38 +-
 tests/unixctl-py.at  |  3 +++
 6 files changed, 93 insertions(+), 16 deletions(-)

diff --git a/NEWS b/NEWS
index 3c52e5ec1..7076939c5 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,9 @@ Post-v3.3.0
- ovs-appctl:
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+   - Python:
+ * Added support for different output formats like 'json' to appctl.py and
+   Python's unixctl classes.
- Userspace datapath:
  * Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 8283f99bb..8a6fcb1b9 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -14,6 +14,7 @@
 
 import os
 
+import ovs.json
 import ovs.jsonrpc
 import ovs.stream
 import ovs.util
@@ -41,10 +42,10 @@ class UnixctlClient(object):
 return error, None, None
 
 if reply.error is not None:
-return 0, str(reply.error), None
+return 0, reply.error, None
 else:
 assert reply.result is not None
-return 0, None, str(reply.result)
+return 0, None, reply.result
 
 def close(self):
 self._conn.close()
diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index d24a7092c..0665eb837 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import argparse
 import copy
 import errno
 import os
@@ -35,6 +36,7 @@ class UnixctlConnection(object):
 assert isinstance(rpc, ovs.jsonrpc.Connection)
 self._rpc = rpc
 self._request_id = None
+self._fmt = ovs.util.OutputFormat.TEXT
 
 def run(self):
 self._rpc.run()
@@ -63,10 +65,29 @@ class UnixctlConnection(object):
 return error
 
 def reply(self, body):
-self._reply_impl(True, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+if self._fmt == ovs.util.OutputFormat.JSON:
+body = {
+"reply-format": "plain",
+"reply": body
+}
+
+return self._reply_impl_json(True, body)
+
+def reply_json(self, body):
+self._reply_impl_json(True, body)
 
 def reply_error(self, body):
-self._reply_impl(False, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+return self._reply_impl_json(False, body)
 
 # Called only by unixctl classes.
 def _close(self):
@@ -78,15 +99,11 @@ class UnixctlConnection(object):
 if not self._rpc.get_backlog():
 self._rpc.recv_wait(poller)
 
-def _reply_impl(self, success, body):
+def _reply_impl_json(self, success, body):
 assert isinstance(success, bool)
-assert body is None or isinstance(body, str)
 
 assert self._request_id is not None
 
-if body is None:
-body = ""
-
 if success:
 reply = Message.create_reply(body, self._request_id)
 else:
@@ -133,6 +150,24 @@ def _unixctl_version(conn, unused_argv, version):
 conn.reply(version)
 
 
+def _unixctl_set_options(conn, argv, unused_aux):
+assert isinstance(conn, UnixctlConnection)
+
+parser = argparse.ArgumentParser()
+parser.add_argument("--format", default="text",
+choices=[fmt.name.lower()
+ for fmt in ovs.util.OutputFormat])
+
+try:
+args = parser.parse_args(args=argv)
+except argparse.ArgumentError as e:
+conn.reply_error(str(e))
+return
+
+conn._fmt = ovs.util.OutputFormat[args.format.upper()]
+conn.reply(None)
+
+
 class UnixctlServer(object):
 def __init__(self, listener):
 assert isinstance(listener, ovs.stream.PassiveStream)
@@ -207,4 +242,7 @@ class UnixctlServer(object):
 ovs.unixctl.command_register("version", "", 0, 0, _unixctl_version,
  version)
 
+ovs.unixctl.command_register("set-options", "[--format text|json]", 0,

[ovs-dev] [PATCH v2 2/2] ipf: Handle common case of ipf defragmentation.

[Mike Pattrick]

When conntrack is reassembling packet fragments, the same reassembly
context can be shared across multiple threads handling different packets
simultaneously. Once a full packet is assembled, it is added to a packet
batch for processing, in the case where there are multiple different pmd
threads accessing conntrack simultaneously, there is a race condition
where the reassembled packet may be added to an arbitrary batch even if
the current batch is available.

When this happens, the packet may be handled incorrectly as it is
inserted into a random openflow execution pipeline, instead of the
pipeline for that packets flow.

This change makes a best effort attempt to try to add the defragmented
packet to the current batch. directly. This should succeed most of the
time.

Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
Reported-at: https://issues.redhat.com/browse/FDP-560
Signed-off-by: Mike Pattrick 
---
 lib/ipf.c | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/lib/ipf.c b/lib/ipf.c
index 3c8960be3..2d715f5e9 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -506,13 +506,15 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list)
 }
 
 /* Called when a frag list state transitions to another state. This is
- * triggered by new fragment for the list being received.*/
-static void
+* triggered by new fragment for the list being received. Returns a reassembled
+* packet if this fragment has completed one. */
+static struct reassembled_pkt *
 ipf_list_state_transition(struct ipf *ipf, struct ipf_list *ipf_list,
   bool ff, bool lf, bool v6)
 OVS_REQUIRES(ipf->ipf_lock)
 {
 enum ipf_list_state curr_state = ipf_list->state;
+struct reassembled_pkt *ret = NULL;
 enum ipf_list_state next_state;
 switch (curr_state) {
 case IPF_LIST_STATE_UNUSED:
@@ -562,12 +564,15 @@ ipf_list_state_transition(struct ipf *ipf, struct 
ipf_list *ipf_list,
 ipf_reassembled_list_add(>reassembled_pkt_list, rp);
 ipf_expiry_list_remove(ipf_list);
 next_state = IPF_LIST_STATE_COMPLETED;
+ret = rp;
 } else {
 next_state = IPF_LIST_STATE_REASS_FAIL;
 }
 }
 }
 ipf_list->state = next_state;
+
+return ret;
 }
 
 /* Some sanity checks are redundant, but prudent, in case code paths for
@@ -799,7 +804,8 @@ ipf_is_frag_duped(const struct ipf_frag *frag_list, int 
last_inuse_idx,
 static bool
 ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
  struct dp_packet *pkt, uint16_t start_data_byte,
- uint16_t end_data_byte, bool ff, bool lf, bool v6)
+ uint16_t end_data_byte, bool ff, bool lf, bool v6,
+ struct reassembled_pkt **rp)
 OVS_REQUIRES(ipf->ipf_lock)
 {
 bool duped_frag = ipf_is_frag_duped(ipf_list->frag_list,
@@ -820,7 +826,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
 ipf_list->last_inuse_idx++;
 atomic_count_inc(>nfrag);
 ipf_count(ipf, v6, IPF_NFRAGS_ACCEPTED);
-ipf_list_state_transition(ipf, ipf_list, ff, lf, v6);
+*rp = ipf_list_state_transition(ipf, ipf_list, ff, lf, v6);
 } else {
 OVS_NOT_REACHED();
 }
@@ -853,7 +859,8 @@ ipf_list_init(struct ipf_list *ipf_list, struct 
ipf_list_key *key,
  * to a list of fragemnts. */
 static bool
 ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, ovs_be16 dl_type,
-uint16_t zone, long long now, uint32_t hash_basis)
+uint16_t zone, long long now, uint32_t hash_basis,
+struct reassembled_pkt **rp)
 OVS_REQUIRES(ipf->ipf_lock)
 {
 struct ipf_list_key key;
@@ -922,7 +929,7 @@ ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, 
ovs_be16 dl_type,
 }
 
 return ipf_process_frag(ipf, ipf_list, pkt, start_data_byte,
-end_data_byte, ff, lf, v6);
+end_data_byte, ff, lf, v6, rp);
 }
 
 /* Filters out fragments from a batch of fragments and adjust the batch. */
@@ -941,11 +948,17 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct 
dp_packet_batch *pb,
   ||
   (dl_type == htons(ETH_TYPE_IPV6) &&
   ipf_is_valid_v6_frag(ipf, pkt {
+struct reassembled_pkt *rp = NULL;
 
 ovs_mutex_lock(>ipf_lock);
-if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis)) {
+if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis,
+ )) {
 dp_packet_batch_refill(pb, pkt, pb_idx);
 } else {
+if (rp && !dp_packet_batch_is_full(pb)) {
+dp_packet_batch_refill(pb, rp->pkt, pb_idx);
+rp->list->reass_execute_ctx = rp->pkt;
+}
   

[ovs-dev] [PATCH v2 1/2] ipf: Only add fragments to batch of same dl_type.

[Mike Pattrick]

When conntrack is reassembling packet fragments, the same reassembly
context can be shared across multiple threads handling different packets
simultaneously. Once a full packet is assembled, it is added to a packet
batch for processing, this is most likely the batch that added it in the
first place, but that isn't a guarantee.

The packets in these batches should be segregated by network protocol
version (ipv4 vs ipv6) for conntrack defragmentation to function
appropriately. However, there are conditions where we would add a
reassembled packet of one type to a batch of another.

This change introduces checks to make sure that reassembled or expired
fragments are only added to packet batches of the same type.

Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
Reported-at: https://issues.redhat.com/browse/FDP-560
Signed-off-by: Mike Pattrick 
---
 lib/ipf.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/ipf.c b/lib/ipf.c
index 7d74e2c13..3c8960be3 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -1063,6 +1063,9 @@ ipf_send_completed_frags(struct ipf *ipf, struct 
dp_packet_batch *pb,
 struct ipf_list *ipf_list;
 
 LIST_FOR_EACH_SAFE (ipf_list, list_node, >frag_complete_list) {
+if ((ipf_list->key.dl_type == htons(ETH_TYPE_IPV6)) != v6) {
+continue;
+}
 if (ipf_send_frags_in_list(ipf, ipf_list, pb, IPF_FRAG_COMPLETED_LIST,
v6, now)) {
 ipf_completed_list_clean(>frag_lists, ipf_list);
@@ -1096,6 +1099,9 @@ ipf_send_expired_frags(struct ipf *ipf, struct 
dp_packet_batch *pb,
 size_t lists_removed = 0;
 
 LIST_FOR_EACH_SAFE (ipf_list, list_node, >frag_exp_list) {
+if ((ipf_list->key.dl_type == htons(ETH_TYPE_IPV6)) != v6) {
+continue;
+}
 if (now <= ipf_list->expiration ||
 lists_removed >= IPF_FRAG_LIST_MAX_EXPIRED) {
 break;
@@ -1116,7 +1122,8 @@ ipf_send_expired_frags(struct ipf *ipf, struct 
dp_packet_batch *pb,
 /* Adds a reassmebled packet to a packet batch to be processed by the caller.
  */
 static void
-ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb)
+ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb,
+   ovs_be16 dl_type)
 {
 if (ovs_list_is_empty(>reassembled_pkt_list)) {
 return;
@@ -1127,6 +1134,7 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct 
dp_packet_batch *pb)
 
 LIST_FOR_EACH_SAFE (rp, rp_list_node, >reassembled_pkt_list) {
 if (!rp->list->reass_execute_ctx &&
+rp->list->key.dl_type == dl_type &&
 ipf_dp_packet_batch_add(pb, rp->pkt, false)) {
 rp->list->reass_execute_ctx = rp->pkt;
 }
@@ -1237,7 +1245,7 @@ ipf_preprocess_conntrack(struct ipf *ipf, struct 
dp_packet_batch *pb,
 }
 
 if (ipf_get_enabled(ipf) || atomic_count_get(>nfrag)) {
-ipf_execute_reass_pkts(ipf, pb);
+ipf_execute_reass_pkts(ipf, pb, dl_type);
 }
 }
 
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ipf: Only add fragments to batch of same dl_type.

[Mike Pattrick]

On Thu, May 16, 2024 at 8:35 AM Simon Horman  wrote:
>
> Hi Mike,
>
> On Wed, May 15, 2024 at 12:24:33PM -0400, Mike Pattrick wrote:
> > When conntrack is reassembling packet fragments, the same reassembly
> > context can be shared across multiple threads handling different packets
> > simultaneously. Once a full packet is assembled, it is added to a packet
> > batch for processing, this is most likely the batch that added it in the
> > first place, but that isn't a guarantee.
> >
> > The packets in these batches should be segregated by network protocol
> > versuib (ipv4 vs ipv6) for conntrack defragmentation to function
>
> nit: version
>
> > appropriately. However, there are conditions where we would add a
> > reassembled packet of one type to a batch of another.
> >
> > This change introduces checks to make sure that reassembled or expired
> > fragments are only added to packet batches of the same type. It also
> > makes a best effort attempt to make sure the defragmented packet is
> > inserted into the current batch.
>
> Would it make any sense to separate these changes into separate patches?

Can do!

>
> >
> > Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> > Reported-at: https://issues.redhat.com/browse/FDP-560
> > Signed-off-by: Mike Pattrick 
> > ---
> > Note: This solution is far from perfect, ipf.c can still insert packets
> > into more or less arbitrary batches but this bug fix is needed to avoid a
> > memory overrun and should insert packets into the proper batch in the
> > common case. I'm working on a more correct solution but it changes how
> > fragments are fundimentally handled, and couldn't be considered a bug fix.
>
> FWIIW, I'm ok with changes that move things to a better, even if not ideal,
> state.
>
> ...
>
> > @@ -943,9 +952,14 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct 
> > dp_packet_batch *pb,
> >ipf_is_valid_v6_frag(ipf, pkt {
> >
> >  ovs_mutex_lock(>ipf_lock);
> > -if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, 
> > hash_basis)) {
> > +if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis,
> > + )) {
> >  dp_packet_batch_refill(pb, pkt, pb_idx);
> >  } else {
> > +if (rp && !dp_packet_batch_is_full(pb)) {
>
> The conditions under which rp are set are complex and buried
> inside the call-chain under ipf_handle_frag(). I am concerned
> that there are cases where it may be used unset here. Or that
> the complexity allows for such cases to be inadvertently added
> later.
>
> Could we make this more robust, f.e. by making sure rp is
> always initialised when ipf_handle_frag returns by setting
> it to NULL towards the top of that function.

Agreed that it's overly complex. I'll change this to initialize this
in ipf_extract_frags_from_batch(), the functions in between
ipf_list_state_transition and ipf_extract_frags_from_batch shouldn't
really touch or care about this value.

-M

>
> > +dp_packet_batch_refill(pb, rp->pkt, pb_idx);
> > +rp->list->reass_execute_ctx = rp->pkt;
> > +}
> >  dp_packet_delete(pkt);
> >  }
> >  ovs_mutex_unlock(>ipf_lock);
>
> ...
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] compiler: Fix errors in Clang 17 ubsan checks.

[Mike Pattrick]

This patch attempts to fix a large number of ubsan error messages that
take the following form:

lib/netlink-notifier.c:237:13: runtime error: call to function
route_table_change through pointer to incorrect function type
'void (*)(const void *, void *)'

In Clang 17 the undefined behaviour sanatizer check for function
pointers was enabled by default, whereas it was previously disabled
while compiling C code. These warnings are a false positive in the case
of OVS, as our macros already check to make sure the function parameter
is the correct size.

So that check is disabled in the single function that is causing all of
the errors.

Signed-off-by: Mike Pattrick 
---
v2: Changed macro name
---
 include/openvswitch/compiler.h | 11 +++
 lib/ovs-rcu.c  |  1 +
 2 files changed, 12 insertions(+)

diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h
index 878c5c6a7..f49b23683 100644
--- a/include/openvswitch/compiler.h
+++ b/include/openvswitch/compiler.h
@@ -69,6 +69,17 @@
 #define OVS_UNLIKELY(CONDITION) (!!(CONDITION))
 #endif
 
+/* Clang 17's implementation of ubsan enables checking that function pointers
+ * match the type of the called function. This currently breaks ovs-rcu, which
+ * calls multiple different types of callbacks via a generic void *(void*)
+ * function pointer type. This macro enables disabling that check for specific
+ * functions. */
+#if __clang__ && __has_feature(undefined_behavior_sanitizer)
+#define OVS_NO_SANITIZE_FUNCTION __attribute__((no_sanitize("function")))
+#else
+#define OVS_NO_SANITIZE_FUNCTION
+#endif
+
 #if __has_feature(c_thread_safety_attributes)
 /* "clang" annotations for thread safety check.
  *
diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
index 9e07d9bab..597fe6826 100644
--- a/lib/ovs-rcu.c
+++ b/lib/ovs-rcu.c
@@ -327,6 +327,7 @@ ovsrcu_postpone__(void (*function)(void *aux), void *aux)
 }
 
 static bool
+OVS_NO_SANITIZE_FUNCTION
 ovsrcu_call_postponed(void)
 {
 struct ovsrcu_cbset *cbset;
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] utilities: Correct deletion reason in flow_reval_monitor.py.

[Aaron Conole]

Eelco Chaudron  writes:

> The flow_reval_monitor.py script incorrectly reported the reasons for
> FDR_PURGE and FDR_TOO_EXPENSIVE, as their descriptions were swapped.
> This patch rectifies the order using a dictionary to avoid similar
> problems in the future.
>
> In addition this patch also syncs the delete reason output of the
> script, with the comments in the code.
>
> Fixes: 86b9e653ef22 ("revalidator: Add a USDT probe during flow deletion with 
> purge reason.")
> Signed-off-by: Eelco Chaudron 
>
> ---
> v3: - Renamed ofproto dpif to bridge in delete reasons.
> - Added comment pointing back to delete reasons in .c.
> v2: - Converted the list of strings to dictionary.
> - Added comment to code to keep code and script in sync.
> - Unified flow_delete reason comments and script output.
> ---

Acked-by: Aaron Conole 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] northd: Add bfd and bfd_consumer nodes to I-P engine.

[Lorenzo Bianconi]

> On Thu, May 2, 2024 at 8:22 AM Lorenzo Bianconi 
> wrote:
> >
> > Introduce bfd and bfd_consumer nodes to northd I-P engine to track bfd
> > connections and northd static_route/policy_route changes.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-600
> > Signed-off-by: Lorenzo Bianconi 
> 
> Hi Lorenzo,

Hi Han,

Thx for the review.

> 
> Thanks for the patch. Could you explain in more detail about the motivation
> and background information of this patch? I followed the link in
> Reported-at but still not clear.
> I assume it is for performance reasons, so could you explain what scenarios
> would benefit from this?

The main motivation for this patch is to address a request from Dumitru [0] 
for FDP-56 [1]. In particular, in the new ECMP_nexthop monitor [2], we need to
recalculate the static routes map, so he requested to compute this hashmap in
advance and reuse it in the lflow codebase and in the new ECMP_nexthop monitor
codebase. Since we will need even the BFD info there, I moved the code out of
lflow generation codebase and introduced two dedicated nodes (bfd and
bfd-consumer) in the Northd IP engine.

@Dumitru: is my explanation correct?

> 
> For the patch, I haven't looked into details yet, but it seems you didn't
> add any change handler for the two new nodes. So will there be follow up
> patches for the real performance improvement?

for BFD node the logic is already in en_bfd_run() since we do not always
return EN_UPDATED but if there are no valid changes, we will return
EN_UNCHANGED. Am I missing something?
For bfd-consumer node I think we actually do not need it. Am I wrong?

> But even for the current patch, with this: engine_add_input(_bfd,
> _northd, NULL); does it mean any en_northd change would trigger a lflow
> recompute because of the dependency en_northd -> en_bfd -> en_lflow. Would
> this be a performance regression? If not, why?

I guess no, otherwise the IP ovn tests will fail, right?

> 
> For the noop handler used, please provide a detailed explanation to justify
> it: why the node depends on the input but doesn't need to process when the
> input changes?

I think for the bfd-consumer node we have just an order dependency with northd
one, right? I will review this part.

Regards,
Lorenzo

> 
> Thanks,
> Han

[0] 
https://patchwork.ozlabs.org/project/ovn/patch/f31fb6630039c9b310b7fa489247d354f8a46719.1710257650.git.lorenzo.bianc...@redhat.com/
[1] https://issues.redhat.com/browse/FDP-56
[2] 
https://patchwork.ozlabs.org/project/ovn/cover/cover.1710257649.git.lorenzo.bianc...@redhat.com/

> 
> > ---
> >  northd/en-lflow.c|  19 +--
> >  northd/en-northd.c   |  92 +
> >  northd/en-northd.h   |   8 ++
> >  northd/inc-proc-northd.c |  21 ++-
> >  northd/northd.c  | 274 ---
> >  northd/northd.h  |  48 ++-
> >  6 files changed, 344 insertions(+), 118 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index c4b927fb8..9efbd5117 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -41,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
> >   struct lflow_input *lflow_input)
> >  {
> >  struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> > +struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> > +struct bfd_consumer_data *bfd_consumer_data =
> > +engine_get_input_data("bfd_consumer", node);
> >  struct port_group_data *pg_data =
> >  engine_get_input_data("port_group", node);
> >  struct sync_meters_data *sync_meters_data =
> > @@ -78,7 +81,10 @@ lflow_get_input_data(struct engine_node *node,
> >  lflow_input->meter_groups = _meters_data->meter_groups;
> >  lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
> >  lflow_input->svc_monitor_map = _data->svc_monitor_map;
> > -lflow_input->bfd_connections = NULL;
> > +lflow_input->bfd_connections = _data->bfd_connections;
> > +lflow_input->parsed_routes = _consumer_data->parsed_routes;
> > +lflow_input->route_tables = _consumer_data->route_tables;
> > +lflow_input->route_policies = _consumer_data->route_policies;
> >
> >  struct ed_type_global_config *global_config =
> >  engine_get_input_data("global_config", node);
> > @@ -95,25 +101,14 @@ void en_lflow_run(struct engine_node *node, void
> *data)
> >  struct lflow_input lflow_input;
> >  lflow_get_input_data(node, _input);
> >
> > -struct hmap bfd_connections = HMAP_INITIALIZER(_connections);
> > -lflow_input.bfd_connections = _connections;
> > -
> >  stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
> >
> >  struct lflow_data *lflow_data = data;
> >  lflow_table_clear(lflow_data->lflow_table);
> >  lflow_reset_northd_refs(_input);
> >
> > -build_bfd_table(eng_ctx->ovnsb_idl_txn,
> > -lflow_input.nbrec_bfd_table,
> > -

Re: [ovs-dev] [PATCH v3] utilities: Correct deletion reason in flow_reval_monitor.py.

[Ilya Maximets]

On 5/14/24 15:15, Eelco Chaudron wrote:
> The flow_reval_monitor.py script incorrectly reported the reasons for
> FDR_PURGE and FDR_TOO_EXPENSIVE, as their descriptions were swapped.
> This patch rectifies the order using a dictionary to avoid similar
> problems in the future.
> 
> In addition this patch also syncs the delete reason output of the
> script, with the comments in the code.
> 
> Fixes: 86b9e653ef22 ("revalidator: Add a USDT probe during flow deletion with 
> purge reason.")
> Signed-off-by: Eelco Chaudron 
> 
> ---
> v3: - Renamed ofproto dpif to bridge in delete reasons.
> - Added comment pointing back to delete reasons in .c.
> v2: - Converted the list of strings to dictionary.
> - Added comment to code to keep code and script in sync.
> - Unified flow_delete reason comments and script output.
> ---

I didn't test, but the code looks fine to me.  Thanks!

Acked-by: Ilya Maximets 

BTW, is there a reason why we can't just report a static string
from the USDT probe instead of an integer?  If we did that we
would not need to have this mapping in the script at all.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ipf: Only add fragments to batch of same dl_type.

[Simon Horman]

Hi Mike,

On Wed, May 15, 2024 at 12:24:33PM -0400, Mike Pattrick wrote:
> When conntrack is reassembling packet fragments, the same reassembly
> context can be shared across multiple threads handling different packets
> simultaneously. Once a full packet is assembled, it is added to a packet
> batch for processing, this is most likely the batch that added it in the
> first place, but that isn't a guarantee.
> 
> The packets in these batches should be segregated by network protocol
> versuib (ipv4 vs ipv6) for conntrack defragmentation to function

nit: version

> appropriately. However, there are conditions where we would add a
> reassembled packet of one type to a batch of another.
> 
> This change introduces checks to make sure that reassembled or expired
> fragments are only added to packet batches of the same type. It also
> makes a best effort attempt to make sure the defragmented packet is
> inserted into the current batch.

Would it make any sense to separate these changes into separate patches?

> 
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Reported-at: https://issues.redhat.com/browse/FDP-560
> Signed-off-by: Mike Pattrick 
> ---
> Note: This solution is far from perfect, ipf.c can still insert packets
> into more or less arbitrary batches but this bug fix is needed to avoid a
> memory overrun and should insert packets into the proper batch in the
> common case. I'm working on a more correct solution but it changes how
> fragments are fundimentally handled, and couldn't be considered a bug fix.

FWIIW, I'm ok with changes that move things to a better, even if not ideal,
state.

...

> @@ -943,9 +952,14 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct 
> dp_packet_batch *pb,
>ipf_is_valid_v6_frag(ipf, pkt {
>  
>  ovs_mutex_lock(>ipf_lock);
> -if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis)) {
> +if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis,
> + )) {
>  dp_packet_batch_refill(pb, pkt, pb_idx);
>  } else {
> +if (rp && !dp_packet_batch_is_full(pb)) {

The conditions under which rp are set are complex and buried
inside the call-chain under ipf_handle_frag(). I am concerned
that there are cases where it may be used unset here. Or that
the complexity allows for such cases to be inadvertently added
later.

Could we make this more robust, f.e. by making sure rp is
always initialised when ipf_handle_frag returns by setting
it to NULL towards the top of that function.

> +dp_packet_batch_refill(pb, rp->pkt, pb_idx);
> +rp->list->reass_execute_ctx = rp->pkt;
> +}
>  dp_packet_delete(pkt);
>  }
>  ovs_mutex_unlock(>ipf_lock);

...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v11 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[0-day Robot]

Bleep bloop.  Greetings Jakob Meng, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#112 FILE: utilities/ovs-appctl.c:155:
 Requires: --format=json.\n\

Lines checked: 195, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v11 1/6] Add global option for JSON output to ovs-appctl.

[0-day Robot]

Bleep bloop.  Greetings Jakob Meng, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#676 FILE: utilities/ovs-appctl.c:150:
  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\

Lines checked: 791, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v11 1/6] Add global option for JSON output to ovs-appctl.

[jmeng]

From: Jakob Meng 

For monitoring systems such as Prometheus it would be beneficial if
OVS would expose statistics in a machine-readable format.

This patch introduces support for different output formats to
ovs-appctl. It gains a global option '-f,--format' which changes it to
print a JSON document instead of plain-text for humans. For example, a
later patch implements support for
'ovs-appctl --format json dpif/show'. By default, the output format
is plain-text as before.

A new 'set-options' command has been added to lib/unixctl.c which
allows to change the output format of the commands executed afterwards
on the same socket connection. It is supposed to be run by ovs-appctl
transparently for the user when a specific output format has been
requested.
For example, when a user calls 'ovs-appctl --format json dpif/show',
then ovs-appctl will call 'set-options' to set the output format as
requested by the user and afterwards it will call the actual command
'dpif/show'.
This ovs-appctl behaviour has been implemented in a backward compatible
way. One can use an updated client (ovs-appctl) with an old server
(ovs-vswitchd) and vice versa. Of course, JSON output only works when
both sides have been updated.

Two access functions unixctl_command_{get,set}_output_format() and a
unixctl_command_reply_json function have been added to lib/unixctl.h:
unixctl_command_get_output_format() is supposed to be used in commands
like 'dpif/show' to query the requested output format. When JSON output
has been selected, the unixctl_command_reply_json() function can be
used to return JSON objects to the client (ovs-appctl) instead of
plain-text with the unixctl_command_reply{,_error}() functions.

When JSON has been requested but a command has not implemented JSON
output the plain-text output will be wrapped in a provisional JSON
document with the following structure:

  {"reply":"$PLAIN_TEXT_HERE","reply-format":"plain"}

Thus commands which have been executed successfully will not fail when
they try to render the output at a later stage.

A test for the 'version' command has been implemented which shows how
the provisional JSON document looks like in practice. For a cleaner
JSON document, the trailing newline has been moved from the program
version string to function ovs_print_version(). This way, the
plain-text output of the 'version' command has not changed.

Output formatting has been moved from unixctl_client_transact() in
lib/unixctl.c to utilities/ovs-appctl.c. The former merely returns the
JSON objects returned from the server and the latter is now responsible
for printing it properly.

In popular tools like kubectl the option for output control is usually
called '-o|--output' instead of '-f,--format'. But ovs-appctl already
has an short option '-o' which prints the available ovs-appctl options
('--option'). The now choosen name also better alignes with
ovsdb-client where '-f,--format' controls output formatting.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 Documentation/ref/ovs-appctl.8.rst |  12 ++
 NEWS   |   3 +
 lib/unixctl.c  | 195 ++---
 lib/unixctl.h  |  20 ++-
 lib/util.c |   6 +-
 python/ovs/unixctl/server.py   |   3 -
 tests/appctl.py|   5 +
 tests/ovs-vswitchd.at  |  11 ++
 utilities/ovs-appctl.c | 130 ---
 9 files changed, 314 insertions(+), 71 deletions(-)

diff --git a/Documentation/ref/ovs-appctl.8.rst 
b/Documentation/ref/ovs-appctl.8.rst
index 3ce02e984..9619c1226 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -8,6 +8,7 @@ Synopsis
 ``ovs-appctl``
 [``--target=`` | ``-t`` ]
 [``--timeout=`` | ``-T`` ]
+[``--format=`` | ``-f`` ]
  [...]
 
 ``ovs-appctl --help``
@@ -67,6 +68,17 @@ In normal use only a single option is accepted:
   runtime to approximately  seconds.  If the timeout expires,
   ``ovs-appctl`` exits with a ``SIGALRM`` signal.
 
+* ``-f `` or ``--format=``
+
+  Tells ``ovs-appctl`` which output format to use. By default, or with a
+   of ``text``, ``ovs-appctl`` will print plain-text for humans.
+  When  is ``json``, ``ovs-appctl`` will return a JSON document.
+  When ``json`` is requested, but a command has not implemented JSON
+  output, the plain-text output will be wrapped in a provisional JSON
+  document with the following structure:
+
+  ``{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}``
+
 Common Commands
 ===
 
diff --git a/NEWS b/NEWS
index b92cec532..3c52e5ec1 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
 Post-v3.3.0
 
+   - ovs-appctl:
+ * Added new option [-f|--format] to choose the output format, e.g. 'json'
+   or 'text' (by default).
- Userspace datapath:
  * Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' 

[ovs-dev] [PATCH v11 5/6] vswitchd: Add JSON output for 'list-commands' command.

[jmeng]

From: Jakob Meng 

The 'list-commands' command now supports machine-readable JSON output
in addition to the plain-text output for humans.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS  |  1 +
 lib/unixctl.c | 44 +--
 tests/ovs-vswitchd.at | 13 +
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/NEWS b/NEWS
index 0d41061d3..479310d49 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,7 @@ Post-v3.3.0
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
  * Added new option [--pretty] to print JSON output in a readable fashion.
+ * 'list-commands' now supports output in JSON format.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/lib/unixctl.c b/lib/unixctl.c
index c430eac0b..c84ca125f 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -107,24 +107,40 @@ static void
 unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
   const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
-struct ds ds = DS_EMPTY_INITIALIZER;
-const struct shash_node **nodes = shash_sort();
-size_t i;
+if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
+struct json *json_commands = json_object_create();
+const struct shash_node *node;
 
-ds_put_cstr(, "The available commands are:\n");
+SHASH_FOR_EACH (node, ) {
+const struct unixctl_command *command = node->data;
 
-for (i = 0; i < shash_count(); i++) {
-const struct shash_node *node = nodes[i];
-const struct unixctl_command *command = node->data;
-
-if (command->usage) {
-ds_put_format(, "  %-23s %s\n", node->name, command->usage);
+if (command->usage) {
+json_object_put_string(json_commands, node->name,
+   command->usage);
+}
 }
-}
-free(nodes);
+unixctl_command_reply_json(conn, json_commands);
+} else {
+struct ds ds = DS_EMPTY_INITIALIZER;
+const struct shash_node **nodes = shash_sort();
+size_t i;
 
-unixctl_command_reply(conn, ds_cstr());
-ds_destroy();
+ds_put_cstr(, "The available commands are:\n");
+
+for (i = 0; i < shash_count(); ++i) {
+const struct shash_node *node = nodes[i];
+const struct unixctl_command *command = node->data;
+
+if (command->usage) {
+ds_put_format(, "  %-23s %s\n", node->name,
+  command->usage);
+}
+}
+free(nodes);
+
+unixctl_command_reply(conn, ds_cstr());
+ds_destroy();
+}
 }
 
 static void
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 2db8138f1..4f909603a 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -281,3 +281,16 @@ AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty 
version], [0], [dnl
   "reply-format": "plain"}])
 
 AT_CLEANUP
+
+AT_SETUP([ovs-vswitchd list-commands])
+OVS_VSWITCHD_START
+
+AT_CHECK([ovs-appctl list-commands], [0], [ignore])
+AT_CHECK([ovs-appctl --format json list-commands], [0], [stdout])
+# Check that ovs-appctl prints a single line without a trailing newline.
+AT_CHECK([wc -l stdout], [0], [0 stdout
+])
+# Check that ovs-appctl prints text which roughly looks like a JSON object.
+AT_CHECK([grep -E '^\{"autoattach/show-isid":.*\}$' stdout], [0], [ignore])
+
+AT_CLEANUP
-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v11 6/6] ofproto: Add JSON output for 'dpif/show' command.

[jmeng]

From: Jakob Meng 

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 
---
 NEWS   |   1 +
 ofproto/ofproto-dpif.c | 120 +
 tests/pmd.at   |  23 
 3 files changed, 133 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 479310d49..c434b1497 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,7 @@ Post-v3.3.0
or 'text' (by default).
  * Added new option [--pretty] to print JSON output in a readable fashion.
  * 'list-commands' now supports output in JSON format.
+ * 'dpif/show' now supports output in JSON format.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 32d037be6..db0405886 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -28,6 +28,7 @@
 #include "fail-open.h"
 #include "guarded-list.h"
 #include "hmapx.h"
+#include "json.h"
 #include "lacp.h"
 #include "learn.h"
 #include "mac-learning.h"
@@ -6519,8 +6520,99 @@ done:
 return changed;
 }
 
+static struct json *
+dpif_show_backer_json(struct json *backers, const struct dpif_backer *backer)
+{
+struct json *json_backer = json_object_create();
+
+/* Add datapath as new JSON object using its name as key. */
+json_object_put(backers, dpif_name(backer->dpif), json_backer);
+
+/* Add datapath's stats under "stats" key. */
+struct dpif_dp_stats dp_stats;
+struct json *json_dp_stats = json_object_create();
+
+json_object_put(json_backer, "stats", json_dp_stats);
+dpif_get_dp_stats(backer->dpif, _stats);
+json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);
+json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
+   dp_stats.n_missed);
+
+/* Add datapath's bridges under "bridges" key. */
+struct json *json_dp_bridges = json_object_create();
+json_object_put(json_backer, "bridges", json_dp_bridges);
+
+struct shash ofproto_shash;
+shash_init(_shash);
+const struct shash_node **ofprotos = get_ofprotos(_shash);
+for (size_t i = 0; i < shash_count(_shash); i++) {
+struct ofproto_dpif *ofproto = ofprotos[i]->data;
+
+if (ofproto->backer != backer) {
+continue;
+}
+
+struct json *json_ofproto = json_object_create();
+/* Add bridge to "bridges" dictionary using its name as key. */
+json_object_put(json_dp_bridges, ofproto->up.name, json_ofproto);
+
+/* Add bridge ports to the current bridge dictionary. */
+const struct shash_node **ports;
+ports = shash_sort(>up.port_by_name);
+for (size_t j = 0; j < shash_count(>up.port_by_name); j++) {
+const struct shash_node *port = ports[j];
+struct ofport *ofport = port->data;
+
+struct json * json_ofproto_port = json_object_create();
+/* Add bridge port to a bridge's dict using port name as key. */
+json_object_put(json_ofproto, netdev_get_name(ofport->netdev),
+json_ofproto_port);
+/* Add OpenFlow port associated with a bridge port. */
+json_object_put_format(json_ofproto_port, "ofport", "%u",
+   ofport->ofp_port);
+
+/* Add bridge port number. */
+odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
+   ofport->ofp_port);
+if (odp_port != ODPP_NONE) {
+json_object_put_format(json_ofproto_port, "port_no",
+   "%"PRIu32, odp_port);
+} else {
+json_object_put_string(json_ofproto_port, "port_no", "none");
+}
+
+/* Add type of a bridge port. */
+json_object_put_string(json_ofproto_port, "type",
+   netdev_get_type(ofport->netdev));
+
+/* Add config entries for a bridge port. */
+struct json *json_port_config = json_object_create();
+struct smap config;
+smap_init();
+
+json_object_put(json_ofproto_port, "config", json_port_config);
+if (!netdev_get_config(ofport->netdev, )) {
+struct smap_node *node;
+
+SMAP_FOR_EACH (node, ) {
+json_object_put_string(json_port_config, node->key,
+   node->value);
+}
+}
+smap_destroy();
+
+} /* End of bridge port(s). */
+
+free(ports);
+} /* End of bridge(s). */
+shash_destroy(_shash);
+free(ofprotos);
+
+return json_backer;
+}
+
 

[ovs-dev] [PATCH v11 4/6] python: Add option for pretty-printing JSON output to appctl.py.

[jmeng]

From: Jakob Meng 

With the '--pretty' option, appctl.py will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys. The pretty-printed output from appctl.py is not
strictly the same as with ovs-appctl because of both use different
pretty-printing implementations.

Signed-off-by: Jakob Meng 
---
 tests/appctl.py | 13 ++---
 tests/unixctl-py.at |  5 +
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/tests/appctl.py b/tests/appctl.py
index cf3ea3642..b08bf9033 100644
--- a/tests/appctl.py
+++ b/tests/appctl.py
@@ -37,7 +37,7 @@ def connect_to_target(target):
 return client
 
 
-def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT):
+def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT, fmt_flags={}):
 if fmt == ovs.util.OutputFormat.TEXT:
 body = str(reply)
 
@@ -46,7 +46,7 @@ def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT):
 
 return body
 else:
-return ovs.json.to_string(reply)
+return ovs.json.to_string(reply, **fmt_flags)
 
 
 def main():
@@ -65,13 +65,20 @@ def main():
 help="Output format.", default="text",
 choices=[fmt.name.lower()
  for fmt in ovs.util.OutputFormat])
+parser.add_argument("--pretty", action="store_true",
+help="Format the output in a more readable fashion."
+ " Requires: --format json.")
 args = parser.parse_args()
 
+if args.format != ovs.util.OutputFormat.JSON.name.lower() and args.pretty:
+ovs.util.ovs_fatal(0, "--pretty is supported with --format json only")
+
 signal_alarm(int(args.timeout) if args.timeout else None)
 
 ovs.vlog.Vlog.init()
 target = args.target
 format = ovs.util.OutputFormat[args.format.upper()]
+format_flags = dict(pretty=True) if args.pretty else {}
 client = connect_to_target(target)
 
 if format != ovs.util.OutputFormat.TEXT:
@@ -96,7 +103,7 @@ def main():
 sys.exit(2)
 else:
 assert result is not None
-sys.stdout.write(reply_to_string(result, format))
+sys.stdout.write(reply_to_string(result, format, format_flags))
 
 
 if __name__ == '__main__':
diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
index 92f557b67..dffe40d2b 100644
--- a/tests/unixctl-py.at
+++ b/tests/unixctl-py.at
@@ -115,6 +115,11 @@ AT_CHECK([APPCTL -t test-unixctl.py version], [0], 
[expout])
 AT_CHECK([PYAPPCTL_PY -t test-unixctl.py version], [0], [expout])
 AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json version], [0], 
[dnl
 {"reply":"$(cat expout)","reply-format":"plain"}])
+AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json --pretty 
version], [0], [dnl
+{
+  "reply":"$(cat expout)",
+  "reply-format":"plain"
+}])
 
 AT_CHECK([APPCTL -t test-unixctl.py echo robot ninja], [0], [stdout])
 AT_CHECK([cat stdout | sed -e "s/u'/'/g"], [0], [dnl
-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v11 2/6] python: Add option for JSON output to unixctl classes and appctl.py.

[jmeng]

From: Jakob Meng 

This patch introduces support for different output formats to Python
Unixctl* classes and appctl.py, similar to what the previous commit did
for ovs-appctl.
In particular, tests/appctl.py gains a global option '-f,--format'
which allows users to request JSON instead of plain-text for humans.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS |  3 +++
 python/ovs/unixctl/client.py |  5 ++--
 python/ovs/unixctl/server.py | 52 +++-
 python/ovs/util.py   |  8 ++
 tests/appctl.py  | 38 +-
 tests/unixctl-py.at  |  3 +++
 6 files changed, 93 insertions(+), 16 deletions(-)

diff --git a/NEWS b/NEWS
index 3c52e5ec1..7076939c5 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,9 @@ Post-v3.3.0
- ovs-appctl:
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+   - Python:
+ * Added support for different output formats like 'json' to appctl.py and
+   Python's unixctl classes.
- Userspace datapath:
  * Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 8283f99bb..8a6fcb1b9 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -14,6 +14,7 @@
 
 import os
 
+import ovs.json
 import ovs.jsonrpc
 import ovs.stream
 import ovs.util
@@ -41,10 +42,10 @@ class UnixctlClient(object):
 return error, None, None
 
 if reply.error is not None:
-return 0, str(reply.error), None
+return 0, reply.error, None
 else:
 assert reply.result is not None
-return 0, None, str(reply.result)
+return 0, None, reply.result
 
 def close(self):
 self._conn.close()
diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index d24a7092c..0665eb837 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import argparse
 import copy
 import errno
 import os
@@ -35,6 +36,7 @@ class UnixctlConnection(object):
 assert isinstance(rpc, ovs.jsonrpc.Connection)
 self._rpc = rpc
 self._request_id = None
+self._fmt = ovs.util.OutputFormat.TEXT
 
 def run(self):
 self._rpc.run()
@@ -63,10 +65,29 @@ class UnixctlConnection(object):
 return error
 
 def reply(self, body):
-self._reply_impl(True, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+if self._fmt == ovs.util.OutputFormat.JSON:
+body = {
+"reply-format": "plain",
+"reply": body
+}
+
+return self._reply_impl_json(True, body)
+
+def reply_json(self, body):
+self._reply_impl_json(True, body)
 
 def reply_error(self, body):
-self._reply_impl(False, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+return self._reply_impl_json(False, body)
 
 # Called only by unixctl classes.
 def _close(self):
@@ -78,15 +99,11 @@ class UnixctlConnection(object):
 if not self._rpc.get_backlog():
 self._rpc.recv_wait(poller)
 
-def _reply_impl(self, success, body):
+def _reply_impl_json(self, success, body):
 assert isinstance(success, bool)
-assert body is None or isinstance(body, str)
 
 assert self._request_id is not None
 
-if body is None:
-body = ""
-
 if success:
 reply = Message.create_reply(body, self._request_id)
 else:
@@ -133,6 +150,24 @@ def _unixctl_version(conn, unused_argv, version):
 conn.reply(version)
 
 
+def _unixctl_set_options(conn, argv, unused_aux):
+assert isinstance(conn, UnixctlConnection)
+
+parser = argparse.ArgumentParser()
+parser.add_argument("--format", default="text",
+choices=[fmt.name.lower()
+ for fmt in ovs.util.OutputFormat])
+
+try:
+args = parser.parse_args(args=argv)
+except argparse.ArgumentError as e:
+conn.reply_error(str(e))
+return
+
+conn._fmt = ovs.util.OutputFormat[args.format.upper()]
+conn.reply(None)
+
+
 class UnixctlServer(object):
 def __init__(self, listener):
 assert isinstance(listener, ovs.stream.PassiveStream)
@@ -207,4 +242,7 @@ class UnixctlServer(object):
 ovs.unixctl.command_register("version", "", 0, 0, _unixctl_version,
  version)
 
+ovs.unixctl.command_register("set-options", "[--format text|json]", 0,

[ovs-dev] [PATCH v11 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[jmeng]

From: Jakob Meng 

With the '--pretty' option, ovs-appctl will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys.

Signed-off-by: Jakob Meng 
---
 Documentation/ref/ovs-appctl.8.rst |  7 ++
 NEWS   |  1 +
 tests/ovs-vswitchd.at  |  5 +
 utilities/ovs-appctl.c | 34 --
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/Documentation/ref/ovs-appctl.8.rst 
b/Documentation/ref/ovs-appctl.8.rst
index 9619c1226..db6c52b42 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -79,6 +79,13 @@ In normal use only a single option is accepted:
 
   ``{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}``
 
+* ``--pretty``
+
+  By default, JSON output is printed as compactly as possible. This option
+  causes JSON in output to be printed in a more readable fashion. For example,
+  members of objects and elements of arrays are printed one per line, with
+  indentation. Requires ``--format=json``.
+
 Common Commands
 ===
 
diff --git a/NEWS b/NEWS
index 7076939c5..0d41061d3 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,7 @@ Post-v3.3.0
- ovs-appctl:
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+ * Added new option [--pretty] to print JSON output in a readable fashion.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 1ae7fcc32..2db8138f1 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -275,4 +275,9 @@ ovs_version=$(ovs-appctl version)
 AT_CHECK_UNQUOTED([ovs-appctl --format json version], [0], [dnl
 {"reply":"$ovs_version","reply-format":"plain"}])
 
+AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty version], [0], [dnl
+{
+  "reply": "$ovs_version",
+  "reply-format": "plain"}])
+
 AT_CLEANUP
diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
index a359a14f1..2ade64336 100644
--- a/utilities/ovs-appctl.c
+++ b/utilities/ovs-appctl.c
@@ -40,13 +40,15 @@ static void usage(void);
 /* Parsed command line args. */
 struct cmdl_args {
 enum unixctl_output_fmt format;
+unsigned int format_flags;
 char *target;
 };
 
 static struct cmdl_args *cmdl_args_create(void);
 static struct cmdl_args *parse_command_line(int argc, char *argv[]);
 static struct jsonrpc *connect_to_target(const char *target);
-static char * reply_to_string(struct json *reply, enum unixctl_output_fmt fmt);
+static char * reply_to_string(struct json *reply, enum unixctl_output_fmt fmt,
+  unsigned int fmt_flags);
 
 int
 main(int argc, char *argv[])
@@ -84,7 +86,7 @@ main(int argc, char *argv[])
 
 if (cmd_error) {
 jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
 ovs_error(0, "%s: server returned an error", args->target);
 exit(2);
@@ -107,12 +109,12 @@ main(int argc, char *argv[])
 
 if (cmd_error) {
 jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
 ovs_error(0, "%s: server returned an error", args->target);
 exit(2);
 } else if (cmd_result) {
-msg = reply_to_string(cmd_result, args->format);
+msg = reply_to_string(cmd_result, args->format, args->format_flags);
 fputs(msg, stdout);
 } else {
 OVS_NOT_REACHED();
@@ -149,6 +151,8 @@ Other options:\n\
   --timeout=SECS wait at most SECS seconds for a response\n\
   -f, --format=FMT   Output format. One of: 'json', or 'text'\n\
  (default: text)\n\
+  --pretty   Format the output in a more readable fashion.\n\
+ Requires: --format=json.\n\
   -h, --help Print this helpful information\n\
   -V, --version  Display ovs-appctl version information\n",
program_name, program_name);
@@ -161,6 +165,7 @@ cmdl_args_create(void)
 struct cmdl_args *args = xmalloc(sizeof *args);
 
 args->format = UNIXCTL_OUTPUT_FMT_TEXT;
+args->format_flags = 0;
 args->target = NULL;
 
 return args;
@@ -171,7 +176,8 @@ parse_command_line(int argc, char *argv[])
 {
 enum {
 OPT_START = UCHAR_MAX + 1,
-VLOG_OPTION_ENUMS
+OPT_PRETTY,
+VLOG_OPTION_ENUMS,
 };
 static const struct option long_options[] = {
 {"target", required_argument, NULL, 't'},
@@ -179,6 +185,7 @@ parse_command_line(int argc, char *argv[])
 {"format", required_argument, NULL, 'f'},
 {"help", no_argument, NULL, 'h'},
  

[ovs-dev] [PATCH v11 0/6] Add global option to output JSON from ovs-appctl cmds.

[jmeng]

From: Jakob Meng 

v11 has one tiny change compared to v10 [0]:

* Added a missing newline at the end of utilities/ovs-appctl.c.

[0] 
https://patchwork.ozlabs.org/project/openvswitch/list/?series=407002=both=*

Jakob Meng (6):
  Add global option for JSON output to ovs-appctl.
  python: Add option for JSON output to unixctl classes and appctl.py.
  appctl: Add option '--pretty' for pretty-printing JSON output.
  python: Add option for pretty-printing JSON output to appctl.py.
  vswitchd: Add JSON output for 'list-commands' command.
  ofproto: Add JSON output for 'dpif/show' command.

 Documentation/ref/ovs-appctl.8.rst |  19 +++
 NEWS   |   9 ++
 lib/unixctl.c  | 239 ++---
 lib/unixctl.h  |  20 ++-
 lib/util.c |   6 +-
 ofproto/ofproto-dpif.c | 120 +--
 python/ovs/unixctl/client.py   |   5 +-
 python/ovs/unixctl/server.py   |  55 +--
 python/ovs/util.py |   8 +
 tests/appctl.py|  40 -
 tests/ovs-vswitchd.at  |  29 
 tests/pmd.at   |  23 +++
 tests/unixctl-py.at|   8 +
 utilities/ovs-appctl.c | 152 +++---
 14 files changed, 625 insertions(+), 108 deletions(-)

-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] northd: Fix an issue wrt mac binding aging.

[Indrajitt Valsaraj]

Issue:
In case of a Logical_Router without mac_binding_age_threshold set or a
Logical_Router with an incorrectly formatted mac_binding_threshold option,
entries were not being purged from the Mac Binding table in SouthBound.

This was because in the function `en_mac_binding_aging_run` in case of
an invalid mac_binding_threshold entry or if mac_binding_threshold is
not set we are returning from the loop instead of iterating through the
remaining LRs. As a result, subsequent runs of the aging_waker node are
also not scehduled and we end up not purging any MAC Bindings.

Fix:
This patch fixes this issue by changing the return to a continue so that
we skip the current LR but continue processing for the remaining LRs.

Fixes: 78851b6ffb58 ("northd: Support CIDR-based MAC binding aging threshold.")
Signed-off-by: Indrajitt Valsaraj 
Acked-by: Naveen Yerramneni 
---
 northd/aging.c |   2 +-
 tests/ovn.at   | 111 -
 2 files changed, 74 insertions(+), 39 deletions(-)

diff --git a/northd/aging.c b/northd/aging.c
index b76963a2d..9685044e7 100644
--- a/northd/aging.c
+++ b/northd/aging.c
@@ -421,7 +421,7 @@ en_mac_binding_aging_run(struct engine_node *node, void 
*data OVS_UNUSED)
 if (!parse_aging_threshold(smap_get(>nbr->options,
 "mac_binding_age_threshold"),
_config)) {
-return;
+continue;
 }

 aging_context_set_threshold(, _config);
diff --git a/tests/ovn.at b/tests/ovn.at
index 486680649..28a62d03d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -34407,32 +34407,40 @@ ovn_start
 net_add n1

 AT_CHECK([ovn-nbctl ls-add public])
-AT_CHECK([ovn-nbctl ls-add internal])
+AT_CHECK([ovn-nbctl ls-add internal-1])

 AT_CHECK([ovn-nbctl lsp-add public ln_port])
 AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown])
 AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
 AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])

-AT_CHECK([ovn-nbctl lsp-add public public-gw])
-AT_CHECK([ovn-nbctl lsp-set-type public-gw router])
-AT_CHECK([ovn-nbctl lsp-set-addresses public-gw 00:00:00:00:10:00 router])
-AT_CHECK([ovn-nbctl lsp-set-options public-gw router-port=gw-public])
+AT_CHECK([ovn-nbctl lsp-add public public-gw-1])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-1 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-1 00:00:00:00:10:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-1 router-port=gw-1-public])

-AT_CHECK([ovn-nbctl lsp-add internal internal-gw])
-AT_CHECK([ovn-nbctl lsp-set-type internal-gw router])
-AT_CHECK([ovn-nbctl lsp-set-addresses internal-gw 00:00:00:00:20:00 router])
-AT_CHECK([ovn-nbctl lsp-set-options internal-gw router-port=gw-internal])
+AT_CHECK([ovn-nbctl lsp-add public public-gw-2])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-2 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-2 00:00:00:00:30:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-2 router-port=gw-2-public])

-AT_CHECK([ovn-nbctl lsp-add internal vif1])
+AT_CHECK([ovn-nbctl lsp-add internal-1 internal-gw-1])
+AT_CHECK([ovn-nbctl lsp-set-type internal-gw-1 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses internal-gw-1 00:00:00:00:20:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options internal-gw-1 router-port=gw-internal-1])
+
+AT_CHECK([ovn-nbctl lsp-add internal-1 vif1])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif1 "00:00:00:00:20:10 192.168.20.10"])

-AT_CHECK([ovn-nbctl lsp-add internal vif2])
+AT_CHECK([ovn-nbctl lsp-add internal-1 vif2])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif2 "00:00:00:00:20:20 192.168.20.20"])

-AT_CHECK([ovn-nbctl lr-add gw])
-AT_CHECK([ovn-nbctl lrp-add gw gw-public 00:00:00:00:10:00 192.168.10.1/24])
-AT_CHECK([ovn-nbctl lrp-add gw gw-internal 00:00:00:00:20:00 192.168.20.1/24])
+AT_CHECK([ovn-nbctl lr-add gw-1])
+AT_CHECK([ovn-nbctl lrp-add gw-1 gw-1-public 00:00:00:00:10:00 
192.168.10.1/24])
+AT_CHECK([ovn-nbctl lrp-add gw-1 gw-internal-1 00:00:00:00:20:00 
192.168.20.1/24])
+
+AT_CHECK([ovn-nbctl lr-add gw-2])
+AT_CHECK([ovn-nbctl lrp-add gw-2 gw-2-public 00:00:00:00:30:00 
192.168.10.2/24])

 sim_add hv1
 as hv1
@@ -34500,21 +34508,27 @@ send_udp() {
 as $hv ovs-appctl netdev-dummy/receive $dev $packet
 }
 # Check if the option is not present by default
-AT_CHECK([fetch_column nb:logical_router options name="gw" | grep -q 
mac_binding_age_threshold], [1])
+AT_CHECK([fetch_column nb:logical_router options name="gw-1" | grep -q 
mac_binding_age_threshold], [1])
+AT_CHECK([fetch_column nb:logical_router options name="gw-2" | grep -q 
mac_binding_age_threshold], [1])

 # Send GARP to populate MAC binding table records
 send_garp hv1 ext1 10
 send_garp hv2 ext2 20

-wait_row_count mac_binding 1 ip="192.168.10.10"
-wait_row_count mac_binding 1 ip="192.168.10.20"
+# Two rows present for each IP, one corresponding to each logical_port
+wait_row_count mac_binding 2 ip="192.168.10.10"

Re: [ovs-dev] [PATCH v3 6/6] netdev-dpdk: Refactor tunnel checksum offloading.

[David Marchand]

On Wed, May 15, 2024 at 2:11 PM Kevin Traynor  wrote:
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 1dad2ef833..31dd6f1d5a 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2584,6 +2584,9 @@ static bool
> >  netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf 
> > *mbuf)
> >  {
> >  struct dp_packet *pkt = CONTAINER_OF(mbuf, struct dp_packet, mbuf);
> > +void *l2;
> > +void *l3;
> > +void *l4;
> >
> >  const uint64_t all_inner_requests = (RTE_MBUF_F_TX_IP_CKSUM |
> >   RTE_MBUF_F_TX_L4_MASK |
> > @@ -2613,11 +2616,6 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk 
> > *dev, struct rte_mbuf *mbuf)
> >  return true;
> >  }
> >
> > -ovs_assert(dp_packet_l4(pkt));
> > -
> > -/* If packet is vxlan or geneve tunnel packet, calculate outer
> > - * l2 len and outer l3 len. Inner l2/l3/l4 len are calculated
> > - * before. */
> >  const uint64_t tunnel_type = mbuf->ol_flags & 
> > RTE_MBUF_F_TX_TUNNEL_MASK;
> >  if (OVS_UNLIKELY(tunnel_type &&
> >   tunnel_type != RTE_MBUF_F_TX_TUNNEL_GENEVE &&
> > @@ -2635,6 +2633,11 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk 
> > *dev, struct rte_mbuf *mbuf)
> >   (char *) dp_packet_eth(pkt);
> >  mbuf->outer_l3_len = (char *) dp_packet_l4(pkt) -
> >   (char *) dp_packet_l3(pkt);
> > +
>
> > +/* Inner L2 length must account for the tunnel header length. 
> > */
> > +l2 = dp_packet_l4(pkt);
>
> Code looks ok to me, but it's tricky and the L2 settings with inner
> requests are a bit unintuitive without a notepad and thinking from the
> driver perspective backwards. Not sure there is much can be done to
> mitigate that here, other than the comment you added.

Unfortunately, I don't have a better idea.
It was already unintuitive before this patch, but to make it worse,
the logic was split across lib/netdev-dpdk.c and
lib/netdev-native-tnl.c.

Like for example this comment in dp_packet_tnl_ol_process(), which is
DPDK specific.
/* Attention please, tunnel inner l2 len is consist of udp header
 * len and tunnel header len and inner l2 len. */


>
> Did you manage to test to confirm they're working as expected ?

In general, I tested the series with CX6, E810 and ixgbe, with ipv4
traffic, ipv4 traffic tunneled in ipv4/ipv6 vxlan and ipv4 traffic
tunneled in ipv4/ipv6 geneve.
But I am not sure I covered every possible combinations.

Specifically for this case you point at (outer + inner offloads), I
tested CX6 with IPv4/IPv6 VxLAN and Geneve (for which I have traces in
my bash history).
With E810, I remember testing the same with the DPDK fixes, but I
don't have a trace of it.

I'll double check before sending a next revision.

>
> > +l3 = dp_packet_inner_l3(pkt);
> > +l4 = dp_packet_inner_l4(pkt);
>
> see below
>
> >  } else {
> >  /* If no outer offloading is requested, clear outer marks. */
> >  mbuf->ol_flags &= ~all_outer_marks;
> > @@ -2642,8 +2645,9 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, 
> > struct rte_mbuf *mbuf)
> >  mbuf->outer_l3_len = 0;
> >
> >  /* Skip outer headers. */
> > -mbuf->l2_len += (char *) dp_packet_l4(pkt) -
> > -(char *) dp_packet_eth(pkt);
> > +l2 = dp_packet_eth(pkt);
>
> > +l3 = dp_packet_inner_l3(pkt);
> > +l4 = dp_packet_inner_l4(pkt);
>
> You could move these outside the inner (pardon the pun) if else, but I
> could understand if you prefer to set l2/l3/l4 together for better
> readability ?

Well, as you noted, this code is not trivial.
I preferred to have all 3 pointers grouped, with a comment relating to
the group.


-- 
David Marchand

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v10 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[0-day Robot]

Bleep bloop.  Greetings Jakob Meng, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#112 FILE: utilities/ovs-appctl.c:155:
 Requires: --format=json.\n\

Lines checked: 196, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v10 1/6] Add global option for JSON output to ovs-appctl.

[0-day Robot]

Bleep bloop.  Greetings Jakob Meng, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#676 FILE: utilities/ovs-appctl.c:150:
  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\

Lines checked: 792, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v9 6/6] ofproto: Add JSON output for 'dpif/show' command.

[Jakob Meng]

On 13.04.24 00:55, Ilya Maximets wrote:
> On 4/12/24 09:26, jm...@redhat.com wrote:
>> From: Jakob Meng 
>>
>> 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 
>> ---
>>  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.
>
>

Hi Ilya!

Thank you for taking the time and reviewing all patches of the series ☺️ Very 
much appreciated! I agree with all you wrote and prepared a new patch series 
v10 [0]. I am curious to hear your opinion on the latest changes 

[0] 
https://patchwork.ozlabs.org/project/openvswitch/list/?series=407002=%2A=both

Best regards,
Jakob
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 5/6] netdev-dpdk: Use guest TSO segmentation size hint.

[David Marchand]

On Wed, May 15, 2024 at 2:09 PM Kevin Traynor  wrote:
>
> On 19/04/2024 15:06, David Marchand wrote:
> > In a typical setup like:
> > guest A <-virtio-> OVS A <-vxlan-> OVS B <-virtio-> guest B
> >
> > TSO packets from guest A are segmented against the OVS A physical port
> > mtu adjusted by the vxlan tunnel header size, regardless of guest A
> > interface mtu.
> >
> > As an example, let's say guest A and guest B mtu are set to 1500 bytes.
> > OVS A and OVS B physical ports mtu are set to 1600 bytes.
> > Guest A will request TCP segmentation for 1448 bytes segments.
> > On the other hand, OVS A will request 1498 bytes segments to the HW.
> > This results in OVS B dropping packets because decapsulated packets
> > are larger than the vhost-user port (serving guest B) mtu.
> >
> > 2024-04-17T14:13:01.239Z|2|netdev_dpdk(pmd-c03/id:7)|WARN|vhost0:
> >   Too big size 1564 max_packet_len 1518
> >
> > vhost-user ports expose a guest mtu by filling mbuf->tso_segsz.
> > Use it as a hint.
> >
> > This may result in segments (on the wire) slightly shorter than the
> > optimal size.
> >
> > Reported-at: https://github.com/openvswitch/ovs-issues/issues/321
> > Signed-off-by: David Marchand 
> > ---
> > Note:
> > As we trust the guest with this change, should we put a lower limit on
> > mbuf->tso_segsz?
> >
>
> There are some checks I looked at (e.g [0]), but it could be checked
> here for an earlier drop i suppose...additional comment below
>
> [0]
> https://git.dpdk.org/dpdk/tree/drivers/net/ice/ice_rxtx.c#n3754

I guess you meant
https://git.dpdk.org/dpdk/tree/drivers/net/ice/ice_rxtx.c#n3818
And same in v23.11, there are checks at the prepare stage:
https://git.dpdk.org/dpdk-stable/tree/drivers/net/ice/ice_rxtx.c?h=23.11#n3681

I had forgotten about those checks.
There is no limit exposed per driver from DPDK, so the simpler for OVS
is to trust them.


>
> > ---
> >  lib/netdev-dpdk.c | 11 ---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 661269e4b6..1dad2ef833 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2671,14 +2671,19 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk 
> > *dev, struct rte_mbuf *mbuf)
> >
> >  if (mbuf->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
> >  struct tcp_header *th = dp_packet_l4(pkt);
> > +uint16_t link_tso_segsz;
> >  int hdr_len;
> >
> >  if (tunnel_type) {
> > -mbuf->tso_segsz = dev->mtu - mbuf->l2_len - mbuf->l3_len -
> > -  mbuf->l4_len - mbuf->outer_l3_len;
> > +link_tso_segsz = dev->mtu - mbuf->l2_len - mbuf->l3_len -
> > + mbuf->l4_len - mbuf->outer_l3_len;
> >  } else {
> >  mbuf->l4_len = TCP_OFFSET(th->tcp_ctl) * 4;
> > -mbuf->tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;
> > +link_tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;
> > +}
> > +
> > +if (!mbuf->tso_segsz || mbuf->tso_segsz > link_tso_segsz) {
>
> It seems like something is not right if the flag is set but tso_segsz is
> 0. It is set by vhost lib from gso_size, but I don't see a validation
> there either.

At the time I added a check on the 0 value, I thought there was a case
where RTE_MBUF_F_TX_TCP_SEG could be set with no segsz value.
But as you mention, all setters of this flag (either in vhost or in
OVS) set a segsz too.

So with segsz always set, combined with the drivers check, OVS
probably does not need any check on tso_segsz.
I intend to remove this check in a next revision.


-- 
David Marchand

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v10 6/6] ofproto: Add JSON output for 'dpif/show' command.

[jmeng]

From: Jakob Meng 

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 
---
 NEWS   |   1 +
 ofproto/ofproto-dpif.c | 120 +
 tests/pmd.at   |  23 
 3 files changed, 133 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 479310d49..c434b1497 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,7 @@ Post-v3.3.0
or 'text' (by default).
  * Added new option [--pretty] to print JSON output in a readable fashion.
  * 'list-commands' now supports output in JSON format.
+ * 'dpif/show' now supports output in JSON format.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 32d037be6..db0405886 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -28,6 +28,7 @@
 #include "fail-open.h"
 #include "guarded-list.h"
 #include "hmapx.h"
+#include "json.h"
 #include "lacp.h"
 #include "learn.h"
 #include "mac-learning.h"
@@ -6519,8 +6520,99 @@ done:
 return changed;
 }
 
+static struct json *
+dpif_show_backer_json(struct json *backers, const struct dpif_backer *backer)
+{
+struct json *json_backer = json_object_create();
+
+/* Add datapath as new JSON object using its name as key. */
+json_object_put(backers, dpif_name(backer->dpif), json_backer);
+
+/* Add datapath's stats under "stats" key. */
+struct dpif_dp_stats dp_stats;
+struct json *json_dp_stats = json_object_create();
+
+json_object_put(json_backer, "stats", json_dp_stats);
+dpif_get_dp_stats(backer->dpif, _stats);
+json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);
+json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
+   dp_stats.n_missed);
+
+/* Add datapath's bridges under "bridges" key. */
+struct json *json_dp_bridges = json_object_create();
+json_object_put(json_backer, "bridges", json_dp_bridges);
+
+struct shash ofproto_shash;
+shash_init(_shash);
+const struct shash_node **ofprotos = get_ofprotos(_shash);
+for (size_t i = 0; i < shash_count(_shash); i++) {
+struct ofproto_dpif *ofproto = ofprotos[i]->data;
+
+if (ofproto->backer != backer) {
+continue;
+}
+
+struct json *json_ofproto = json_object_create();
+/* Add bridge to "bridges" dictionary using its name as key. */
+json_object_put(json_dp_bridges, ofproto->up.name, json_ofproto);
+
+/* Add bridge ports to the current bridge dictionary. */
+const struct shash_node **ports;
+ports = shash_sort(>up.port_by_name);
+for (size_t j = 0; j < shash_count(>up.port_by_name); j++) {
+const struct shash_node *port = ports[j];
+struct ofport *ofport = port->data;
+
+struct json * json_ofproto_port = json_object_create();
+/* Add bridge port to a bridge's dict using port name as key. */
+json_object_put(json_ofproto, netdev_get_name(ofport->netdev),
+json_ofproto_port);
+/* Add OpenFlow port associated with a bridge port. */
+json_object_put_format(json_ofproto_port, "ofport", "%u",
+   ofport->ofp_port);
+
+/* Add bridge port number. */
+odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
+   ofport->ofp_port);
+if (odp_port != ODPP_NONE) {
+json_object_put_format(json_ofproto_port, "port_no",
+   "%"PRIu32, odp_port);
+} else {
+json_object_put_string(json_ofproto_port, "port_no", "none");
+}
+
+/* Add type of a bridge port. */
+json_object_put_string(json_ofproto_port, "type",
+   netdev_get_type(ofport->netdev));
+
+/* Add config entries for a bridge port. */
+struct json *json_port_config = json_object_create();
+struct smap config;
+smap_init();
+
+json_object_put(json_ofproto_port, "config", json_port_config);
+if (!netdev_get_config(ofport->netdev, )) {
+struct smap_node *node;
+
+SMAP_FOR_EACH (node, ) {
+json_object_put_string(json_port_config, node->key,
+   node->value);
+}
+}
+smap_destroy();
+
+} /* End of bridge port(s). */
+
+free(ports);
+} /* End of bridge(s). */
+shash_destroy(_shash);
+free(ofprotos);
+
+return json_backer;
+}
+
 

[ovs-dev] [PATCH v10 4/6] python: Add option for pretty-printing JSON output to appctl.py.

[jmeng]

From: Jakob Meng 

With the '--pretty' option, appctl.py will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys. The pretty-printed output from appctl.py is not
strictly the same as with ovs-appctl because of both use different
pretty-printing implementations.

Signed-off-by: Jakob Meng 
---
 tests/appctl.py | 13 ++---
 tests/unixctl-py.at |  5 +
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/tests/appctl.py b/tests/appctl.py
index cf3ea3642..b08bf9033 100644
--- a/tests/appctl.py
+++ b/tests/appctl.py
@@ -37,7 +37,7 @@ def connect_to_target(target):
 return client
 
 
-def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT):
+def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT, fmt_flags={}):
 if fmt == ovs.util.OutputFormat.TEXT:
 body = str(reply)
 
@@ -46,7 +46,7 @@ def reply_to_string(reply, fmt=ovs.util.OutputFormat.TEXT):
 
 return body
 else:
-return ovs.json.to_string(reply)
+return ovs.json.to_string(reply, **fmt_flags)
 
 
 def main():
@@ -65,13 +65,20 @@ def main():
 help="Output format.", default="text",
 choices=[fmt.name.lower()
  for fmt in ovs.util.OutputFormat])
+parser.add_argument("--pretty", action="store_true",
+help="Format the output in a more readable fashion."
+ " Requires: --format json.")
 args = parser.parse_args()
 
+if args.format != ovs.util.OutputFormat.JSON.name.lower() and args.pretty:
+ovs.util.ovs_fatal(0, "--pretty is supported with --format json only")
+
 signal_alarm(int(args.timeout) if args.timeout else None)
 
 ovs.vlog.Vlog.init()
 target = args.target
 format = ovs.util.OutputFormat[args.format.upper()]
+format_flags = dict(pretty=True) if args.pretty else {}
 client = connect_to_target(target)
 
 if format != ovs.util.OutputFormat.TEXT:
@@ -96,7 +103,7 @@ def main():
 sys.exit(2)
 else:
 assert result is not None
-sys.stdout.write(reply_to_string(result, format))
+sys.stdout.write(reply_to_string(result, format, format_flags))
 
 
 if __name__ == '__main__':
diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
index 92f557b67..dffe40d2b 100644
--- a/tests/unixctl-py.at
+++ b/tests/unixctl-py.at
@@ -115,6 +115,11 @@ AT_CHECK([APPCTL -t test-unixctl.py version], [0], 
[expout])
 AT_CHECK([PYAPPCTL_PY -t test-unixctl.py version], [0], [expout])
 AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json version], [0], 
[dnl
 {"reply":"$(cat expout)","reply-format":"plain"}])
+AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json --pretty 
version], [0], [dnl
+{
+  "reply":"$(cat expout)",
+  "reply-format":"plain"
+}])
 
 AT_CHECK([APPCTL -t test-unixctl.py echo robot ninja], [0], [stdout])
 AT_CHECK([cat stdout | sed -e "s/u'/'/g"], [0], [dnl
-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v10 2/6] python: Add option for JSON output to unixctl classes and appctl.py.

[jmeng]

From: Jakob Meng 

This patch introduces support for different output formats to Python
Unixctl* classes and appctl.py, similar to what the previous commit did
for ovs-appctl.
In particular, tests/appctl.py gains a global option '-f,--format'
which allows users to request JSON instead of plain-text for humans.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS |  3 +++
 python/ovs/unixctl/client.py |  5 ++--
 python/ovs/unixctl/server.py | 52 +++-
 python/ovs/util.py   |  8 ++
 tests/appctl.py  | 38 +-
 tests/unixctl-py.at  |  3 +++
 6 files changed, 93 insertions(+), 16 deletions(-)

diff --git a/NEWS b/NEWS
index 3c52e5ec1..7076939c5 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,9 @@ Post-v3.3.0
- ovs-appctl:
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+   - Python:
+ * Added support for different output formats like 'json' to appctl.py and
+   Python's unixctl classes.
- Userspace datapath:
  * Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 8283f99bb..8a6fcb1b9 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -14,6 +14,7 @@
 
 import os
 
+import ovs.json
 import ovs.jsonrpc
 import ovs.stream
 import ovs.util
@@ -41,10 +42,10 @@ class UnixctlClient(object):
 return error, None, None
 
 if reply.error is not None:
-return 0, str(reply.error), None
+return 0, reply.error, None
 else:
 assert reply.result is not None
-return 0, None, str(reply.result)
+return 0, None, reply.result
 
 def close(self):
 self._conn.close()
diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index d24a7092c..0665eb837 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import argparse
 import copy
 import errno
 import os
@@ -35,6 +36,7 @@ class UnixctlConnection(object):
 assert isinstance(rpc, ovs.jsonrpc.Connection)
 self._rpc = rpc
 self._request_id = None
+self._fmt = ovs.util.OutputFormat.TEXT
 
 def run(self):
 self._rpc.run()
@@ -63,10 +65,29 @@ class UnixctlConnection(object):
 return error
 
 def reply(self, body):
-self._reply_impl(True, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+if self._fmt == ovs.util.OutputFormat.JSON:
+body = {
+"reply-format": "plain",
+"reply": body
+}
+
+return self._reply_impl_json(True, body)
+
+def reply_json(self, body):
+self._reply_impl_json(True, body)
 
 def reply_error(self, body):
-self._reply_impl(False, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+return self._reply_impl_json(False, body)
 
 # Called only by unixctl classes.
 def _close(self):
@@ -78,15 +99,11 @@ class UnixctlConnection(object):
 if not self._rpc.get_backlog():
 self._rpc.recv_wait(poller)
 
-def _reply_impl(self, success, body):
+def _reply_impl_json(self, success, body):
 assert isinstance(success, bool)
-assert body is None or isinstance(body, str)
 
 assert self._request_id is not None
 
-if body is None:
-body = ""
-
 if success:
 reply = Message.create_reply(body, self._request_id)
 else:
@@ -133,6 +150,24 @@ def _unixctl_version(conn, unused_argv, version):
 conn.reply(version)
 
 
+def _unixctl_set_options(conn, argv, unused_aux):
+assert isinstance(conn, UnixctlConnection)
+
+parser = argparse.ArgumentParser()
+parser.add_argument("--format", default="text",
+choices=[fmt.name.lower()
+ for fmt in ovs.util.OutputFormat])
+
+try:
+args = parser.parse_args(args=argv)
+except argparse.ArgumentError as e:
+conn.reply_error(str(e))
+return
+
+conn._fmt = ovs.util.OutputFormat[args.format.upper()]
+conn.reply(None)
+
+
 class UnixctlServer(object):
 def __init__(self, listener):
 assert isinstance(listener, ovs.stream.PassiveStream)
@@ -207,4 +242,7 @@ class UnixctlServer(object):
 ovs.unixctl.command_register("version", "", 0, 0, _unixctl_version,
  version)
 
+ovs.unixctl.command_register("set-options", "[--format text|json]", 0,

[ovs-dev] [PATCH v10 0/6] Add global option to output JSON from ovs-appctl cmds.

[jmeng]

From: Jakob Meng 

v10 addresses all of Ilya's comments for v9 [0], most importantly:

* Output formatting has been moved from unixctl_client_transact() in unixctl.c 
to ovs-appctl.c; same for Python code.
* unixctl_command_reply_error() will never wrap plain-text in JSON.
* unixctl_command_reply_error_json() has been dropped.
* Home-grown args parsing in unixctl_set_options() has been replaced with 
ovs_cmdl_parse_all().
* ovs-appctl manpage has been updated with '--format' and '--pretty' options.
* JSON output is always sorted now (also for non-pretty).
* JSON output of 'list-commands' will now be a dictionary, not an array.
* JSON output of 'dpif/show' command changed (last patch).
* NEWS entries have been sorted, clarified and shortened.
* Trailing newline has been moved from program version string to 
ovs_print_version(). For cleaner JSON output in new 'version' test.

[0] 
https://patchwork.ozlabs.org/project/openvswitch/list/?series=402752=both=*

Thanks again, Ilya, for your thorough review and helpful suggestions☺️ I hope, 
the patches are getting better now!

Best regards,
Jakob

Jakob Meng (6):
  Add global option for JSON output to ovs-appctl.
  python: Add option for JSON output to unixctl classes and appctl.py.
  appctl: Add option '--pretty' for pretty-printing JSON output.
  python: Add option for pretty-printing JSON output to appctl.py.
  vswitchd: Add JSON output for 'list-commands' command.
  ofproto: Add JSON output for 'dpif/show' command.

 Documentation/ref/ovs-appctl.8.rst |  19 +++
 NEWS   |   9 ++
 lib/unixctl.c  | 239 ++---
 lib/unixctl.h  |  20 ++-
 lib/util.c |   6 +-
 ofproto/ofproto-dpif.c | 120 +--
 python/ovs/unixctl/client.py   |   5 +-
 python/ovs/unixctl/server.py   |  55 +--
 python/ovs/util.py |   8 +
 tests/appctl.py|  40 -
 tests/ovs-vswitchd.at  |  29 
 tests/pmd.at   |  23 +++
 tests/unixctl-py.at|   8 +
 utilities/ovs-appctl.c | 152 +++---
 14 files changed, 625 insertions(+), 108 deletions(-)

-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v10 5/6] vswitchd: Add JSON output for 'list-commands' command.

[jmeng]

From: Jakob Meng 

The 'list-commands' command now supports machine-readable JSON output
in addition to the plain-text output for humans.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS  |  1 +
 lib/unixctl.c | 44 +--
 tests/ovs-vswitchd.at | 13 +
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/NEWS b/NEWS
index 0d41061d3..479310d49 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,7 @@ Post-v3.3.0
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
  * Added new option [--pretty] to print JSON output in a readable fashion.
+ * 'list-commands' now supports output in JSON format.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/lib/unixctl.c b/lib/unixctl.c
index c430eac0b..c84ca125f 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -107,24 +107,40 @@ static void
 unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
   const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
-struct ds ds = DS_EMPTY_INITIALIZER;
-const struct shash_node **nodes = shash_sort();
-size_t i;
+if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
+struct json *json_commands = json_object_create();
+const struct shash_node *node;
 
-ds_put_cstr(, "The available commands are:\n");
+SHASH_FOR_EACH (node, ) {
+const struct unixctl_command *command = node->data;
 
-for (i = 0; i < shash_count(); i++) {
-const struct shash_node *node = nodes[i];
-const struct unixctl_command *command = node->data;
-
-if (command->usage) {
-ds_put_format(, "  %-23s %s\n", node->name, command->usage);
+if (command->usage) {
+json_object_put_string(json_commands, node->name,
+   command->usage);
+}
 }
-}
-free(nodes);
+unixctl_command_reply_json(conn, json_commands);
+} else {
+struct ds ds = DS_EMPTY_INITIALIZER;
+const struct shash_node **nodes = shash_sort();
+size_t i;
 
-unixctl_command_reply(conn, ds_cstr());
-ds_destroy();
+ds_put_cstr(, "The available commands are:\n");
+
+for (i = 0; i < shash_count(); ++i) {
+const struct shash_node *node = nodes[i];
+const struct unixctl_command *command = node->data;
+
+if (command->usage) {
+ds_put_format(, "  %-23s %s\n", node->name,
+  command->usage);
+}
+}
+free(nodes);
+
+unixctl_command_reply(conn, ds_cstr());
+ds_destroy();
+}
 }
 
 static void
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 2db8138f1..4f909603a 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -281,3 +281,16 @@ AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty 
version], [0], [dnl
   "reply-format": "plain"}])
 
 AT_CLEANUP
+
+AT_SETUP([ovs-vswitchd list-commands])
+OVS_VSWITCHD_START
+
+AT_CHECK([ovs-appctl list-commands], [0], [ignore])
+AT_CHECK([ovs-appctl --format json list-commands], [0], [stdout])
+# Check that ovs-appctl prints a single line without a trailing newline.
+AT_CHECK([wc -l stdout], [0], [0 stdout
+])
+# Check that ovs-appctl prints text which roughly looks like a JSON object.
+AT_CHECK([grep -E '^\{"autoattach/show-isid":.*\}$' stdout], [0], [ignore])
+
+AT_CLEANUP
-- 
2.39.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v10 1/6] Add global option for JSON output to ovs-appctl.

[jmeng]

From: Jakob Meng 

For monitoring systems such as Prometheus it would be beneficial if
OVS would expose statistics in a machine-readable format.

This patch introduces support for different output formats to
ovs-appctl. It gains a global option '-f,--format' which changes it to
print a JSON document instead of plain-text for humans. For example, a
later patch implements support for
'ovs-appctl --format json dpif/show'. By default, the output format
is plain-text as before.

A new 'set-options' command has been added to lib/unixctl.c which
allows to change the output format of the commands executed afterwards
on the same socket connection. It is supposed to be run by ovs-appctl
transparently for the user when a specific output format has been
requested.
For example, when a user calls 'ovs-appctl --format json dpif/show',
then ovs-appctl will call 'set-options' to set the output format as
requested by the user and afterwards it will call the actual command
'dpif/show'.
This ovs-appctl behaviour has been implemented in a backward compatible
way. One can use an updated client (ovs-appctl) with an old server
(ovs-vswitchd) and vice versa. Of course, JSON output only works when
both sides have been updated.

Two access functions unixctl_command_{get,set}_output_format() and a
unixctl_command_reply_json function have been added to lib/unixctl.h:
unixctl_command_get_output_format() is supposed to be used in commands
like 'dpif/show' to query the requested output format. When JSON output
has been selected, the unixctl_command_reply_json() function can be
used to return JSON objects to the client (ovs-appctl) instead of
plain-text with the unixctl_command_reply{,_error}() functions.

When JSON has been requested but a command has not implemented JSON
output the plain-text output will be wrapped in a provisional JSON
document with the following structure:

  {"reply":"$PLAIN_TEXT_HERE","reply-format":"plain"}

Thus commands which have been executed successfully will not fail when
they try to render the output at a later stage.

A test for the 'version' command has been implemented which shows how
the provisional JSON document looks like in practice. For a cleaner
JSON document, the trailing newline has been moved from the program
version string to function ovs_print_version(). This way, the
plain-text output of the 'version' command has not changed.

Output formatting has been moved from unixctl_client_transact() in
lib/unixctl.c to utilities/ovs-appctl.c. The former merely returns the
JSON objects returned from the server and the latter is now responsible
for printing it properly.

In popular tools like kubectl the option for output control is usually
called '-o|--output' instead of '-f,--format'. But ovs-appctl already
has an short option '-o' which prints the available ovs-appctl options
('--option'). The now choosen name also better alignes with
ovsdb-client where '-f,--format' controls output formatting.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 Documentation/ref/ovs-appctl.8.rst |  12 ++
 NEWS   |   3 +
 lib/unixctl.c  | 195 ++---
 lib/unixctl.h  |  20 ++-
 lib/util.c |   6 +-
 python/ovs/unixctl/server.py   |   3 -
 tests/appctl.py|   5 +
 tests/ovs-vswitchd.at  |  11 ++
 utilities/ovs-appctl.c | 130 ---
 9 files changed, 314 insertions(+), 71 deletions(-)

diff --git a/Documentation/ref/ovs-appctl.8.rst 
b/Documentation/ref/ovs-appctl.8.rst
index 3ce02e984..9619c1226 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -8,6 +8,7 @@ Synopsis
 ``ovs-appctl``
 [``--target=`` | ``-t`` ]
 [``--timeout=`` | ``-T`` ]
+[``--format=`` | ``-f`` ]
  [...]
 
 ``ovs-appctl --help``
@@ -67,6 +68,17 @@ In normal use only a single option is accepted:
   runtime to approximately  seconds.  If the timeout expires,
   ``ovs-appctl`` exits with a ``SIGALRM`` signal.
 
+* ``-f `` or ``--format=``
+
+  Tells ``ovs-appctl`` which output format to use. By default, or with a
+   of ``text``, ``ovs-appctl`` will print plain-text for humans.
+  When  is ``json``, ``ovs-appctl`` will return a JSON document.
+  When ``json`` is requested, but a command has not implemented JSON
+  output, the plain-text output will be wrapped in a provisional JSON
+  document with the following structure:
+
+  ``{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}``
+
 Common Commands
 ===
 
diff --git a/NEWS b/NEWS
index b92cec532..3c52e5ec1 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
 Post-v3.3.0
 
+   - ovs-appctl:
+ * Added new option [-f|--format] to choose the output format, e.g. 'json'
+   or 'text' (by default).
- Userspace datapath:
  * Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' 

[ovs-dev] [PATCH v10 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[jmeng]

From: Jakob Meng 

With the '--pretty' option, ovs-appctl will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys.

Signed-off-by: Jakob Meng 
---
 Documentation/ref/ovs-appctl.8.rst |  7 ++
 NEWS   |  1 +
 tests/ovs-vswitchd.at  |  5 +
 utilities/ovs-appctl.c | 34 --
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/Documentation/ref/ovs-appctl.8.rst 
b/Documentation/ref/ovs-appctl.8.rst
index 9619c1226..db6c52b42 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -79,6 +79,13 @@ In normal use only a single option is accepted:
 
   ``{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}``
 
+* ``--pretty``
+
+  By default, JSON output is printed as compactly as possible. This option
+  causes JSON in output to be printed in a more readable fashion. For example,
+  members of objects and elements of arrays are printed one per line, with
+  indentation. Requires ``--format=json``.
+
 Common Commands
 ===
 
diff --git a/NEWS b/NEWS
index 7076939c5..0d41061d3 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,7 @@ Post-v3.3.0
- ovs-appctl:
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+ * Added new option [--pretty] to print JSON output in a readable fashion.
- Python:
  * Added support for different output formats like 'json' to appctl.py and
Python's unixctl classes.
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 1ae7fcc32..2db8138f1 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -275,4 +275,9 @@ ovs_version=$(ovs-appctl version)
 AT_CHECK_UNQUOTED([ovs-appctl --format json version], [0], [dnl
 {"reply":"$ovs_version","reply-format":"plain"}])
 
+AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty version], [0], [dnl
+{
+  "reply": "$ovs_version",
+  "reply-format": "plain"}])
+
 AT_CLEANUP
diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
index 88f32c13a..68315e766 100644
--- a/utilities/ovs-appctl.c
+++ b/utilities/ovs-appctl.c
@@ -40,13 +40,15 @@ static void usage(void);
 /* Parsed command line args. */
 struct cmdl_args {
 enum unixctl_output_fmt format;
+unsigned int format_flags;
 char *target;
 };
 
 static struct cmdl_args *cmdl_args_create(void);
 static struct cmdl_args *parse_command_line(int argc, char *argv[]);
 static struct jsonrpc *connect_to_target(const char *target);
-static char * reply_to_string(struct json *reply, enum unixctl_output_fmt fmt);
+static char * reply_to_string(struct json *reply, enum unixctl_output_fmt fmt,
+  unsigned int fmt_flags);
 
 int
 main(int argc, char *argv[])
@@ -84,7 +86,7 @@ main(int argc, char *argv[])
 
 if (cmd_error) {
 jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
 ovs_error(0, "%s: server returned an error", args->target);
 exit(2);
@@ -107,12 +109,12 @@ main(int argc, char *argv[])
 
 if (cmd_error) {
 jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
 fputs(msg, stderr);
 ovs_error(0, "%s: server returned an error", args->target);
 exit(2);
 } else if (cmd_result) {
-msg = reply_to_string(cmd_result, args->format);
+msg = reply_to_string(cmd_result, args->format, args->format_flags);
 fputs(msg, stdout);
 } else {
 OVS_NOT_REACHED();
@@ -149,6 +151,8 @@ Other options:\n\
   --timeout=SECS wait at most SECS seconds for a response\n\
   -f, --format=FMT   Output format. One of: 'json', or 'text'\n\
  (default: text)\n\
+  --pretty   Format the output in a more readable fashion.\n\
+ Requires: --format=json.\n\
   -h, --help Print this helpful information\n\
   -V, --version  Display ovs-appctl version information\n",
program_name, program_name);
@@ -161,6 +165,7 @@ cmdl_args_create(void)
 struct cmdl_args *args = xmalloc(sizeof *args);
 
 args->format = UNIXCTL_OUTPUT_FMT_TEXT;
+args->format_flags = 0;
 args->target = NULL;
 
 return args;
@@ -171,7 +176,8 @@ parse_command_line(int argc, char *argv[])
 {
 enum {
 OPT_START = UCHAR_MAX + 1,
-VLOG_OPTION_ENUMS
+OPT_PRETTY,
+VLOG_OPTION_ENUMS,
 };
 static const struct option long_options[] = {
 {"target", required_argument, NULL, 't'},
@@ -179,6 +185,7 @@ parse_command_line(int argc, char *argv[])
 {"format", required_argument, NULL, 'f'},
 {"help", no_argument, NULL, 'h'},
  

Re: [ovs-dev] [PATCH ovn] northd: Add bfd and bfd_consumer nodes to I-P engine.

[Han Zhou]

On Thu, May 2, 2024 at 8:22 AM Lorenzo Bianconi 
wrote:
>
> Introduce bfd and bfd_consumer nodes to northd I-P engine to track bfd
> connections and northd static_route/policy_route changes.
>
> Reported-at: https://issues.redhat.com/browse/FDP-600
> Signed-off-by: Lorenzo Bianconi 

Hi Lorenzo,

Thanks for the patch. Could you explain in more detail about the motivation
and background information of this patch? I followed the link in
Reported-at but still not clear.
I assume it is for performance reasons, so could you explain what scenarios
would benefit from this?

For the patch, I haven't looked into details yet, but it seems you didn't
add any change handler for the two new nodes. So will there be follow up
patches for the real performance improvement?
But even for the current patch, with this: engine_add_input(_bfd,
_northd, NULL); does it mean any en_northd change would trigger a lflow
recompute because of the dependency en_northd -> en_bfd -> en_lflow. Would
this be a performance regression? If not, why?

For the noop handler used, please provide a detailed explanation to justify
it: why the node depends on the input but doesn't need to process when the
input changes?

Thanks,
Han

> ---
>  northd/en-lflow.c|  19 +--
>  northd/en-northd.c   |  92 +
>  northd/en-northd.h   |   8 ++
>  northd/inc-proc-northd.c |  21 ++-
>  northd/northd.c  | 274 ---
>  northd/northd.h  |  48 ++-
>  6 files changed, 344 insertions(+), 118 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index c4b927fb8..9efbd5117 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -41,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
>   struct lflow_input *lflow_input)
>  {
>  struct northd_data *northd_data = engine_get_input_data("northd",
node);
> +struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> +struct bfd_consumer_data *bfd_consumer_data =
> +engine_get_input_data("bfd_consumer", node);
>  struct port_group_data *pg_data =
>  engine_get_input_data("port_group", node);
>  struct sync_meters_data *sync_meters_data =
> @@ -78,7 +81,10 @@ lflow_get_input_data(struct engine_node *node,
>  lflow_input->meter_groups = _meters_data->meter_groups;
>  lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
>  lflow_input->svc_monitor_map = _data->svc_monitor_map;
> -lflow_input->bfd_connections = NULL;
> +lflow_input->bfd_connections = _data->bfd_connections;
> +lflow_input->parsed_routes = _consumer_data->parsed_routes;
> +lflow_input->route_tables = _consumer_data->route_tables;
> +lflow_input->route_policies = _consumer_data->route_policies;
>
>  struct ed_type_global_config *global_config =
>  engine_get_input_data("global_config", node);
> @@ -95,25 +101,14 @@ void en_lflow_run(struct engine_node *node, void
*data)
>  struct lflow_input lflow_input;
>  lflow_get_input_data(node, _input);
>
> -struct hmap bfd_connections = HMAP_INITIALIZER(_connections);
> -lflow_input.bfd_connections = _connections;
> -
>  stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
>  struct lflow_data *lflow_data = data;
>  lflow_table_clear(lflow_data->lflow_table);
>  lflow_reset_northd_refs(_input);
>
> -build_bfd_table(eng_ctx->ovnsb_idl_txn,
> -lflow_input.nbrec_bfd_table,
> -lflow_input.sbrec_bfd_table,
> -lflow_input.lr_ports,
> -_connections);
>  build_lflows(eng_ctx->ovnsb_idl_txn, _input,
>   lflow_data->lflow_table);
> -bfd_cleanup_connections(lflow_input.nbrec_bfd_table,
> -_connections);
> -hmap_destroy(_connections);
>  stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
>  engine_set_node_state(node, EN_UPDATED);
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 4479b4aff..2d8c05608 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -236,6 +236,66 @@ northd_global_config_handler(struct engine_node
*node, void *data OVS_UNUSED)
>  return true;
>  }
>
> +void
> +en_bfd_consumer_run(struct engine_node *node, void *data)
> +{
> +
> +struct northd_data *northd_data = engine_get_input_data("northd",
node);
> +struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> +const struct nbrec_bfd_table *nbrec_bfd_table =
> +EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> +struct bfd_consumer_data *bfd_consumer_data = data;
> +
> +bfd_consumer_destroy(data);
> +bfd_consumer_init(data);
> +
> +struct ovn_datapath *od;
> +HMAP_FOR_EACH (od, key_node, _data->lr_datapaths.datapaths) {
> +for (int i = 0; i < od->nbr->n_ports; i++) {
> +const char *route_table_name =
> +smap_get(>nbr->ports[i]->options, 

Re: [ovs-dev] [PATCH ovn] northd: Fix an issue wrt mac binding aging.

[Indrajitt Valsaraj]

Hi Ales,
Have addressed the comment on the new patch.

Thanks,
Indrajitt

On 16 May 2024, at 11:18 AM, Ales Musil  wrote:

CAUTION: External Email


On Thu, May 16, 2024 at 7:24 AM Indrajitt Valsaraj 
mailto:indrajitt.valsa...@nutanix.com>> wrote:
Issue:
In case of a Logical_Router without mac_binding_age_threshold set or a
Logical_Router with an incorrectly formatted mac_binding_threshold option,
entries were not being purged from the Mac Binding table in SouthBound.

This was because in the function `en_mac_binding_aging_run` in case of
an invalid mac_binding_threshold entry or if mac_binding_threshold is
not set we are returning from the loop instead of iterating through the
remaining LRs. As a result, subsequent runs of the aging_waker node are
also not scehduled and we end up not purging any MAC Bindings.

Fix:
This patch fixes this issue by changing the return to a continue so that
we skip the current LR but continue processing for the remaining LRs.

Fixes: 78851b6ffb58 ("northd: Support CIDR-based MAC binding aging threshold.")
Signed-off-by: Indrajitt Valsaraj 
mailto:indrajitt.valsa...@nutanix.com>>
Acked-by: Naveen Yerramneni 
mailto:naveen.yerramn...@nutanix.com>>
---

Hi Indrajitt,

thank you for the fix I have one comment down below.

 northd/aging.c |   2 +-
 tests/ovn.at 
[ovn.at]
   | 111 -
 2 files changed, 74 insertions(+), 39 deletions(-)

diff --git a/northd/aging.c b/northd/aging.c
index b76963a2d..9685044e7 100644
--- a/northd/aging.c
+++ b/northd/aging.c
@@ -421,7 +421,7 @@ en_mac_binding_aging_run(struct engine_node *node, void 
*data OVS_UNUSED)
 if (!parse_aging_threshold(smap_get(>nbr->options,
 "mac_binding_age_threshold"),
_config)) {
-return;
+continue;
 }

 aging_context_set_threshold(, _config);
diff --git a/tests/ovn.at 
[ovn.at]
 b/tests/ovn.at 
[ovn.at]
index 486680649..28a62d03d 100644
--- a/tests/ovn.at 
[ovn.at]
+++ b/tests/ovn.at 
[ovn.at]
@@ -34407,32 +34407,40 @@ ovn_start
 net_add n1

 AT_CHECK([ovn-nbctl ls-add public])
-AT_CHECK([ovn-nbctl ls-add internal])
+AT_CHECK([ovn-nbctl ls-add internal-1])

Why do we need to rename internal to internal-1? There is only 1 anyway. So it 
can very much remain just internal.


 AT_CHECK([ovn-nbctl lsp-add public ln_port])
 AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown])
 AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
 AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])

-AT_CHECK([ovn-nbctl lsp-add public public-gw])
-AT_CHECK([ovn-nbctl lsp-set-type public-gw router])
-AT_CHECK([ovn-nbctl lsp-set-addresses public-gw 00:00:00:00:10:00 router])
-AT_CHECK([ovn-nbctl lsp-set-options public-gw router-port=gw-public])
+AT_CHECK([ovn-nbctl lsp-add public public-gw-1])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-1 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-1 00:00:00:00:10:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-1 router-port=gw-1-public])

-AT_CHECK([ovn-nbctl lsp-add internal internal-gw])
-AT_CHECK([ovn-nbctl lsp-set-type internal-gw router])
-AT_CHECK([ovn-nbctl lsp-set-addresses internal-gw 00:00:00:00:20:00 router])
-AT_CHECK([ovn-nbctl lsp-set-options internal-gw router-port=gw-internal])
+AT_CHECK([ovn-nbctl lsp-add public public-gw-2])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-2 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-2 00:00:00:00:30:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-2 router-port=gw-2-public])

-AT_CHECK([ovn-nbctl lsp-add internal vif1])
+AT_CHECK([ovn-nbctl lsp-add internal-1 internal-gw-1])
+AT_CHECK([ovn-nbctl lsp-set-type internal-gw-1 

[ovs-dev] [PATCH ovn v1] northd: Fix an issue wrt mac binding aging.

[Indrajitt Valsaraj]

Issue:
In case of a Logical_Router without mac_binding_age_threshold set or a
Logical_Router with an incorrectly formatted mac_binding_threshold
option, entries were not being purged from the Mac Binding table in
SouthBound.

This was because in the function `en_mac_binding_aging_run` in case of
an invalid mac_binding_threshold entry or if mac_binding_threshold is
not set we are returning from the loop instead of iterating through the
remaining LRs. As a result, subsequent runs of the aging_waker node are
also not scehduled and we end up not purging any MAC Bindings.

Fix:
This patch fixes this issue by changing the return to a continue so that
we skip the current LR but continue processing for the remaining LRs.

Fixes: 78851b6ffb58 ("Support CIDR-based MAC binding aging threshold.")
Signed-off-by: Indrajitt Valsaraj 
Acked-by: Naveen Yerramneni 
---
 northd/aging.c |  2 +-
 tests/ovn.at   | 97 ++
 2 files changed, 67 insertions(+), 32 deletions(-)

diff --git a/northd/aging.c b/northd/aging.c
index b76963a2d..9685044e7 100644
--- a/northd/aging.c
+++ b/northd/aging.c
@@ -421,7 +421,7 @@ en_mac_binding_aging_run(struct engine_node *node, void 
*data OVS_UNUSED)
 if (!parse_aging_threshold(smap_get(>nbr->options,
 "mac_binding_age_threshold"),
_config)) {
-return;
+continue;
 }

 aging_context_set_threshold(, _config);
diff --git a/tests/ovn.at b/tests/ovn.at
index 486680649..8d5512bf2 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -34414,10 +34414,15 @@ AT_CHECK([ovn-nbctl lsp-set-addresses ln_port 
unknown])
 AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
 AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])

-AT_CHECK([ovn-nbctl lsp-add public public-gw])
-AT_CHECK([ovn-nbctl lsp-set-type public-gw router])
-AT_CHECK([ovn-nbctl lsp-set-addresses public-gw 00:00:00:00:10:00 router])
-AT_CHECK([ovn-nbctl lsp-set-options public-gw router-port=gw-public])
+AT_CHECK([ovn-nbctl lsp-add public public-gw-1])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-1 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-1 00:00:00:00:10:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-1 router-port=gw-1-public])
+
+AT_CHECK([ovn-nbctl lsp-add public public-gw-2])
+AT_CHECK([ovn-nbctl lsp-set-type public-gw-2 router])
+AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-2 00:00:00:00:30:00 router])
+AT_CHECK([ovn-nbctl lsp-set-options public-gw-2 router-port=gw-2-public])

 AT_CHECK([ovn-nbctl lsp-add internal internal-gw])
 AT_CHECK([ovn-nbctl lsp-set-type internal-gw router])
@@ -34430,9 +34435,12 @@ AT_CHECK([ovn-nbctl lsp-set-addresses vif1 
"00:00:00:00:20:10 192.168.20.10"])
 AT_CHECK([ovn-nbctl lsp-add internal vif2])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif2 "00:00:00:00:20:20 192.168.20.20"])

-AT_CHECK([ovn-nbctl lr-add gw])
-AT_CHECK([ovn-nbctl lrp-add gw gw-public 00:00:00:00:10:00 192.168.10.1/24])
-AT_CHECK([ovn-nbctl lrp-add gw gw-internal 00:00:00:00:20:00 192.168.20.1/24])
+AT_CHECK([ovn-nbctl lr-add gw-1])
+AT_CHECK([ovn-nbctl lrp-add gw-1 gw-1-public 00:00:00:00:10:00 
192.168.10.1/24])
+AT_CHECK([ovn-nbctl lrp-add gw-1 gw-internal 00:00:00:00:20:00 
192.168.20.1/24])
+
+AT_CHECK([ovn-nbctl lr-add gw-2])
+AT_CHECK([ovn-nbctl lrp-add gw-2 gw-2-public 00:00:00:00:30:00 
192.168.10.2/24])

 sim_add hv1
 as hv1
@@ -34500,21 +34508,27 @@ send_udp() {
 as $hv ovs-appctl netdev-dummy/receive $dev $packet
 }
 # Check if the option is not present by default
-AT_CHECK([fetch_column nb:logical_router options name="gw" | grep -q 
mac_binding_age_threshold], [1])
+AT_CHECK([fetch_column nb:logical_router options name="gw-1" | grep -q 
mac_binding_age_threshold], [1])
+AT_CHECK([fetch_column nb:logical_router options name="gw-2" | grep -q 
mac_binding_age_threshold], [1])

 # Send GARP to populate MAC binding table records
 send_garp hv1 ext1 10
 send_garp hv2 ext2 20

-wait_row_count mac_binding 1 ip="192.168.10.10"
-wait_row_count mac_binding 1 ip="192.168.10.20"
+# Two rows present for each IP, one corresponding to each logical_port
+wait_row_count mac_binding 2 ip="192.168.10.10"
+wait_row_count mac_binding 2 ip="192.168.10.20"

-dp_key=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key 
external_ids:name=gw))
-port_key=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key 
logical_port=gw-public))
+dp_key_1=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key 
external_ids:name=gw-1))
+port_key_1=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key 
logical_port=gw-1-public))
+dp_key_2=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key 
external_ids:name=gw-2))
+port_key_2=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key 
logical_port=gw-2-public))

 AT_CHECK_UNQUOTED([as hv1 ovs-ofctl dump-flows br-int 
table=OFTABLE_MAC_CACHE_USE --no-stats | strip_cookie