Re: [ovs-dev] [PATCH ovn v4] controller: Track individual address set constants.
On Mon, May 6, 2024 at 8:41 PM Han Zhou wrote:
>
>
> On Thu, May 2, 2024 at 10:35 PM Ales Musil wrote:
> >
> > On Thu, May 2, 2024 at 6:23 PM Han Zhou wrote:
> > >
> > >
> > >
> > > On Thu, May 2, 2024 at 6:29 AM Ales Musil wrote:
> > > >
> > > > Instead of tracking address set per struct expr_constant_set track it
> > > > per individual struct expr_constant. This allows more fine grained
> > > > control for I-P processing of address sets in controller. It helps
> with
> > > > scenarios like matching on two address sets in one expression e.g.
> > > > "ip4.src == {$as1, $as2}". This allows any addition or removal of
> > > > individual adress from the set to be incrementally processed instead
> > > > of reprocessing all the flows.
> > > >
> > > > This unfortunately doesn't help with the following flows:
> > > > "ip4.src == $as1 && ip4.dst == $as2"
> > > > "ip4.src == $as1 || ip4.dst == $as2"
> > > >
> > > > The memory impact should be minimal as there is only increase of 8
> bytes
> > > > per the struct expr_constant.
> > > >
> > > > Reported-at: https://issues.redhat.com/browse/FDP-509
> > > > Signed-off-by: Ales Musil
> > > > ---
> > > > v4: Rebase on top of current main.
> > > > Update the "lflow_handle_addr_set_update" comment according to
> suggestion from Han.
> > >
> > > Thanks Ales. I updated the commit message for the same, and applied to
> main branch.
> > >
> > > Regards,
> > > Han
> > >
> > > > v3: Rebase on top of current main.
> > > > Address comments from Han:
> > > > - Adjust the comment for "lflow_handle_addr_set_update" to
> include remaning corner cases.
> > > > - Make sure that the flows are consistent between I-P and
> recompute.
> > > > v2: Rebase on top of current main.
> > > > Adjust the comment for I-P optimization.
> > > > ---
> > > > controller/lflow.c | 11 ++---
> > > > include/ovn/actions.h | 2 +-
> > > > include/ovn/expr.h | 46 ++-
> > > > lib/actions.c | 20 -
> > > > lib/expr.c | 99
> +
> > > > tests/ovn-controller.at | 79 +---
> > > > 6 files changed, 154 insertions(+), 103 deletions(-)
> > > >
> > > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > > index 760ec0b41..1e05665a1 100644
> > > > --- a/controller/lflow.c
> > > > +++ b/controller/lflow.c
> > > > @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
> > > > }
> > > >
> > > > static bool
> > > > -as_info_from_expr_const(const char *as_name, const union
> expr_constant *c,
> > > > +as_info_from_expr_const(const char *as_name, const struct
> expr_constant *c,
> > > > struct addrset_info *as_info)
> > > > {
> > > > as_info->name = as_name;
> > > > @@ -644,14 +644,11 @@ as_update_can_be_handled(const char *as_name,
> struct addr_set_diff *as_diff,
> > > > *generated.
> > > > *
> > > > * - The sub expression of the address set is combined with
> other sub-
> > > > - *expressions/constants, usually because of disjunctions
> between
> > > > - *sub-expressions/constants, e.g.:
> > > > + *expressions/constants on different fields, e.g.:
> > > > *
> > > > * ip.src == $as1 || ip.dst == $as2
> > > > - * ip.src == {$as1, $as2}
> > > > - * ip.src == {$as1, ip1}
> > > > *
> > > > - *All these could have been split into separate lflows.
> > > > + *This could have been split into separate lflows.
> > > > *
> > > > * - Conjunctions overlapping between lflows, which can be
> caused by
> > > > *overlapping address sets or same address set used by
> multiple lflows
> > > > @@ -714,7 +711,7 @@ lflow_handle_addr_set_update(const char *as_name,
> > > > if (as_diff->deleted) {
> > > > struct addrset_info as_info;
> > > > for (size_t i = 0; i < as_diff->deleted->n_values; i++)
> {
> > > > -union expr_constant *c =
> _diff->deleted->values[i];
> > > > +struct expr_constant *c =
> _diff->deleted->values[i];
> > > > if (!as_info_from_expr_const(as_name, c, _info))
> {
> > > > continue;
> > > > }
> > > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > > > index ae0864fdd..88cf4de79 100644
> > > > --- a/include/ovn/actions.h
> > > > +++ b/include/ovn/actions.h
> > > > @@ -241,7 +241,7 @@ struct ovnact_next {
> > > > struct ovnact_load {
> > > > struct ovnact ovnact;
> > > > struct expr_field dst;
> > > > -union expr_constant imm;
> > > > +struct expr_constant imm;
> > > > };
> > > >
> > > > /* OVNACT_MOVE, OVNACT_EXCHANGE. */
> > > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > > > index c48f82398..e54edb5bf 100644
> > > > --- a/include/ovn/expr.h
> > > > +++ b/include/ovn/expr.h
> > > > @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum
Re: [ovs-dev] [PATCH ovn v2] northd: Support routing over other address families.
On Mon, Apr 22, 2024 at 9:02 AM Felix Huettner via dev
wrote:
>
> In most cases IPv4 packets are routed only over other IPv4 networks and
> IPv6 packets are routed only over IPv6 networks. However there is no
> inherent reason for this limitation. Routing IPv4 packets over IPv6
> networks just requires the router to contain a route for an IPv4 network
> with an IPv6 nexthop.
>
> This was previously prevented in OVN in ovn-nbctl and northd. By
> removing these filters the forwarding will work if the mac addresses are
> prepopulated.
>
> If the mac addresses are not prepopulated we will attempt to resolve them
> using
> the original address family of the packet and not the address family of the
> nexthop. This will fail and we will not forward the packet.
>
Thanks for adding this feature.
If I understand correctly from the above commit message about
prepopulation, is it expected that
CMS/admin needs to add a static mac binding entry in the OVN
Northbound 'Static_MAC_Binding' table
for this feature to work ? Please correct me if I'm wrong.
If this is the case, then it needs to be documented properly (perhaps
in ovn-nb.xml)
The patch overall looks good to me. Can you please add a small test
case in ovn-northd.at to check
the expected logical flows in the "lr_in_ip_routing" stage when you
have these mixed mode static routes ?
You can also check for correctness of flows in other tables too which
are generated for the static routes.
Thanks
Numan
> This feature can for example be used by service providers to
> interconnect multiple IPv4 networks of a customer without needing to
> negotiate free IPv4 addresses by just using any IPv6 address.
>
> Signed-off-by: Felix Huettner
> ---
> v1->v2:
> - move ipv4 info to parsed_route
> - add tests for lr-route-add
> - switch tests to use fmt_pkt
> - some minor test cleanups
>
> NEWS | 4 +
> northd/northd.c | 56 ++---
> tests/ovn-nbctl.at| 26 ++-
> tests/ovn.at | 511 ++
> utilities/ovn-nbctl.c | 12 +-
> 5 files changed, 570 insertions(+), 39 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 141f1831c..14a935c86 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -13,6 +13,10 @@ Post v24.03.0
> "lflow-stage-to-oftable STAGE_NAME" that converts stage name into
> OpenFlow
> table id.
>- Rename the ovs-sandbox script to ovn-sandbox.
> + - Allow Static Routes where the address families of ip_prefix and nexthop
> +diverge (e.g. IPv4 packets over IPv6 links). This is currently limited to
> +nexthops that have their mac addresses prepopulated (so
> +dynamic_neigh_routers must be false).
>
> OVN v24.03.0 - 01 Mar 2024
> --
> diff --git a/northd/northd.c b/northd/northd.c
> index 331d9c267..e377d3e18 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10194,6 +10194,8 @@ struct parsed_route {
> const struct nbrec_logical_router_static_route *route;
> bool ecmp_symmetric_reply;
> bool is_discard_route;
> +bool is_ipv4_prefix;
> +bool is_ipv4_nexthop;
> };
>
> static uint32_t
> @@ -10219,6 +10221,7 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> /* Verify that the next hop is an IP address with an all-ones mask. */
> struct in6_addr nexthop;
> unsigned int plen;
> +bool is_ipv4_nexthop, is_ipv4_prefix;
> bool is_discard_route = !strcmp(route->nexthop, "discard");
> bool valid_nexthop = route->nexthop[0] && !is_discard_route;
> if (valid_nexthop) {
> @@ -10237,6 +10240,7 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> UUID_ARGS(>header_.uuid));
> return NULL;
> }
> +is_ipv4_nexthop = IN6_IS_ADDR_V4MAPPED();
> }
>
> /* Parse ip_prefix */
> @@ -10248,18 +10252,7 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> UUID_ARGS(>header_.uuid));
> return NULL;
> }
> -
> -/* Verify that ip_prefix and nexthop have same address familiy. */
> -if (valid_nexthop) {
> -if (IN6_IS_ADDR_V4MAPPED() != IN6_IS_ADDR_V4MAPPED())
> {
> -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -VLOG_WARN_RL(, "Address family doesn't match between
> 'ip_prefix'"
> - " %s and 'nexthop' %s in static route "UUID_FMT,
> - route->ip_prefix, route->nexthop,
> - UUID_ARGS(>header_.uuid));
> -return NULL;
> -}
> -}
> +is_ipv4_prefix = IN6_IS_ADDR_V4MAPPED();
>
> /* Verify that ip_prefix and nexthop are on the same network. */
> if (!is_discard_route &&
> @@ -10302,6 +10295,8 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> pr->ecmp_symmetric_reply = smap_get_bool(>options,
>
Re: [ovs-dev] [PATCH v2] selftests/net: fix uninitialized variables
John Hubbard wrote: > When building with clang, via: > > make LLVM=1 -C tools/testing/selftest > > ...clang warns about three variables that are not initialized in all > cases: > > 1) The opt_ipproto_off variable is used uninitialized if "testname" is > not "ip". Willem de Bruijn pointed out that this is an actual bug, and > suggested the fix that I'm using here (thanks!). > > 2) The addr_len is used uninitialized, but only in the assert case, >which bails out, so this is harmless. > > 3) The family variable in add_listener() is only used uninitialized in >the error case (neither IPv4 nor IPv6 is specified), so it's also >harmless. > > Fix by initializing each variable. > > Cc: Willem de Bruijn > Signed-off-by: John Hubbard Reviewed-by: Willem de Bruijn Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] selftests/net: fix uninitialized variables
Bleep bloop. Greetings John Hubbard, 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. git-am: error: sha1 information is lacking or useless (tools/testing/selftests/net/gro.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 selftests/net: fix uninitialized variables When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. 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] selftests/net: fix uninitialized variables
When building with clang, via:
make LLVM=1 -C tools/testing/selftest
...clang warns about three variables that are not initialized in all
cases:
1) The opt_ipproto_off variable is used uninitialized if "testname" is
not "ip". Willem de Bruijn pointed out that this is an actual bug, and
suggested the fix that I'm using here (thanks!).
2) The addr_len is used uninitialized, but only in the assert case,
which bails out, so this is harmless.
3) The family variable in add_listener() is only used uninitialized in
the error case (neither IPv4 nor IPv6 is specified), so it's also
harmless.
Fix by initializing each variable.
Cc: Willem de Bruijn
Signed-off-by: John Hubbard
---
tools/testing/selftests/net/gro.c | 3 +++
tools/testing/selftests/net/ip_local_port_range.c | 2 +-
tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 2 +-
3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/gro.c
b/tools/testing/selftests/net/gro.c
index 353e1e867fbb..6038b96ecee8 100644
--- a/tools/testing/selftests/net/gro.c
+++ b/tools/testing/selftests/net/gro.c
@@ -119,6 +119,9 @@ static void setup_sock_filter(int fd)
next_off = offsetof(struct ipv6hdr, nexthdr);
ipproto_off = ETH_HLEN + next_off;
+ /* Overridden later if exthdrs are used: */
+ opt_ipproto_off = ipproto_off;
+
if (strcmp(testname, "ip") == 0) {
if (proto == PF_INET)
optlen = sizeof(struct ip_timestamp);
diff --git a/tools/testing/selftests/net/ip_local_port_range.c
b/tools/testing/selftests/net/ip_local_port_range.c
index 193b82745fd8..29451d2244b7 100644
--- a/tools/testing/selftests/net/ip_local_port_range.c
+++ b/tools/testing/selftests/net/ip_local_port_range.c
@@ -359,7 +359,7 @@ TEST_F(ip_local_port_range, late_bind)
struct sockaddr_in v4;
struct sockaddr_in6 v6;
} addr;
- socklen_t addr_len;
+ socklen_t addr_len = 0;
const int one = 1;
int fd, err;
__u32 range;
diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 7426a2cbd4a0..7ad5a59adff2 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -1276,7 +1276,7 @@ int add_listener(int argc, char *argv[])
struct sockaddr_storage addr;
struct sockaddr_in6 *a6;
struct sockaddr_in *a4;
- u_int16_t family;
+ u_int16_t family = AF_UNSPEC;
int enable = 1;
int sock;
int err;
base-commit: f462ae0edd3703edd6f22fe41d336369c38b884b
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
prerequisite-patch-id: e81ae5ca6c427dde802acd4c1442c82e170c251a
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] selftests/net: fix uninitialized variables
John Hubbard wrote: > When building with clang, via: > > make LLVM=1 -C tools/testing/selftest > > ...clang warns about three variables that are not initialized in all > cases: > > 1) The opt_ipproto_off variable is used uninitialized if "testname" is > not "ip". This seems like an actual bug. > > 2) The addr_len is used uninitialized, but only in the assert case, >which bails out, so this is harmless. > > 3) The family variable in add_listener() is only used uninitialized in >the error case (neither IPv4 nor IPv6 is specified), so it's also >harmless. > > Fix by initializing each variable. > > Signed-off-by: John Hubbard > --- > tools/testing/selftests/net/gro.c | 3 ++- > tools/testing/selftests/net/ip_local_port_range.c | 2 +- > tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 2 +- > 3 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/net/gro.c > b/tools/testing/selftests/net/gro.c > index 353e1e867fbb..0eb61edaad83 100644 > --- a/tools/testing/selftests/net/gro.c > +++ b/tools/testing/selftests/net/gro.c > @@ -110,7 +110,8 @@ static void setup_sock_filter(int fd) > const int dport_off = tcp_offset + offsetof(struct tcphdr, dest); > const int ethproto_off = offsetof(struct ethhdr, h_proto); > int optlen = 0; > - int ipproto_off, opt_ipproto_off; > + int ipproto_off; > + int opt_ipproto_off = 0; This is only intended to be used in the case where the IP proto is not TCP: BPF_STMT(BPF_LD + BPF_B + BPF_ABS, ipproto_off), + BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 2, 0), + BPF_STMT(BPF_LD + BPF_B + BPF_ABS, opt_ipproto_off), BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 0, 5), In that case the test tries again at a different offset that accounts for optional IPv6 extension headers. This is indeed buggy, in that it might accidentally accept packets that should be dropped. Initializing to 0 compares against against the first byte of the Ethernet header. Which is an external argument to the test. So safest is to initialize opt_ipproto_off to ipproto_off and just repeat the previous check. Perhaps: @@ -118,6 +118,7 @@ static void setup_sock_filter(int fd) else next_off = offsetof(struct ipv6hdr, nexthdr); ipproto_off = ETH_HLEN + next_off; + opt_ipproto_off = ipproto_off; /* overridden later if may have exthdrs */ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] selftests/net: fix uninitialized variables
On 5/6/24 11:00 AM, Willem de Bruijn wrote: John Hubbard wrote: ... diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c index 353e1e867fbb..0eb61edaad83 100644 --- a/tools/testing/selftests/net/gro.c +++ b/tools/testing/selftests/net/gro.c @@ -110,7 +110,8 @@ static void setup_sock_filter(int fd) const int dport_off = tcp_offset + offsetof(struct tcphdr, dest); const int ethproto_off = offsetof(struct ethhdr, h_proto); int optlen = 0; - int ipproto_off, opt_ipproto_off; + int ipproto_off; + int opt_ipproto_off = 0; This is only intended to be used in the case where the IP proto is not TCP: BPF_STMT(BPF_LD + BPF_B + BPF_ABS, ipproto_off), + BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 2, 0), + BPF_STMT(BPF_LD + BPF_B + BPF_ABS, opt_ipproto_off), BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 0, 5), In that case the test tries again at a different offset that accounts for optional IPv6 extension headers. This is indeed buggy, in that it might accidentally accept packets that should be dropped. Initializing to 0 compares against against the first byte of the Ethernet header. Which is an external argument to the test. So safest is to initialize opt_ipproto_off to ipproto_off and just repeat the previous check. Perhaps: @@ -118,6 +118,7 @@ static void setup_sock_filter(int fd) else next_off = offsetof(struct ipv6hdr, nexthdr); ipproto_off = ETH_HLEN + next_off; + opt_ipproto_off = ipproto_off; /* overridden later if may have exthdrs */ OK, thanks for pointing out the right fix, I'll send a v2 that does that. thanks, -- John Hubbard NVIDIA ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v4] controller: Track individual address set constants.
On Thu, May 2, 2024 at 10:35 PM Ales Musil wrote:
>
> On Thu, May 2, 2024 at 6:23 PM Han Zhou wrote:
> >
> >
> >
> > On Thu, May 2, 2024 at 6:29 AM Ales Musil wrote:
> > >
> > > Instead of tracking address set per struct expr_constant_set track it
> > > per individual struct expr_constant. This allows more fine grained
> > > control for I-P processing of address sets in controller. It helps
with
> > > scenarios like matching on two address sets in one expression e.g.
> > > "ip4.src == {$as1, $as2}". This allows any addition or removal of
> > > individual adress from the set to be incrementally processed instead
> > > of reprocessing all the flows.
> > >
> > > This unfortunately doesn't help with the following flows:
> > > "ip4.src == $as1 && ip4.dst == $as2"
> > > "ip4.src == $as1 || ip4.dst == $as2"
> > >
> > > The memory impact should be minimal as there is only increase of 8
bytes
> > > per the struct expr_constant.
> > >
> > > Reported-at: https://issues.redhat.com/browse/FDP-509
> > > Signed-off-by: Ales Musil
> > > ---
> > > v4: Rebase on top of current main.
> > > Update the "lflow_handle_addr_set_update" comment according to
suggestion from Han.
> >
> > Thanks Ales. I updated the commit message for the same, and applied to
main branch.
> >
> > Regards,
> > Han
> >
> > > v3: Rebase on top of current main.
> > > Address comments from Han:
> > > - Adjust the comment for "lflow_handle_addr_set_update" to
include remaning corner cases.
> > > - Make sure that the flows are consistent between I-P and
recompute.
> > > v2: Rebase on top of current main.
> > > Adjust the comment for I-P optimization.
> > > ---
> > > controller/lflow.c | 11 ++---
> > > include/ovn/actions.h | 2 +-
> > > include/ovn/expr.h | 46 ++-
> > > lib/actions.c | 20 -
> > > lib/expr.c | 99
+
> > > tests/ovn-controller.at | 79 +---
> > > 6 files changed, 154 insertions(+), 103 deletions(-)
> > >
> > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > index 760ec0b41..1e05665a1 100644
> > > --- a/controller/lflow.c
> > > +++ b/controller/lflow.c
> > > @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
*l_ctx_in,
> > > }
> > >
> > > static bool
> > > -as_info_from_expr_const(const char *as_name, const union
expr_constant *c,
> > > +as_info_from_expr_const(const char *as_name, const struct
expr_constant *c,
> > > struct addrset_info *as_info)
> > > {
> > > as_info->name = as_name;
> > > @@ -644,14 +644,11 @@ as_update_can_be_handled(const char *as_name,
struct addr_set_diff *as_diff,
> > > *generated.
> > > *
> > > * - The sub expression of the address set is combined with
other sub-
> > > - *expressions/constants, usually because of disjunctions
between
> > > - *sub-expressions/constants, e.g.:
> > > + *expressions/constants on different fields, e.g.:
> > > *
> > > * ip.src == $as1 || ip.dst == $as2
> > > - * ip.src == {$as1, $as2}
> > > - * ip.src == {$as1, ip1}
> > > *
> > > - *All these could have been split into separate lflows.
> > > + *This could have been split into separate lflows.
> > > *
> > > * - Conjunctions overlapping between lflows, which can be
caused by
> > > *overlapping address sets or same address set used by
multiple lflows
> > > @@ -714,7 +711,7 @@ lflow_handle_addr_set_update(const char *as_name,
> > > if (as_diff->deleted) {
> > > struct addrset_info as_info;
> > > for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> > > -union expr_constant *c =
_diff->deleted->values[i];
> > > +struct expr_constant *c =
_diff->deleted->values[i];
> > > if (!as_info_from_expr_const(as_name, c, _info)) {
> > > continue;
> > > }
> > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > > index ae0864fdd..88cf4de79 100644
> > > --- a/include/ovn/actions.h
> > > +++ b/include/ovn/actions.h
> > > @@ -241,7 +241,7 @@ struct ovnact_next {
> > > struct ovnact_load {
> > > struct ovnact ovnact;
> > > struct expr_field dst;
> > > -union expr_constant imm;
> > > +struct expr_constant imm;
> > > };
> > >
> > > /* OVNACT_MOVE, OVNACT_EXCHANGE. */
> > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > > index c48f82398..e54edb5bf 100644
> > > --- a/include/ovn/expr.h
> > > +++ b/include/ovn/expr.h
> > > @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type,
enum expr_relop *relop);
> > > struct expr {
> > > struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR
if any. */
> > > enum expr_type type;/* Expression type. */
> > > -char *as_name; /* Address set name. Null if it is
not an
> > > +const
[ovs-dev] [PATCH v2 2/2] python: ovsdb-idl: Use monitor_cond for _Server DB.
Unlike the C IDL code, the Python version still monitors the
_Server DB with "monitor" instead of "monitor_cond". This results
in receiving an entire Database row every time the "index" value
is updated which includes the 'schema' column. Using "monitor_cond"
will result in "update2" notifications which just include the
changed "index" value.
Unlike the C IDL, the Python IDL requires a SchemaHelper object
to instanitate the IDL, leaving it to the user of the library to
call "get_schema" themselves. Since the Python IDL did not have
support for retrieving the schema automatically and did not have
a state for doing so, instead of transitioning on an error response
from retrieving the _Server schema to requesting the "data" schema,
this moves directly to monitoring the "data" DB.
Signed-off-by: Terry Wilson
---
python/ovs/db/idl.py | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index a80da84e7..c1341fc2a 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -35,9 +35,9 @@ ROW_CREATE = "create"
ROW_UPDATE = "update"
ROW_DELETE = "delete"
-OVSDB_UPDATE = 0
-OVSDB_UPDATE2 = 1
-OVSDB_UPDATE3 = 2
+OVSDB_UPDATE = "update"
+OVSDB_UPDATE2 = "update2"
+OVSDB_UPDATE3 = "update3"
CLUSTERED = "clustered"
RELAY = "relay"
@@ -77,7 +77,7 @@ class ColumnDefaultDict(dict):
return item in self.keys()
-class Monitor(enum.IntEnum):
+class Monitor(enum.Enum):
monitor = OVSDB_UPDATE
monitor_cond = OVSDB_UPDATE2
monitor_cond_since = OVSDB_UPDATE3
@@ -465,23 +465,18 @@ class Idl(object):
self.__parse_update(msg.params[2], OVSDB_UPDATE3)
self.last_id = msg.params[1]
elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
-and msg.method == "update2"
-and len(msg.params) == 2):
-# Database contents changed.
-self.__parse_update(msg.params[1], OVSDB_UPDATE2)
-elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
-and msg.method == "update"
+and msg.method in (OVSDB_UPDATE, OVSDB_UPDATE2)
and len(msg.params) == 2):
# Database contents changed.
if msg.params[0] == str(self.server_monitor_uuid):
-self.__parse_update(msg.params[1], OVSDB_UPDATE,
+self.__parse_update(msg.params[1], msg.method,
tables=self.server_tables)
self.change_seqno = previous_change_seqno
if not self.__check_server_db():
self.force_reconnect()
break
else:
-self.__parse_update(msg.params[1], OVSDB_UPDATE)
+self.__parse_update(msg.params[1], msg.method)
elif self.handle_monitor_canceled(msg):
break
elif self.handle_monitor_cancel_reply(msg):
@@ -540,7 +535,7 @@ class Idl(object):
# Reply to our "monitor" of _Server request.
try:
self._server_monitor_request_id = None
-self.__parse_update(msg.result, OVSDB_UPDATE,
+self.__parse_update(msg.result, OVSDB_UPDATE2,
tables=self.server_tables)
self.change_seqno = previous_change_seqno
if self.__check_server_db():
@@ -579,6 +574,11 @@ class Idl(object):
elif msg.type == ovs.jsonrpc.Message.T_NOTIFY and msg.id == "echo":
# Reply to our echo request. Ignore it.
pass
+elif (msg.type == ovs.jsonrpc.Message.T_ERROR and
+ self.state == self.IDL_S_SERVER_MONITOR_REQUESTED and
+ msg.id == self._server_monitor_request_id):
+self._server_monitor_request_id = None
+self.__send_monitor_request()
elif (msg.type == ovs.jsonrpc.Message.T_ERROR and
self.state == (
self.IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED) and
@@ -912,7 +912,7 @@ class Idl(object):
monitor_request = {"columns": columns}
monitor_requests[table.name] = [monitor_request]
msg = ovs.jsonrpc.Message.create_request(
-'monitor', [self._server_db.name,
+'monitor_cond', [self._server_db.name,
str(self.server_monitor_uuid),
monitor_requests])
self._server_monitor_request_id = msg.id
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 1/2] ovsdb-idl: Add C IDL test for "monitor" fallback.
There was a Python-only test for ensuring that the library would work when connecting to an older ovsdb-server that did not support monitor_cond. This adds a C IDL version of that test. Signed-off-by: Terry Wilson --- tests/ovsdb-idl.at | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index c9e36d678..97162707e 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -1119,6 +1119,19 @@ OVSDB_CHECK_IDL_FETCH_COLUMNS([simple idl, initially populated], 003: done ]]) +m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND_C], + [AT_SETUP([$1 - C]) + AT_KEYWORDS([ovsdb server idl monitor $4]) + OVSDB_START_IDLTEST + AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/disable-monitor-cond]) + + AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl unix:socket $2], +[0], [stdout], [ignore]) + AT_CHECK([sort stdout | uuidfilt]m4_if([$5],,, [[| $5]]), +[0], [$3]) + OVSDB_SERVER_SHUTDOWN + AT_CLEANUP]) + m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND_PY], [AT_SETUP([$1 - Python3]) AT_KEYWORDS([ovsdb server idl Python monitor $4]) @@ -1132,7 +1145,8 @@ m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND_PY], AT_CLEANUP]) m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND], - [OVSDB_CHECK_IDL_WO_MONITOR_COND_PY($@)]) + [OVSDB_CHECK_IDL_WO_MONITOR_COND_C($@) +OVSDB_CHECK_IDL_WO_MONITOR_COND_PY($@)]) OVSDB_CHECK_IDL_WO_MONITOR_COND([simple idl disable monitor-cond], -- 2.34.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/2] python: ovsdb-idl Use monitor_cond for _Server DB.
Using "monitor" for _Server means that every bump to Database.index sends a large "update" notification containing a lot of data including the Database schema text. This updates the Python IDL to use "monitor_cond" as the C IDL does. There is a difference in logic between the two IDL implementations. Python IDL's Idl takes a SchemaHelper object as a required object for instantiation, while the C IDL will transition through calling "get_schema" on first the _Server DB and data DB if that errors out. Although the Python version calls get_schema for _Server, it has never called it on data, presumably because the schema is passed at instantiation. This implementation instead of adding support for getting and using the schema via "get_schema" on the data db, transitions from an error retreiving the schema from _Server to directly monitoring the data DB (since we already were passed a schema at instantiation). It would be good to, in the future, add support for not having to specify the schema at Idl creation, but that kind of API change should be done on its own (hopefully in a backward-compatible way). Terry Wilson (2): ovsdb-idl: Add C IDL test for "monitor" fallback python: ovsdb-idl: Use monitor_cond for _Server DB python/ovs/db/idl.py | 28 ++-- tests/ovsdb-idl.at | 16 +++- 2 files changed, 29 insertions(+), 15 deletions(-) -- 2.34.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 0/4] Mac cache handling refactor
On Tue, Apr 23, 2024 at 3:44 PM Mark Michelson wrote: > > Hi Ales, thanks for this. I always like a change that results in a net > loss of lines of code :) > > For the series, > > Acked-by: Mark Michelson Hi Ales, Can you please rebase the series ? It has some conflicts. Numan > > On 4/22/24 03:35, Ales Musil wrote: > > There were two modules in controller mac_cache and mac-learn, both of > > them did very similar thing with pretty big overlap. The goal of the > > series is to consolidate and merge both of those modules into single > > one. That will reduce the duplication and should make it easier for > > future updates to MAC binding, FDB or packet buffering functionality. > > > > There is also fix to properly handle tunnel_key change for LSP, LRP, > > LR and LS. This was inconsistent and could lead to wrong flows being > > still present even after the tunnel key change. This is not a huge > > issue because the tunnel_key is rarelyt changed during runtime. > > > > Ales Musil (4): > >northd, controller: Handle tunnel_key change consistently. > >controller: Rename mac_cache to to mac-cache. > >controller: Merge the mac-cache and mac-learn. > >controller: Use datapath key for the mac cache thresholds. > > > > controller/automake.mk | 6 +- > > controller/binding.c| 13 +- > > controller/mac-cache.c | 745 > > controller/mac-cache.h | 210 ++ > > controller/mac-learn.c | 482 --- > > controller/mac-learn.h | 145 --- > > controller/mac_cache.c | 547 -- > > controller/mac_cache.h | 124 -- > > controller/ovn-controller.c | 214 +++ > > controller/pinctrl.c| 165 > > controller/statctrl.c | 7 +- > > controller/statctrl.h | 2 +- > > northd/northd.c | 7 + > > tests/ovn.at| 56 ++- > > 14 files changed, 1253 insertions(+), 1470 deletions(-) > > create mode 100644 controller/mac-cache.c > > create mode 100644 controller/mac-cache.h > > delete mode 100644 controller/mac-learn.c > > delete mode 100644 controller/mac-learn.h > > delete mode 100644 controller/mac_cache.c > > delete mode 100644 controller/mac_cache.h > > > > ___ > 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 2/2] python: ovsdb-idl: Use monitor_cond for _Server DB
Bleep bloop. Greetings Terry Wilson, 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 summary should end with a dot. Subject: python: ovsdb-idl: Use monitor_cond for _Server DB Lines checked: 112, 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/2] ovsdb-idl: Add C IDL test for "monitor" fallback
Bleep bloop. Greetings Terry Wilson, 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 summary should end with a dot. Subject: ovsdb-idl: Add C IDL test for "monitor" fallback Lines checked: 52, 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 2/2] python: ovsdb-idl: Use monitor_cond for _Server DB
Unlike the C IDL code, the Python version still monitors the
_Server DB with "monitor" instead of "monitor_cond". This results
in receiving an entire Database row every time the "index" value
is updated which includes the 'schema' column. Using "monitor_cond"
will result in "update2" notifications which just include the
changed "index" value.
Unlike the C IDL, the Python IDL requires a SchemaHelper object
to instanitate the IDL, leaving it to the user of the library to
call "get_schema" themselves. Since the Python IDL did not have
support for retrieving the schema automatically and did not have
a state for doing so, instead of transitioning on an error response
from retrieving the _Server schema to requesting the "data" schema,
this moves directly to monitoring the "data" DB.
Signed-off-by: Terry Wilson
---
python/ovs/db/idl.py | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index a80da84e7..c1341fc2a 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -35,9 +35,9 @@ ROW_CREATE = "create"
ROW_UPDATE = "update"
ROW_DELETE = "delete"
-OVSDB_UPDATE = 0
-OVSDB_UPDATE2 = 1
-OVSDB_UPDATE3 = 2
+OVSDB_UPDATE = "update"
+OVSDB_UPDATE2 = "update2"
+OVSDB_UPDATE3 = "update3"
CLUSTERED = "clustered"
RELAY = "relay"
@@ -77,7 +77,7 @@ class ColumnDefaultDict(dict):
return item in self.keys()
-class Monitor(enum.IntEnum):
+class Monitor(enum.Enum):
monitor = OVSDB_UPDATE
monitor_cond = OVSDB_UPDATE2
monitor_cond_since = OVSDB_UPDATE3
@@ -465,23 +465,18 @@ class Idl(object):
self.__parse_update(msg.params[2], OVSDB_UPDATE3)
self.last_id = msg.params[1]
elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
-and msg.method == "update2"
-and len(msg.params) == 2):
-# Database contents changed.
-self.__parse_update(msg.params[1], OVSDB_UPDATE2)
-elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
-and msg.method == "update"
+and msg.method in (OVSDB_UPDATE, OVSDB_UPDATE2)
and len(msg.params) == 2):
# Database contents changed.
if msg.params[0] == str(self.server_monitor_uuid):
-self.__parse_update(msg.params[1], OVSDB_UPDATE,
+self.__parse_update(msg.params[1], msg.method,
tables=self.server_tables)
self.change_seqno = previous_change_seqno
if not self.__check_server_db():
self.force_reconnect()
break
else:
-self.__parse_update(msg.params[1], OVSDB_UPDATE)
+self.__parse_update(msg.params[1], msg.method)
elif self.handle_monitor_canceled(msg):
break
elif self.handle_monitor_cancel_reply(msg):
@@ -540,7 +535,7 @@ class Idl(object):
# Reply to our "monitor" of _Server request.
try:
self._server_monitor_request_id = None
-self.__parse_update(msg.result, OVSDB_UPDATE,
+self.__parse_update(msg.result, OVSDB_UPDATE2,
tables=self.server_tables)
self.change_seqno = previous_change_seqno
if self.__check_server_db():
@@ -579,6 +574,11 @@ class Idl(object):
elif msg.type == ovs.jsonrpc.Message.T_NOTIFY and msg.id == "echo":
# Reply to our echo request. Ignore it.
pass
+elif (msg.type == ovs.jsonrpc.Message.T_ERROR and
+ self.state == self.IDL_S_SERVER_MONITOR_REQUESTED and
+ msg.id == self._server_monitor_request_id):
+self._server_monitor_request_id = None
+self.__send_monitor_request()
elif (msg.type == ovs.jsonrpc.Message.T_ERROR and
self.state == (
self.IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED) and
@@ -912,7 +912,7 @@ class Idl(object):
monitor_request = {"columns": columns}
monitor_requests[table.name] = [monitor_request]
msg = ovs.jsonrpc.Message.create_request(
-'monitor', [self._server_db.name,
+'monitor_cond', [self._server_db.name,
str(self.server_monitor_uuid),
monitor_requests])
self._server_monitor_request_id = msg.id
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] ovsdb-idl: Add C IDL test for "monitor" fallback
There was a Python-only test for ensuring that the library would work when connecting to an older ovsdb-server that did not support monitor_cond. This adds a C IDL version of that test. Signed-off-by: Terry Wilson --- tests/ovsdb-idl.at | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index fb568dd82..a8df11ac4 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -1119,6 +1119,19 @@ OVSDB_CHECK_IDL_FETCH_COLUMNS([simple idl, initially populated], 003: done ]]) +m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND_C], + [AT_SETUP([$1 - C]) + AT_KEYWORDS([ovsdb server idl monitor $4]) + OVSDB_START_IDLTEST + AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/disable-monitor-cond]) + + AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl unix:socket $2], +[0], [stdout], [ignore]) + AT_CHECK([sort stdout | uuidfilt]m4_if([$5],,, [[| $5]]), +[0], [$3]) + OVSDB_SERVER_SHUTDOWN + AT_CLEANUP]) + m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND_PY], [AT_SETUP([$1 - Python3]) AT_KEYWORDS([ovsdb server idl Python monitor $4]) @@ -1132,7 +1145,8 @@ m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND_PY], AT_CLEANUP]) m4_define([OVSDB_CHECK_IDL_WO_MONITOR_COND], - [OVSDB_CHECK_IDL_WO_MONITOR_COND_PY($@)]) + [OVSDB_CHECK_IDL_WO_MONITOR_COND_C($@) +OVSDB_CHECK_IDL_WO_MONITOR_COND_PY($@)]) OVSDB_CHECK_IDL_WO_MONITOR_COND([simple idl disable monitor-cond], -- 2.34.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/2] python: ovsdb-idl Use monitor_cond for _Server DB
Using "monitor" for _Server means that every bump to Database.index sends a large "update" notification containing a lot of data including the Database schema text. This updates the Python IDL to use "monitor_cond" as the C IDL does. There is a difference in logic between the two IDL implementations. Python IDL's Idl takes a SchemaHelper object as a required object for instantiation, while the C IDL will transition through calling "get_schema" on first the _Server DB and data DB if that errors out. Although the Python version calls get_schema for _Server, it has never called it on data, presumably because the schema is passed at instantiation. This implementation instead of adding support for getting and using the schema via "get_schema" on the data db, transitions from an error retreiving the schema from _Server to directly monitoring the data DB (since we already were passed a schema at instantiation). It would be good to, in the future, add support for not having to specify the schema at Idl creation, but that kind of API change should be done on its own (hopefully in a backward-compatible way). Terry Wilson (2): ovsdb-idl: Add C IDL test for "monitor" fallback python: ovsdb-idl: Use monitor_cond for _Server DB python/ovs/db/idl.py | 28 ++-- tests/ovsdb-idl.at | 16 +++- 2 files changed, 29 insertions(+), 15 deletions(-) -- 2.34.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] fix ct tp policy when create zone.
>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);
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] selftests/net: fix uninitialized variables
Hi John, On 06/05/2024 00:26, John Hubbard wrote: > When building with clang, via: > > make LLVM=1 -C tools/testing/selftest > > ...clang warns about three variables that are not initialized in all > cases: > > 1) The opt_ipproto_off variable is used uninitialized if "testname" is > not "ip". This seems like an actual bug. > > 2) The addr_len is used uninitialized, but only in the assert case, >which bails out, so this is harmless. > > 3) The family variable in add_listener() is only used uninitialized in >the error case (neither IPv4 nor IPv6 is specified), so it's also >harmless. > > Fix by initializing each variable. > > Signed-off-by: John Hubbard > --- > tools/testing/selftests/net/gro.c | 3 ++- > tools/testing/selftests/net/ip_local_port_range.c | 2 +- > tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 2 +- Thank you for fixing these warnings! The modification in the MPTCP selftest directory looks good to me: Reviewed-by: Matthieu Baerts (NGI0) Cheers, Matt -- Sponsored by the NGI0 Core fund. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] selftests/net: suppress clang's "variable-sized type not at the end" warning
When building with clang, via: make LLVM=1 -C tools/testing/selftest ...clang warns that "a variable sized type not at the end of a struct or class is a GNU extension". These cases are not easily changed, because they involve structs that are part of the API. Fortunately, however, the tests seem to be doing just fine (specifically, neither affected test runs any differently with gcc vs. clang builds, on my test system) regardless of the warning. So, all the warning is doing is preventing a clean build of selftests/net. Fix this by suppressing this particular clang warning for the selftests/net suite. Signed-off-by: John Hubbard --- tools/testing/selftests/net/Makefile | 4 1 file changed, 4 insertions(+) diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index 7b6918d5f4af..956481174783 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -6,6 +6,10 @@ CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES) # Additional include paths needed by kselftest.h CFLAGS += -I../ +ifneq ($(LLVM),) +CFLAGS += -Wno-gnu-variable-sized-type-not-at-end +endif + TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh \ rtnetlink.sh xfrm_policy.sh test_blackhole_dev.sh TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh udpgso.sh ip_defrag.sh base-commit: f462ae0edd3703edd6f22fe41d336369c38b884b prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27 -- 2.45.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] selftests/net: fix uninitialized variables
When building with clang, via: make LLVM=1 -C tools/testing/selftest ...clang warns about three variables that are not initialized in all cases: 1) The opt_ipproto_off variable is used uninitialized if "testname" is not "ip". This seems like an actual bug. 2) The addr_len is used uninitialized, but only in the assert case, which bails out, so this is harmless. 3) The family variable in add_listener() is only used uninitialized in the error case (neither IPv4 nor IPv6 is specified), so it's also harmless. Fix by initializing each variable. Signed-off-by: John Hubbard --- tools/testing/selftests/net/gro.c | 3 ++- tools/testing/selftests/net/ip_local_port_range.c | 2 +- tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c index 353e1e867fbb..0eb61edaad83 100644 --- a/tools/testing/selftests/net/gro.c +++ b/tools/testing/selftests/net/gro.c @@ -110,7 +110,8 @@ static void setup_sock_filter(int fd) const int dport_off = tcp_offset + offsetof(struct tcphdr, dest); const int ethproto_off = offsetof(struct ethhdr, h_proto); int optlen = 0; - int ipproto_off, opt_ipproto_off; + int ipproto_off; + int opt_ipproto_off = 0; int next_off; if (proto == PF_INET) diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c index 193b82745fd8..29451d2244b7 100644 --- a/tools/testing/selftests/net/ip_local_port_range.c +++ b/tools/testing/selftests/net/ip_local_port_range.c @@ -359,7 +359,7 @@ TEST_F(ip_local_port_range, late_bind) struct sockaddr_in v4; struct sockaddr_in6 v6; } addr; - socklen_t addr_len; + socklen_t addr_len = 0; const int one = 1; int fd, err; __u32 range; diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c index 7426a2cbd4a0..7ad5a59adff2 100644 --- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c +++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c @@ -1276,7 +1276,7 @@ int add_listener(int argc, char *argv[]) struct sockaddr_storage addr; struct sockaddr_in6 *a6; struct sockaddr_in *a4; - u_int16_t family; + u_int16_t family = AF_UNSPEC; int enable = 1; int sock; int err; -- 2.45.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Drop invalid parsed packets
On 06/05/2024 14:05, Ilya Maximets wrote: > On 5/5/24 08:42, Roi Dayan via dev wrote: >> From: Eli Britstein >> >> In case of a malformed packet, its parsing fails. Instead of continuing >> and possible form a wrong flow, drop the packet. > > Hi, Eli and Roi. Thanks for the patch! > > But I don't think we can do that. There are few reasons why the > packets should not be dropped in the datapath: > > 1. OVS is a switch, the only reasons why it should drop packets are: > - User configuration > - Inability to make a forwarding decision >Both are not the case here. For example, if the packet has some >issue in the IPv4 header, we should still forward it in case we >just acting as an L2 learning switch. L2 learning switch doesn't >need any information from IPv4 header to function. > > 2. Datapath should not make forwarding decisions including a decision >to drop a packet. Datapath implementation should just execute >actions that OpenFlow layers tell it to execute. OpenFlow layers >should decide what to do. > > Also, inability to parse certain parts of the packet is not a parsing > failure. The resulted flow structure should be valid regardless of > the packet content. Fields that were not extracted remain zero and > OpenFlow layers should correctly handle that and execute appropriate > actions, i.e. properly match on all-zero values if they were used to > make a forwarding decision. > > If the wrong flow can be installed in this situation - it's a bug > somewhere in the flow translation logic that should be fixed. > > Hope this makes sense. > > Best regards, Ilya Maximets. thanks Ilya for the explanation. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Drop invalid parsed packets
On 5/5/24 08:42, Roi Dayan via dev wrote: > From: Eli Britstein > > In case of a malformed packet, its parsing fails. Instead of continuing > and possible form a wrong flow, drop the packet. Hi, Eli and Roi. Thanks for the patch! But I don't think we can do that. There are few reasons why the packets should not be dropped in the datapath: 1. OVS is a switch, the only reasons why it should drop packets are: - User configuration - Inability to make a forwarding decision Both are not the case here. For example, if the packet has some issue in the IPv4 header, we should still forward it in case we just acting as an L2 learning switch. L2 learning switch doesn't need any information from IPv4 header to function. 2. Datapath should not make forwarding decisions including a decision to drop a packet. Datapath implementation should just execute actions that OpenFlow layers tell it to execute. OpenFlow layers should decide what to do. Also, inability to parse certain parts of the packet is not a parsing failure. The resulted flow structure should be valid regardless of the packet content. Fields that were not extracted remain zero and OpenFlow layers should correctly handle that and execute appropriate actions, i.e. properly match on all-zero values if they were used to make a forwarding decision. If the wrong flow can be installed in this situation - it's a bug somewhere in the flow translation logic that should be fixed. Hope this makes sense. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] controller: Avoid use after free in LB I-P.
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
---
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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
