On Tue, Aug 23, 2022 at 3:44 AM Xavier Simonart <[email protected]> wrote:
>
> If a logical switch port is added and connected to a logical router
> port (through options: router-port) before the router port is
> created, then this might cause further issues such as segmentation
> violation when the switch and router ports are deleted.
>
> Signed-off-by: Xavier Simonart <[email protected]>
Thanks Xavier. Great catch! Please see minor comments below.
> ---
> controller/local_data.c | 30 +++++++++++++-----------------
> tests/ovn.at | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 7f874fc19..c5a948702 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -194,17 +194,7 @@ add_local_datapath_peer_port(
> return;
> }
>
> - bool present = false;
> - for (size_t i = 0; i < ld->n_peer_ports; i++) {
> - if (ld->peer_ports[i].local == pb) {
> - present = true;
> - break;
> - }
> - }
> -
> - if (!present) {
> - local_datapath_peer_port_add(ld, pb, peer);
> - }
> + local_datapath_peer_port_add(ld, pb, peer);
>
> struct local_datapath *peer_ld =
> get_local_datapath(local_datapaths,
> @@ -218,12 +208,6 @@ add_local_datapath_peer_port(
> return;
> }
>
> - for (size_t i = 0; i < peer_ld->n_peer_ports; i++) {
> - if (peer_ld->peer_ports[i].local == peer) {
> - return;
> - }
> - }
> -
> local_datapath_peer_port_add(peer_ld, peer, pb);
> }
>
> @@ -597,6 +581,13 @@ add_local_datapath__(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
> tracked_datapaths);
> }
> local_datapath_peer_port_add(ld, pb, peer);
> + struct local_datapath *peer_ld =
> + get_local_datapath(local_datapaths,
> + peer->datapath->tunnel_key);
> +
Since we just added the peer datapath to local_datapaths several lines
above, it seems really a waste to look it up again. It would be nice to
return the *ld* newly created (or existed one) from this function
add_local_datapath__() directly.
> + if (peer_ld != NULL) {
nit: the style of this project is usually "if (peer_ld) ...".
With the above:
Acked-by: Han Zhou <[email protected]>
> + local_datapath_peer_port_add(peer_ld, peer, pb);
> + }
> }
> }
> }
> @@ -622,6 +613,11 @@ local_datapath_peer_port_add(struct local_datapath
*ld,
> const struct sbrec_port_binding *local,
> const struct sbrec_port_binding *remote)
> {
> + for (size_t i = 0; i < ld->n_peer_ports; i++) {
> + if (ld->peer_ports[i].local == local) {
> + return;
> + }
> + }
> ld->n_peer_ports++;
> if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> size_t old_n_ports = ld->n_allocated_peer_ports;
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bba2c9c1d..ae0918d55 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -32580,3 +32580,33 @@ check ovn-nbctl --wait=hv sync
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([router port add then remove - lsp first])
> +ovn_start
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> + options:tx_pcap=hv1/vif1-tx.pcap \
> + options:rxq_pcap=hv1/vif1-rx.pcap \
> + ofport-request=1
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl lr-add ro0
> +check ovn-nbctl lsp-add sw0 sw0-p1
> +check ovn-nbctl lsp-add sw0 lsp
> +check ovn-nbctl lsp-set-type lsp router
> +check ovn-nbctl lsp-set-options lsp router-port=lrp
> +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1
> +check ovn-nbctl --wait=hv lrp-add ro0 lrp 00:00:00:00:00:1
aef0:0:0:0:0:0:0:1/64
> +check ovn-nbctl --wait=hv lsp-del lsp
> +check ovn-nbctl lrp-del lrp
> +check ovn-nbctl --wait=hv sync
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
