Re: [ovs-dev] [PATCH ovn v4] Add a northbound interface to program MAC_Binding table
> From: Karthik Chandrashekar
>
> From: Karthik Chandrashekar
Hi Karthik,
Thanks for the new version. You need rebase it since it does not apply cleanly.
Some comments inline.
Regards,
Lorenzo
>
[...]
> @@ -2346,7 +2368,7 @@ add_lb_hairpin_flows(const struct
> sbrec_load_balancer_table *lb_table,
>
> /* Handles neighbor changes in mac_binding table. */
> void
> -lflow_handle_changed_neighbors(
> +lflow_handle_changed_mac_bindings(
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> const struct sbrec_mac_binding_table *mac_binding_table,
> const struct hmap *local_datapaths,
> @@ -2373,7 +2395,40 @@ lflow_handle_changed_neighbors(
> VLOG_DBG("handle new mac_binding "UUID_FMT,
> UUID_ARGS(&mb->header_.uuid));
> consider_neighbor_flow(sbrec_port_binding_by_name,
> local_datapaths,
> - mb, flow_table);
> + mb, NULL, flow_table, 100);
> +}
> +}
> +}
> +
> +/* Handles changes to static_mac_binding table. */
> +void
> +lflow_handle_changed_static_mac_bindings(
> +struct ovsdb_idl_index *sbrec_port_binding_by_name,
> +const struct sbrec_static_mac_binding_table *smb_table,
> +const struct hmap *local_datapaths,
> +struct ovn_desired_flow_table *flow_table)
> +{
> +const struct sbrec_static_mac_binding *smb;
> +SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, smb_table) {
> +/* Remove any flows that should be removed. */
> +if (sbrec_static_mac_binding_is_deleted(smb)) {
> +VLOG_DBG("handle deleted static_mac_binding "UUID_FMT,
> + UUID_ARGS(&smb->header_.uuid));
> +ofctrl_remove_flows(flow_table, &smb->header_.uuid);
> +}
> +}
I guess we merge these two loops, right?
> +SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, smb_table) {
> +if (!sbrec_static_mac_binding_is_deleted(smb)) {
> +if (!sbrec_static_mac_binding_is_new(smb)) {
> +VLOG_DBG("handle updated static_mac_binding "UUID_FMT,
> + UUID_ARGS(&smb->header_.uuid));
> +ofctrl_remove_flows(flow_table, &smb->header_.uuid);
> +}
> +VLOG_DBG("handle new static_mac_binding "UUID_FMT,
> + UUID_ARGS(&smb->header_.uuid));
> +consider_neighbor_flow(sbrec_port_binding_by_name,
> local_datapaths,
> + NULL, smb, flow_table,
> + smb->override_dynamic_mac ? 150 : 50);
iiuc, if the user does not provide override_dynamic_mac and we have conflict,
we will end up with 2 flows, one dynamically learned (with priority 100) and
one statically added (with priority 50). Correct?
I think we should have just the "static mac" flow when we have a conflict.
> }
> }
> }
> @@ -2443,7 +2498,9 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
> lflow_ctx_out *l_ctx_out)
>
> add_logical_flows(l_ctx_in, l_ctx_out);
> add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name,
> - l_ctx_in->mac_binding_table,
> l_ctx_in->local_datapaths,
> + l_ctx_in->mac_binding_table,
> + l_ctx_in->static_mac_binding_table,
> + l_ctx_in->local_datapaths,
> l_ctx_out->flow_table);
> add_lb_hairpin_flows(l_ctx_in->lb_table, l_ctx_in->local_datapaths,
> l_ctx_out->flow_table,
> diff --git a/controller/lflow.h b/controller/lflow.h
> index d61733bc2..c2944378b 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -147,6 +147,7 @@ struct lflow_ctx_in {
> const struct sbrec_fdb_table *fdb_table;
> const struct sbrec_chassis *chassis;
> const struct sbrec_load_balancer_table *lb_table;
> +const struct sbrec_static_mac_binding_table *static_mac_binding_table;
> const struct hmap *local_datapaths;
> const struct shash *addr_sets;
> const struct shash *port_groups;
> @@ -191,9 +192,14 @@ bool lflow_handle_addr_set_update(const char *as_name,
> struct addr_set_diff *,
>struct lflow_ctx_out *,
>bool *changed);
>
> -void lflow_handle_changed_neighbors(
> +void lflow_handle_changed_mac_bindings(
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> -const struct sbrec_mac_binding_table *,
> +const struct sbrec_mac_binding_table *mac_binding_table,
> +const struct hmap *local_datapaths,
> +struct ovn_desired_flow_table *);
> +void lflow_handle_changed_static_mac_bindings(
> +struct ovsdb_idl_index *sbrec_port_binding_by_name,
> +const struct sbrec_static_mac_binding_table *smb_table,
> const struct hmap *local_datapaths,
> struct ovn_desired_flow_table *);
> bool lflow_handle_changed_lbs(struct lflow_ctx_in *, struct lflow_ctx_o
[ovs-dev] [PATCHv2] IPv6: Add IPv6 extension header support
IPv6 extension headers carry optional internet layer information
and are placed between the fixed header and the upper-layer
protocol header.
This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and
packets can be filtered using ipv6_ext flag.
Some spacing style issues fixed.
Tested-at: https://github.com/TomCodeLV/ovs/actions/runs/504185214
Signed-off-by: Toms Atteka
---
build-aux/extract-odp-netlink-macros-h| 1 +
build-aux/extract-ofp-fields | 4 +-
datapath/flow.c | 192 +++---
datapath/flow.h | 14 ++
datapath/flow_netlink.c | 25 ++-
.../linux/compat/include/linux/openvswitch.h | 6 +
include/openvswitch/flow.h| 8 +-
include/openvswitch/match.h | 2 +
include/openvswitch/meta-flow.h | 18 ++
lib/dpif-netdev-extract-avx512.c | 2 +-
lib/flow.c| 188 -
lib/flow.h| 6 +-
lib/match.c | 25 ++-
lib/meta-flow.c | 80 +++-
lib/meta-flow.xml | 14 ++
lib/nx-match.c| 13 +-
lib/odp-execute.c | 2 +
lib/odp-util.c| 111 +-
lib/odp-util.h| 11 +-
lib/ofp-match.c | 2 +-
lib/packets.c | 28 +++
lib/packets.h | 12 ++
ofproto/ofproto-dpif-rid.h| 2 +-
ofproto/ofproto-dpif-sflow.c | 1 +
ofproto/ofproto-dpif-xlate.c | 2 +-
ofproto/ofproto-dpif.c| 47 +
tests/ofproto.at | 4 +-
tests/system-traffic.at | 31 +++
28 files changed, 789 insertions(+), 62 deletions(-)
diff --git a/build-aux/extract-odp-netlink-macros-h
b/build-aux/extract-odp-netlink-macros-h
index 7152f298c..c5a2d91d2 100755
--- a/build-aux/extract-odp-netlink-macros-h
+++ b/build-aux/extract-odp-netlink-macros-h
@@ -55,6 +55,7 @@ generate_fields_macros "ovs_key_icmpv6"
generate_fields_macros "ovs_key_arp"
generate_fields_macros "ovs_key_nd"
generate_fields_macros "ovs_key_nd_extensions"
+generate_fields_macros "ovs_key_ipv6_exthdrs"
echo
echo "#endif"
diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
index 8766995d9..df71e4dfa 100755
--- a/build-aux/extract-ofp-fields
+++ b/build-aux/extract-ofp-fields
@@ -19,6 +19,7 @@ VERSION = {"1.0": 0x01,
VERSION_REVERSE = dict((v,k) for k, v in VERSION.items())
TYPES = {"u8": (1, False),
+ "u16": (2, False),
"be16": (2, False),
"be32": (4, False),
"MAC": (6, False),
@@ -37,7 +38,8 @@ FORMATTING = {"decimal":("MFS_DECIMAL", 1,
8),
"frag": ("MFS_FRAG", 1, 1),
"tunnel flags": ("MFS_TNL_FLAGS",2, 2),
"TCP flags": ("MFS_TCP_FLAGS",2, 2),
- "packet type":("MFS_PACKET_TYPE", 4, 4)}
+ "packet type":("MFS_PACKET_TYPE", 4, 4),
+ "IPV6 ext hdr": ("MFS_IPV6_EXTHDR", 2, 2)}
PREREQS = {"none": "MFP_NONE",
"Ethernet": "MFP_ETHERNET",
diff --git a/datapath/flow.c b/datapath/flow.c
index 5a00c238c..1247c298a 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -98,17 +98,17 @@ void ovs_flow_stats_update(struct sw_flow *flow, __be16
tcp_flags,
* allocated stats as we have already locked them.
*/
if (likely(flow->stats_last_writer != -1) &&
- likely(!rcu_access_pointer(flow->stats[cpu]))) {
+ likely(!rcu_access_pointer(flow->stats[cpu]))) {
/* Try to allocate CPU-specific stats. */
struct sw_flow_stats *new_stats;
new_stats =
kmem_cache_alloc_node(flow_stats_cache,
- GFP_NOWAIT |
- __GFP_THISNODE |
- __GFP_NOWARN |
- __GFP_NOMEMALLOC,
- numa_node_id());
+
GFP_NOWAIT |
+
Re: [ovs-dev] [PATCH] IPv6: Add IPv6 extension header support
Bleep bloop. Greetings Toms Atteka, 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:
ERROR: Author Toms Atteka needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors
or committers: Toms Atteka
WARNING: Line is 81 characters long (recommended limit is 79)
#1296 FILE: lib/odp-util.c:2750:
[OVS_KEY_ATTR_IPV6_EXTHDRS] = { .len = sizeof(struct ovs_key_ipv6_exthdrs)
},
WARNING: Line is 85 characters long (recommended limit is 79)
#1463 FILE: lib/odp-util.c:8190:
struct ofpbuf *odp_actions, struct
flow_wildcards *wc,
Lines checked: 1798, Warnings: 3, Errors: 1
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] IPv6: Add IPv6 extension header support
From: Toms Atteka
IPv6 extension headers carry optional internet layer information
and are placed between the fixed header and the upper-layer
protocol header.
This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and
packets can be filtered using ipv6_ext flag.
Some spacing style issues fixed.
Tested-at: https://github.com/TomCodeLV/ovs/actions/runs/504185214
Signed-off-by: Toms Atteka
---
build-aux/extract-odp-netlink-macros-h| 1 +
build-aux/extract-ofp-fields | 4 +-
datapath/flow.c | 192 +++---
datapath/flow.h | 14 ++
datapath/flow_netlink.c | 25 ++-
.../linux/compat/include/linux/openvswitch.h | 6 +
include/openvswitch/flow.h| 8 +-
include/openvswitch/match.h | 2 +
include/openvswitch/meta-flow.h | 18 ++
lib/dpif-netdev-extract-avx512.c | 2 +-
lib/flow.c| 188 -
lib/flow.h| 6 +-
lib/match.c | 25 ++-
lib/meta-flow.c | 80 +++-
lib/meta-flow.xml | 14 ++
lib/nx-match.c| 13 +-
lib/odp-execute.c | 2 +
lib/odp-util.c| 110 +-
lib/odp-util.h| 11 +-
lib/ofp-match.c | 2 +-
lib/packets.c | 28 +++
lib/packets.h | 12 ++
ofproto/ofproto-dpif-rid.h| 2 +-
ofproto/ofproto-dpif-sflow.c | 1 +
ofproto/ofproto-dpif-xlate.c | 2 +-
ofproto/ofproto-dpif.c| 47 +
tests/ofproto.at | 4 +-
tests/system-traffic.at | 31 +++
28 files changed, 788 insertions(+), 62 deletions(-)
diff --git a/build-aux/extract-odp-netlink-macros-h
b/build-aux/extract-odp-netlink-macros-h
index 7152f298c..c5a2d91d2 100755
--- a/build-aux/extract-odp-netlink-macros-h
+++ b/build-aux/extract-odp-netlink-macros-h
@@ -55,6 +55,7 @@ generate_fields_macros "ovs_key_icmpv6"
generate_fields_macros "ovs_key_arp"
generate_fields_macros "ovs_key_nd"
generate_fields_macros "ovs_key_nd_extensions"
+generate_fields_macros "ovs_key_ipv6_exthdrs"
echo
echo "#endif"
diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
index 8766995d9..df71e4dfa 100755
--- a/build-aux/extract-ofp-fields
+++ b/build-aux/extract-ofp-fields
@@ -19,6 +19,7 @@ VERSION = {"1.0": 0x01,
VERSION_REVERSE = dict((v,k) for k, v in VERSION.items())
TYPES = {"u8": (1, False),
+ "u16": (2, False),
"be16": (2, False),
"be32": (4, False),
"MAC": (6, False),
@@ -37,7 +38,8 @@ FORMATTING = {"decimal":("MFS_DECIMAL", 1,
8),
"frag": ("MFS_FRAG", 1, 1),
"tunnel flags": ("MFS_TNL_FLAGS",2, 2),
"TCP flags": ("MFS_TCP_FLAGS",2, 2),
- "packet type":("MFS_PACKET_TYPE", 4, 4)}
+ "packet type":("MFS_PACKET_TYPE", 4, 4),
+ "IPV6 ext hdr": ("MFS_IPV6_EXTHDR", 2, 2)}
PREREQS = {"none": "MFP_NONE",
"Ethernet": "MFP_ETHERNET",
diff --git a/datapath/flow.c b/datapath/flow.c
index 5a00c238c..1247c298a 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -98,17 +98,17 @@ void ovs_flow_stats_update(struct sw_flow *flow, __be16
tcp_flags,
* allocated stats as we have already locked them.
*/
if (likely(flow->stats_last_writer != -1) &&
- likely(!rcu_access_pointer(flow->stats[cpu]))) {
+ likely(!rcu_access_pointer(flow->stats[cpu]))) {
/* Try to allocate CPU-specific stats. */
struct sw_flow_stats *new_stats;
new_stats =
kmem_cache_alloc_node(flow_stats_cache,
- GFP_NOWAIT |
- __GFP_THISNODE |
- __GFP_NOWARN |
- __GFP_NOMEMALLOC,
- numa_node_id());
+
GFP_NOWAIT |
+
Re: [ovs-dev] [PATCH ovn] northd: Use separate SNAT for already-DNATted traffic.
On Wed, Mar 9, 2022 at 2:38 PM Mark Michelson wrote:
>
> Commit 4deac4509abbedd6ffaecf27eed01ddefccea40a introduced functionality
> in ovn-northd to use a single zone for SNAT and DNAT when possible. It
> accounts for certain situations, such as hairpinned traffic, where we
> still need a separate SNAT and DNAT zone.
>
> However, one situation it did not account for was when traffic traverses
> a logical router and is DNATted as a result of a load balancer, then
> when the traffic egresses the router, it needs to be SNATted. In this
> situation, we try to use the same CT zone for the SNAT as for the load
> balancer DNAT, which does not work.
>
> This commit fixes the issue by installing a flow in the OUT_SNAT stage
> of the logical router egress pipeline for each SNAT. This flow will fall
> back to using a separate CT zone for SNAT if the ct_label.natted flag is
> set.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2044127
>
> Signed-off-by: Mark Michelson
Hi Mark,
Few comments,
- There are a few test cases failing with this patch.
- I'm pretty sure the issue will not be seen if the load balancer is
also associated with the logical switch. In that case, dnat would
happen in the logical switch pipeline.
But still we should address this issue as it's a regression
introduced by the commit 4deac4509abb.
- Only small concern I've with this patch is the usage of
ct_label.natted field in the egress pipeline. In the ingress pipeline
of the router we send the pkt to conntrack and then do NAT.
I'm just worried what if the ct states/labels are not valid or they
get cleared when the pkt enters ingress to egress pipeline. Looks
like we don't clear the conntrack state now.
But future patches could. I think we should document this somewhere.
- This patch needs to add the relevant documentation in the
ovn-northd.8.xml for the new flows added.
- Instead of adding the flows in the "lr_out_snat" stage, I'd
suggest to add the below flow in the "lr_out_chk_dnat_local" pipeline
table=0 (lr_out_chk_dnat_local), priority=50 , match=(ip &&
ct_label.natted == 1), action=(reg9[4] = 1; next;)
We already have flows in the "lr_out_snat" stage to make use of
"ct_snat" action instead of "ct_snat_in_czone" if reg9[4] is set.
Can you please see if this suggestion is possible ?
Thanks
Numan
> ---
> Note: The "SNAT in separate zone from DNAT" test in system-ovn.at can
> fail on some systems, and at this point it's not clear what causes it.
> On my development system running Fedora 33 and kernel 5.14.18-100, the
> test fails because the pings fail. In a container running Fedora 33 and
> kernel 5.10.19-200, the test succeeds. It's unknown if it's the kernel
> version or some other setting that has a bearing on the success or
> failure of the test. The OpenFlow that ovn-controller generates is
> identical on both successful and failure scenarios.
>
> If this inconsistency causes issues with CI, then this patch may need to
> be re-examined to determine if there are other parameters that may be
> needed to correct the referenced issue.
> ---
> northd/northd.c | 53 ++--
> tests/ovn-northd.at | 36 ++
> tests/system-ovn.at | 117
> 3 files changed, 190 insertions(+), 16 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index b264fb850..866a81310 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -12961,23 +12961,44 @@ build_lrouter_out_snat_flow(struct hmap *lflows,
> struct ovn_datapath *od,
> priority, ds_cstr(match),
> ds_cstr(actions), &nat->header_);
>
> -if (!stateless) {
> -ds_put_cstr(match, " && "REGBIT_DST_NAT_IP_LOCAL" == 1");
> -ds_clear(actions);
> -if (distributed) {
> -ds_put_format(actions, "eth.src = "ETH_ADDR_FMT"; ",
> - ETH_ADDR_ARGS(mac));
> -}
> -ds_put_format(actions, REGBIT_DST_NAT_IP_LOCAL" = 0;
> ct_snat(%s",
> - nat->external_ip);
> -if (nat->external_port_range[0]) {
> -ds_put_format(actions, ",%s", nat->external_port_range);
> -}
> -ds_put_format(actions, ");");
> -ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_SNAT,
> -priority + 1, ds_cstr(match),
> -ds_cstr(actions), &nat->header_);
> +if (stateless) {
> +return;
> +}
> +
> +struct ds match_natted = DS_EMPTY_INITIALIZER;
> +struct ds actions_natted = DS_EMPTY_INITIALIZER;
> +
> +ds_clone(&match_natted, match);
> +
> +ds_clear(actions);
> +
> +if (distributed) {
> +ds_put_format(actions, "eth.src = "ETH_ADDR_FMT"; ",
> + ETH_ADDR_ARGS(mac));
> }
> +
> +
Re: [ovs-dev] [PATCH v2] ofproto-dpif-xlate: avoid successive ct_clear datapath actions
On 3/16/22 15:08, Eelco Chaudron wrote: > > > On 14 Jul 2021, at 14:33, Eelco Chaudron wrote: > >> On 8 Jul 2021, at 21:21, Ilya Maximets wrote: >> >>> On 5/24/21 2:39 PM, Timothy Redaelli wrote: On Tue, 18 May 2021 06:17:48 -0400 Eelco Chaudron wrote: > Due to flow lookup optimizations, especially in the resubmit/clone cases, > we might end up with multiple ct_clear actions, which are not necessary. > > This patch only adds the ct_clear action to the datapath if any ct state > is tracked. > > Signed-off-by: Eelco Chaudron > --- > v2: Insert ct_clear only when ct information is tracked vs tracking > successive > ct_clear actions. > > ofproto/ofproto-dpif-xlate.c |4 +++- > tests/ofproto-dpif.at| 25 + > 2 files changed, 28 insertions(+), 1 deletion(-) Acked-By: Timothy Redaelli >>> >>> Thanks! Applied. >> >> Can this also be backported to the stable releases? > > Guess this email might have gone missing. Can we still get this backported to > make hw offload work better? Oops. Backported down to 2.13 now. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dp-packet: Allow DPDK packet resize.
DPDK based dp-packets points to data buffers that can't be expanded
dynamically.
Their layout is as follows:
- a 128 bytes headroom chosen at DPDK build time (RTE_PKTMBUF_HEADROOM),
- a maximum size chosen at mempool creation,
In some usecases though (like encapsulating with multiple tunnels),
a 128 bytes headroom is too short.
Dynamically allocate buffers in DPDK memory and make use of DPDK
external buffers API (previously used for userspace TSO).
Signed-off-by: David Marchand
---
lib/dp-packet.c | 17 -
lib/netdev-dpdk.c | 47 +++
lib/netdev-dpdk.h | 3 +++
3 files changed, 54 insertions(+), 13 deletions(-)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 35c72542a2..07fa67b1a1 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -250,8 +250,23 @@ dp_packet_resize(struct dp_packet *b, size_t new_headroom,
size_t new_tailroom)
new_allocated = new_headroom + dp_packet_size(b) + new_tailroom;
switch (b->source) {
-case DPBUF_DPDK:
+case DPBUF_DPDK: {
+#ifdef DPDK_NETDEV
+uint32_t buf_len;
+
+buf_len = new_allocated;
+new_base = netdev_dpdk_extbuf_allocate(&buf_len);
+if (!new_base) {
+out_of_memory();
+}
+ovs_assert(buf_len <= UINT16_MAX);
+dp_packet_copy__(b, new_base, new_headroom, new_tailroom);
+netdev_dpdk_extbuf_replace(b, new_base, buf_len);
+break;
+#else
OVS_NOT_REACHED();
+#endif
+}
case DPBUF_MALLOC:
if (new_headroom == dp_packet_headroom(b)) {
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index fbc3b42d84..47e16f22c5 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2646,41 +2646,64 @@ out:
}
}
+void *
+netdev_dpdk_extbuf_allocate(uint32_t *data_len)
+{
+*data_len += sizeof(struct rte_mbuf_ext_shared_info) + sizeof(uintptr_t);
+*data_len = RTE_ALIGN_CEIL(*data_len, sizeof(uintptr_t));
+return rte_malloc(NULL, *data_len, RTE_CACHE_LINE_SIZE);
+}
+
static void
netdev_dpdk_extbuf_free(void *addr OVS_UNUSED, void *opaque)
{
rte_free(opaque);
}
+void
+netdev_dpdk_extbuf_replace(struct dp_packet *b, void *buf, uint32_t data_len)
+{
+struct rte_mbuf *pkt = (struct rte_mbuf *) b;
+struct rte_mbuf_ext_shared_info *shinfo;
+uint16_t buf_len = data_len;
+
+shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
+netdev_dpdk_extbuf_free,
+buf);
+ovs_assert(shinfo != NULL);
+
+if (RTE_MBUF_HAS_EXTBUF(pkt)) {
+rte_pktmbuf_detach_extbuf(pkt);
+}
+rte_pktmbuf_attach_extbuf(pkt, buf, rte_malloc_virt2iova(buf), buf_len,
+ shinfo);
+}
+
static struct rte_mbuf *
dpdk_pktmbuf_attach_extbuf(struct rte_mbuf *pkt, uint32_t data_len)
{
uint32_t total_len = RTE_PKTMBUF_HEADROOM + data_len;
-struct rte_mbuf_ext_shared_info *shinfo = NULL;
+struct rte_mbuf_ext_shared_info *shinfo;
uint16_t buf_len;
void *buf;
-total_len += sizeof *shinfo + sizeof(uintptr_t);
-total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t));
-
+buf = netdev_dpdk_extbuf_allocate(&total_len);
+if (OVS_UNLIKELY(buf == NULL)) {
+VLOG_ERR("Failed to allocate memory using rte_malloc: %u", total_len);
+return NULL;
+}
if (OVS_UNLIKELY(total_len > UINT16_MAX)) {
+netdev_dpdk_extbuf_free(NULL, buf);
VLOG_ERR("Can't copy packet: too big %u", total_len);
return NULL;
}
buf_len = total_len;
-buf = rte_malloc(NULL, buf_len, RTE_CACHE_LINE_SIZE);
-if (OVS_UNLIKELY(buf == NULL)) {
-VLOG_ERR("Failed to allocate memory using rte_malloc: %u", buf_len);
-return NULL;
-}
-
-/* Initialize shinfo. */
shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
netdev_dpdk_extbuf_free,
buf);
if (OVS_UNLIKELY(shinfo == NULL)) {
-rte_free(buf);
+netdev_dpdk_extbuf_free(NULL, buf);
VLOG_ERR("Failed to initialize shared info for mbuf while "
"attempting to attach an external buffer.");
return NULL;
diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
index 699be3fb41..95594f07fb 100644
--- a/lib/netdev-dpdk.h
+++ b/lib/netdev-dpdk.h
@@ -31,6 +31,9 @@ struct netdev;
void netdev_dpdk_register(void);
void free_dpdk_buf(struct dp_packet *);
+void *netdev_dpdk_extbuf_allocate(uint32_t *);
+void netdev_dpdk_extbuf_replace(struct dp_packet *, void *, uint32_t);
+
bool netdev_dpdk_flow_api_supported(struct netdev *);
int
--
2.23.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs-monitor-ipsec: Allow custom options per tunnel
On Wed, Mar 2, 2022 at 8:40 AM Andreas Karis wrote:
>
> Tunnels in LibreSwan and OpenSwan allow for many options to be set on a
> per tunnel basis. Pass through any options starting with ipsec_ to the
> connection in the configuration file. Administrators are responsible for
> picking valid key/value pairs.
>
> Signed-off-by: Andreas Karis
Acked-by: Mike Pattrick
> ---
> Documentation/tutorials/ipsec.rst | 45 +++
> ipsec/ovs-monitor-ipsec.in| 17 +++-
> vswitchd/vswitch.xml | 4 ++-
> 3 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/tutorials/ipsec.rst
> b/Documentation/tutorials/ipsec.rst
> index b6cc1c3a8..00cdc5ec2 100644
> --- a/Documentation/tutorials/ipsec.rst
> +++ b/Documentation/tutorials/ipsec.rst
> @@ -303,6 +303,50 @@ external IP is 1.1.1.1, and `host_2`'s external IP is
> 2.2.2.2. Make sure
> You should be able to see that ESP packets are being sent from `host_1` to
> `host_2`.
>
> +Custom options
> +---
> +
> +Any parameter prefixed with `ipsec_` will be added to the connection profile.
> +For example::
> +
> +# ovs-vsctl set interface tun options:ipsec_encapsulation=yes
> +
> +Will result in::
> +
> +# ovs-appctl -t ovs-monitor-ipsec tunnels/show
> +Interface name: tun v7 (CONFIGURED)
> +Tunnel Type:vxlan
> +Local IP: 192.0.0.1
> +Remote IP: 192.0.0.2
> +Address Family: IPv4
> +SKB mark: None
> +Local cert: None
> +Local name: None
> +Local key: None
> +Remote cert:None
> +Remote name:None
> +CA cert:None
> +PSK:swordfish
> +Custom Options: {'encapsulation': 'yes'}
> +
> +And in the following connection profiles::
> +
> +conn tun-in-7
> +left=192.0.0.1
> +right=192.0.0.2
> +authby=secret
> +encapsulation=yes
> +leftprotoport=udp/4789
> +rightprotoport=udp
> +
> +conn tun-out-7
> +left=192.0.0.1
> +right=192.0.0.2
> +authby=secret
> +encapsulation=yes
> +leftprotoport=udp
> +rightprotoport=udp/4789
> +
> Troubleshooting
> ---
>
> @@ -329,6 +373,7 @@ For example::
> Remote name:None
> CA cert:None
> PSK:swordfish
> + Custom Options: {}
> Ofport: 1 <--- Whether ovs-vswitchd has assigned Ofport
> number to this Tunnel Port
> CFM state: Up <--- Whether CFM declared this tunnel healthy
> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in
> index a8b0705d9..e422b07bf 100755
> --- a/ipsec/ovs-monitor-ipsec.in
> +++ b/ipsec/ovs-monitor-ipsec.in
> @@ -313,6 +313,10 @@ conn prevent_unencrypted_vxlan
> tmpl = self.auth_tmpl["pki_ca"]
> auth_section = tmpl.substitute(tunnel.conf)
>
> +if "custom_options" in tunnel.conf:
> +for key, value in tunnel.conf["custom_options"].items():
> +auth_section += "\n" + key + "=" + value
> +
> vals = tunnel.conf.copy()
> vals["auth_section"] = auth_section
> vals["version"] = tunnel.version
> @@ -543,6 +547,10 @@ conn prevent_unencrypted_vxlan
> if tunnel.conf["address_family"] == "IPv6":
> auth_section = self.IPV6_CONN + auth_section
>
> +if "custom_options" in tunnel.conf:
> +for key, value in tunnel.conf["custom_options"].items():
> +auth_section += "\n" + key + "=" + value
> +
> vals = tunnel.conf.copy()
> vals["auth_section"] = auth_section
> vals["version"] = tunnel.version
> @@ -819,6 +827,7 @@ class IPsecTunnel(object):
>Remote name:$remote_name
>CA cert:$ca_cert
>PSK:$psk
> + Custom Options: $custom_options
> """)
>
> unixctl_status_tmpl = Template("""\
> @@ -862,7 +871,13 @@ class IPsecTunnel(object):
> "remote_cert": remote_cert,
> "remote_name": remote_name,
> "local_name": monitor.conf["pki"]["local_name"],
> -"psk": options.get("psk")}
> +"psk": options.get("psk"),
> +"custom_options": {}}
> +
> +# add custom ipsec options to the connection
> +for key, value in options.items():
> +if key.startswith("ipsec_"):
> +new_conf["custom_options"][key[len("ipsec_"):]] = value
>
> if self.conf != new_conf:
> # Configuration was updated in OVSDB. Validate it and figure
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 0c6632617..b124fee54 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1046,7 +1046,9 @@
>
> These settings control the global configuration of IPsec tunnels.
> The
> options column of the Interface table
> -configures IPsec for
Re: [ovs-dev] IPsec/test: skip the test if tcpdump are not installed
On Thu, Mar 17, 2022 at 7:36 AM Mohammad Heib wrote: > > IPsec unit tests uses tcpdump to capture and validate the ESP > traffic so the test must be skipped in environment that don't > have the tcpdump tool installed. > > Signed-off-by: Mohammad Heib Doesn't system-traffic.at also use tcpdump? When would system-ipsec.at be run but not system-traffic.at ? Cheers, MKP > --- > tests/system-ipsec.at | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/tests/system-ipsec.at b/tests/system-ipsec.at > index f45a153ed..888f79120 100644 > --- a/tests/system-ipsec.at > +++ b/tests/system-ipsec.at > @@ -99,9 +99,10 @@ dnl Check if necessary Libreswan dependencies are > available on the test machine > m4_define([CHECK_LIBRESWAN], >[dnl Skip tests if system has not been set up for Libreswan >AT_SKIP_IF([!(ipsec --version | grep Libreswan)]) > - AT_SKIP_IF([test ! -x $(which certutil)]) > - AT_SKIP_IF([test ! -x $(which pk12util)]) > - AT_SKIP_IF([test ! -x $(which openssl)]) > + AT_SKIP_IF([test ! $(which certutil)]) > + AT_SKIP_IF([test ! $(which pk12util)]) > + AT_SKIP_IF([test ! $(which openssl)]) > + AT_SKIP_IF([test ! $(which tcpdump)]) >dnl If '$ovs_base' is too long, the following Libreswan issue will > trigger >dnl so we check that it is not too long and skip test if it is. >dnl https://github.com/libreswan/libreswan/issues/428 > -- > 2.31.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 net v2] openvswitch: always update flow key after nat
On 18 Mar 2022, at 13:43, Aaron Conole wrote:
> During NAT, a tuple collision may occur. When this happens, openvswitch
> will make a second pass through NAT which will perform additional packet
> modification. This will update the skb data, but not the flow key that
> OVS uses. This means that future flow lookups, and packet matches will
> have incorrect data. This has been supported since
> 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack").
>
> That commit failed to properly update the sw_flow_key attributes, since
> it only called the ovs_ct_nat_update_key once, rather than each time
> ovs_ct_nat_execute was called. As these two operations are linked, the
> ovs_ct_nat_execute() function should always make sure that the
> sw_flow_key is updated after a successful call through NAT infrastructure.
>
> Fixes: 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack")
> Cc: Dumitru Ceara
> Cc: Numan Siddique
> Signed-off-by: Aaron Conole
You were right about the diff, it really looks messy and I had to apply it to
review it :)
The patch looks fine to me!!
Acked-by: Eelco Chaudron
> ---
> v1->v2: removed forward decl., moved the ovs_nat_update_key function
> made sure it compiles with NF_NAT disabled and enabled
>
> net/openvswitch/conntrack.c | 118 ++--
> 1 file changed, 59 insertions(+), 59 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c07afff57dd3..4a947c13c813 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -734,6 +734,57 @@ static bool skb_nfct_cached(struct net *net,
> }
>
> #if IS_ENABLED(CONFIG_NF_NAT)
> +static void ovs_nat_update_key(struct sw_flow_key *key,
> +const struct sk_buff *skb,
> +enum nf_nat_manip_type maniptype)
> +{
> + if (maniptype == NF_NAT_MANIP_SRC) {
> + __be16 src;
> +
> + key->ct_state |= OVS_CS_F_SRC_NAT;
> + if (key->eth.type == htons(ETH_P_IP))
> + key->ipv4.addr.src = ip_hdr(skb)->saddr;
> + else if (key->eth.type == htons(ETH_P_IPV6))
> + memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
> +sizeof(key->ipv6.addr.src));
> + else
> + return;
> +
> + if (key->ip.proto == IPPROTO_UDP)
> + src = udp_hdr(skb)->source;
> + else if (key->ip.proto == IPPROTO_TCP)
> + src = tcp_hdr(skb)->source;
> + else if (key->ip.proto == IPPROTO_SCTP)
> + src = sctp_hdr(skb)->source;
> + else
> + return;
> +
> + key->tp.src = src;
> + } else {
> + __be16 dst;
> +
> + key->ct_state |= OVS_CS_F_DST_NAT;
> + if (key->eth.type == htons(ETH_P_IP))
> + key->ipv4.addr.dst = ip_hdr(skb)->daddr;
> + else if (key->eth.type == htons(ETH_P_IPV6))
> + memcpy(&key->ipv6.addr.dst, &ipv6_hdr(skb)->daddr,
> +sizeof(key->ipv6.addr.dst));
> + else
> + return;
> +
> + if (key->ip.proto == IPPROTO_UDP)
> + dst = udp_hdr(skb)->dest;
> + else if (key->ip.proto == IPPROTO_TCP)
> + dst = tcp_hdr(skb)->dest;
> + else if (key->ip.proto == IPPROTO_SCTP)
> + dst = sctp_hdr(skb)->dest;
> + else
> + return;
> +
> + key->tp.dst = dst;
> + }
> +}
> +
> /* Modelled after nf_nat_ipv[46]_fn().
> * range is only used for new, uninitialized NAT state.
> * Returns either NF_ACCEPT or NF_DROP.
> @@ -741,7 +792,7 @@ static bool skb_nfct_cached(struct net *net,
> static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> enum ip_conntrack_info ctinfo,
> const struct nf_nat_range2 *range,
> - enum nf_nat_manip_type maniptype)
> + enum nf_nat_manip_type maniptype, struct
> sw_flow_key *key)
> {
> int hooknum, nh_off, err = NF_ACCEPT;
>
> @@ -813,58 +864,11 @@ static int ovs_ct_nat_execute(struct sk_buff *skb,
> struct nf_conn *ct,
> push:
> skb_push_rcsum(skb, nh_off);
>
> - return err;
> -}
> -
> -static void ovs_nat_update_key(struct sw_flow_key *key,
> -const struct sk_buff *skb,
> -enum nf_nat_manip_type maniptype)
> -{
> - if (maniptype == NF_NAT_MANIP_SRC) {
> - __be16 src;
> -
> - key->ct_state |= OVS_CS_F_SRC_NAT;
> - if (key->eth.type == htons(ETH_P_IP))
> - key->ipv4.addr.src = ip_hdr(skb)->saddr;
> - else if (key->eth.type == htons(ETH_P_IPV6))
> - mem
Re: [ovs-dev] [PATCH v4 05/15] list: use short version of safe loops if possible
On 9 Mar 2022, at 17:10, Adrian Moreno wrote: > Using the SHORT version of the *_SAFE loops makes the code cleaner > and less error-prone. So, use the SHORT version and remove the extra > variable when possible. > > In order to be able to use both long and short versions without changing > the name of the macro for all the clients, overload the existing name > and select the appropriate version depending on the number of arguments. > > Acked-by: Dumitru Ceara > Signed-off-by: Adrian Moreno Other than some small nits below, the patch looks good to me. Acked-by: Eelco Chaudron > --- > include/openvswitch/list.h | 30 -- > lib/conntrack.c | 4 ++-- > lib/fat-rwlock.c | 4 ++-- > lib/id-fpool.c | 3 +-- > lib/ipf.c| 22 +++--- > lib/lldp/lldpd-structs.c | 7 +++ > lib/lldp/lldpd.c | 8 > lib/mcast-snooping.c | 12 ++-- > lib/netdev-afxdp.c | 4 ++-- > lib/netdev-dpdk.c| 4 ++-- > lib/ofp-msgs.c | 4 ++-- > lib/ovs-lldp.c | 12 ++-- > lib/ovsdb-idl.c | 30 +++--- > lib/seq.c| 4 ++-- > lib/tnl-ports.c | 16 > lib/unixctl.c| 8 > lib/vconn.c | 4 ++-- > ofproto/connmgr.c| 8 > ofproto/ofproto-dpif-ipfix.c | 4 ++-- > ofproto/ofproto-dpif-trace.c | 4 ++-- > ofproto/ofproto-dpif-xlate.c | 4 ++-- > ofproto/ofproto-dpif.c | 24 +++- > ovsdb/jsonrpc-server.c | 16 > ovsdb/monitor.c | 24 > ovsdb/ovsdb.c| 4 ++-- > ovsdb/raft.c | 15 +++ > ovsdb/transaction-forward.c | 6 +++--- > ovsdb/transaction.c | 28 ++-- > ovsdb/trigger.c | 4 ++-- > tests/test-list.c| 29 + > utilities/ovs-ofctl.c| 4 ++-- > utilities/ovs-vsctl.c| 8 > vswitchd/bridge.c| 16 > vtep/vtep-ctl.c | 12 ++-- > 34 files changed, 218 insertions(+), 168 deletions(-) > > diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h > index bbd2edbd0..6a7d4b2ff 100644 > --- a/include/openvswitch/list.h > +++ b/include/openvswitch/list.h > @@ -92,7 +92,8 @@ static inline bool ovs_list_is_short(const struct ovs_list > *); > CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); > \ > UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) > > -#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST) > \ > +/* LONG version of SAFE iterators */ Ending sentence with a dot. > +#define LIST_FOR_EACH_REVERSE_SAFE_LONG(VAR, PREV, MEMBER, LIST) > \ > for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev, > \ > struct ovs_list); > \ > CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, > \ > @@ -101,7 +102,7 @@ static inline bool ovs_list_is_short(const struct > ovs_list *); >ITER_VAR(PREV) != (LIST)); > \ > UPDATE_MULTIVAR_SAFE_LONG(VAR, PREV)) > > -#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST) > \ > +#define LIST_FOR_EACH_SAFE_LONG(VAR, NEXT, MEMBER, LIST) > \ > for (INIT_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, (LIST)->next, > \ > struct ovs_list); > \ > CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, > \ > @@ -110,6 +111,31 @@ static inline bool ovs_list_is_short(const struct > ovs_list *); >ITER_VAR(NEXT) != (LIST)); > \ > UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT)) Do we really want to keep this MACRO as it’s not being used anywhere? > +/* SHORT version of SAFE iterators */ Ending sentence with a dot. > +#define LIST_FOR_EACH_REVERSE_SAFE_SHORT(VAR, MEMBER, LIST) > \ > +for (INIT_MULTIVAR_SAFE_SHORT(VAR, MEMBER, (LIST)->prev, struct > ovs_list);\ > + CONDITION_MULTIVAR_SAFE_SHORT(VAR, MEMBER, > \ > + ITER_VAR(VAR) != (LIST), > \ > + ITER_NEXT_VAR(VAR) = ITER_VAR(VAR)->prev); > \ > + UPDATE_MULTIVAR_SAFE_SHORT(VAR)) > + > +#define LIST_FOR_EACH_SAFE_SHORT(VAR, MEMBER, LIST) > \ > +for (INIT_MULTIVAR_SAFE_SHORT(VAR, MEMBER, (LIST)->next, struct > ovs_list);\ > + CONDITION_MULTIVAR_SAFE_SHORT(VAR, MEMBER, >
Re: [ovs-dev] [PATCH v4 04/15] list: use multi-variable helpers for list loops
On 9 Mar 2022, at 17:10, Adrian Moreno wrote: > Use multi-variable iteration helpers to rewrite non-safe loops. > > There is an important behavior change compared with the previous > implementation: When the loop ends normally (i.e: not via "break;"), the > object pointer provided by the user is NULL. This is safer because it's > not guaranteed that it would end up pointing a valid address. > > For pop iterator, set the variable to NULL when the loop ends. > > Clang-analyzer has successfully picked the potential null-pointer > dereference on the code that triggered this change (bond.c) and nothing > else has been detected. > > For _SAFE loops, use the LONG version for backwards compatibility. > > Signed-off-by: Adrian Moreno Some nits below, rest looks good: Acked-by: Eelco Chaudron > --- > include/openvswitch/list.h | 73 ++ > ofproto/bond.c | 2 +- > tests/test-list.c | 14 ++-- > 3 files changed, 54 insertions(+), 35 deletions(-) > > diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h > index 8ad5eeb32..bbd2edbd0 100644 > --- a/include/openvswitch/list.h > +++ b/include/openvswitch/list.h > @@ -72,37 +72,48 @@ static inline bool ovs_list_is_empty(const struct > ovs_list *); > static inline bool ovs_list_is_singleton(const struct ovs_list *); > static inline bool ovs_list_is_short(const struct ovs_list *); > > -#define LIST_FOR_EACH(ITER, MEMBER, LIST) \ > -for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);\ > - &(ITER)->MEMBER != (LIST); \ > - ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER)) > -#define LIST_FOR_EACH_CONTINUE(ITER, MEMBER, LIST) \ > -for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER); \ > - &(ITER)->MEMBER != (LIST); \ > - ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER)) > -#define LIST_FOR_EACH_REVERSE(ITER, MEMBER, LIST) \ > -for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);\ > - &(ITER)->MEMBER != (LIST); \ > - ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER)) > -#define LIST_FOR_EACH_REVERSE_SAFE(ITER, PREV, MEMBER, LIST)\ > -for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);\ > - (&(ITER)->MEMBER != (LIST) \ > - ? INIT_CONTAINER(PREV, (ITER)->MEMBER.prev, MEMBER), 1\ > - : 0); \ > - (ITER) = (PREV)) > -#define LIST_FOR_EACH_REVERSE_CONTINUE(ITER, MEMBER, LIST) \ > -for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER); \ > - &(ITER)->MEMBER != (LIST); \ > - ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER)) > -#define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST) \ > -for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER); \ > - (&(ITER)->MEMBER != (LIST)\ > - ? INIT_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1 \ > - : 0);\ > - (ITER) = (NEXT)) > -#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST) \ > -while (!ovs_list_is_empty(LIST)\ > - && (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1)) > +#define LIST_FOR_EACH(VAR, MEMBER, LIST) > \ > +for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->next, struct ovs_list); > \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); > \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) > + > +#define LIST_FOR_EACH_CONTINUE(VAR, MEMBER, LIST) > \ > +for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.next, struct ovs_list); > \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); > \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) > + > +#define LIST_FOR_EACH_REVERSE(VAR, MEMBER, LIST) > \ > +for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->prev, struct ovs_list); > \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); > \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) > + > +#define LIST_FOR_EACH_REVERSE_CONTINUE(VAR, MEMBER, LIST) > \ > +for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.prev, struct ovs_list); > \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); > \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) > + > +#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST) > \ > +for (INIT_MULTIVAR_
Re: [ovs-dev] [PATCH v4 03/15] util: add helpers to overload SAFE macro
On 9 Mar 2022, at 17:10, Adrian Moreno wrote: > Having both LONG and SHORT versions of the SAFE macros with different > names is not very convenient. Add helpers that facilitate overloading > such macros using a single name. > > In order to work around a known issue in MSVC [1], an indirection layer > has to be introduced. > > [1] > https://developercommunity.visualstudio.com/t/-va-args-seems-to-be-trated-as-a-single-parameter/460154 > > Acked-by: Dumitru Ceara > Signed-off-by: Adrian Moreno > --- This looks good to me! Acked-by: Eelco Chaudron Small nit, do we need to update the date in the copyright notice? I keep forgetting if we do or do not. But I guess not, as I do not see it in other files. > include/openvswitch/util.h | 21 + > 1 file changed, 21 insertions(+) > > diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > index 157f8cecd..8184dd3d3 100644 > --- a/include/openvswitch/util.h > +++ b/include/openvswitch/util.h > @@ -268,6 +268,27 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, > const char *, const char *); > #define UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT_VAR) > \ > UPDATE_MULTIVAR(VAR, ITER_VAR(NEXT_VAR)) > > +/* Helpers to allow overloading the *_SAFE iterator macros and select either > + * the LONG or the SHORT version depending on the number of arguments. > + */ > +#define GET_SAFE_MACRO2(_1, _2, NAME, ...) NAME > +#define GET_SAFE_MACRO3(_1, _2, _3, NAME, ...) NAME > +#define GET_SAFE_MACRO4(_1, _2, _3, _4, NAME, ...) NAME > +#define GET_SAFE_MACRO5(_1, _2, _3, _4, _5, NAME, ...) NAME > +#define GET_SAFE_MACRO6(_1, _2, _3, _4, _5, _6, NAME, ...) NAME > +#define GET_SAFE_MACRO(MAX_ARGS) GET_SAFE_MACRO ## MAX_ARGS > + > +/* MSVC treats __VA_ARGS__ as a simple token in argument lists. Introduce > + * a level of indirection to work around that. */ > +#define EXPAND_MACRO(name, args) name args > + > +/* Overload the LONG and the SHORT version of the macros. MAX_ARGS is the > + * maximum number of arguments (i.e: the number of arguments of the LONG > + * version). */ > +#define OVERLOAD_SAFE_MACRO(LONG, SHORT, MAX_ARGS, ...) \ > +EXPAND_MACRO(GET_SAFE_MACRO(MAX_ARGS), \ > + (__VA_ARGS__, LONG, SHORT))(__VA_ARGS__) > + > /* Returns the number of elements in ARRAY. */ > #define ARRAY_SIZE(ARRAY) __ARRAY_SIZE(ARRAY) > > -- > 2.34.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 v4 02/15] util: add safe multi-variable iterators
On 9 Mar 2022, at 17:10, Adrian Moreno wrote: > Safe version of multi-variable iterator helpers declare an internal > variable to store the next value of the iterator temporarily. > > Two versions of the macro are provided, one that still uses the NEXT > variable for backwards compatibility and a shorter version that does not > require the use of an additional variable provided by the user. > > Signed-off-by: Adrian Moreno > --- > include/openvswitch/util.h | 80 ++ > 1 file changed, 80 insertions(+) > > diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > index 2d81e3574..157f8cecd 100644 > --- a/include/openvswitch/util.h > +++ b/include/openvswitch/util.h > @@ -188,6 +188,86 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, > const char *, const char *); > #define UPDATE_MULTIVAR(VAR, NEXT_ITER) > \ > (ITER_VAR(VAR) = NEXT_ITER) > > +/* In the safe version of the multi-variable container iteration, the next > + * value of the iterator is precalculated on the condition expression. > + * This allows for the iterator to be freed inside the loop. > + * > + * Two versions of the macros are provided: > + * > + * * In the _SHORT version, the user does not have to provide a variable to > + * store the next value of the iterator. Instead, a second iterator variable > + * is declared in the INIT_ macro and its name is determined by > + * ITER_NEXT_VAR(OBJECT). > + * > + * * In the _LONG version, the user provides another variable of the same > type > + * as the iterator object variable to store the next containing object. > + * We still declare an iterator variable inside the loop but in this case > it's > + * name is derived from the name of the next containing variable. > + * The value of the next containing object will only be set > + * (via OBJECT_CONTAINING) if an additional condition is statisfied. This > + * second condition must ensure it is safe to call OBJECT_CONTAINING on the > + * next iterator variable. > + * With respect to the value of the next containing object: > + * - Inside of the loop: the variable is either NULL or safe to use. > + * - Outside of the loop: the variable is NULL if the loop ends normally. > + * If the loop ends with a "break;" statement, rules of Inside the loop > + * apply. > + */ > +#define ITER_NEXT_VAR(NAME) NAME ## __iterator__next__ > + > +/* Safe initialization declares both iterators. */ > +#define INIT_MULTIVAR_SAFE_SHORT(VAR, MEMBER, POINTER, ITER_TYPE) > \ > +INIT_MULTIVAR_SAFE_SHORT_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0) > + > +#define INIT_MULTIVAR_SAFE_SHORT_EXP(VAR, MEMBER, POINTER, ITER_TYPE, ...) > \ > +ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER), > \ > +*ITER_NEXT_VAR(VAR) = NULL > + > +/* Evaluate the condition expression and, if satisfied, update the _next_ > + * iterator with the NEXT_EXPR. > + * Both EXPR and NEXT_EXPR should only use ITER_VAR(VAR) and > + * ITER_NEXT_VAR(VAR). > + */ > +#define CONDITION_MULTIVAR_SAFE_SHORT(VAR, MEMBER, EXPR, NEXT_EXPR) > \ > +((EXPR) ? > \ > + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), > \ > + (NEXT_EXPR), 1) : > \ > + (((VAR) = NULL), 0)) > + > +#define UPDATE_MULTIVAR_SAFE_SHORT(VAR) > \ > +UPDATE_MULTIVAR(VAR, ITER_NEXT_VAR(VAR)) > + > +/*_LONG versions of the macros*/ Other than the small nit of adding a space and a dot. It looks fine, i.e. +/* _LONG versions of the macros. */ Acked-by: Eelco Chaudron > + > +#define INIT_MULTIVAR_SAFE_LONG(VAR, NEXT_VAR, MEMBER, POINTER, ITER_TYPE) > \ > +INIT_MULTIVAR_SAFE_LONG_EXP(VAR, NEXT_VAR, MEMBER, POINTER, ITER_TYPE, > \ > +(void) 0) > \ > + > +#define INIT_MULTIVAR_SAFE_LONG_EXP(VAR, NEXT_VAR, MEMBER, POINTER, > \ > +ITER_TYPE, ...) > \ > +ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER), > \ > +*ITER_VAR(NEXT_VAR) = NULL > + > +/* Evaluate the condition expression and, if satisfied, update the _next_ > + * iterator with the NEXT_EXPR. After, evaluate the NEXT_COND and, if > + * satisfied, set the value to NEXT_VAR. NEXT_COND must use > ITER_VAR(NEXT_VAR). > + * > + * Both EXPR and NEXT_EXPR should only use ITER_VAR(VAR) and > + * ITER_VAR(NEXT_VAR). > + */ > +#define CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT_VAR, MEMBER, EXPR, NEXT_EXPR, > \ > + NEXT_COND) > \ > +((EXPR) ? > \ > + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER
Re: [ovs-dev] [PATCH v4 01/15] util: add multi-variable loop iterator macros
On 9 Mar 2022, at 17:10, Adrian Moreno wrote: > Multi-variable loop iterators avoid potential undefined behavior by > using an internal iterator variable to perform the iteration and only > referencing the containing object (via OBJECT_CONTAINING) if the > iterator has been validated via the second expression of the for > statement. > > That way, the user can easily implement a loop that never tries to > obtain the object containing NULL or stack-allocated non-contained > nodes. > > When the loop ends normally (not via "break;") the user-provided > variable is set to NULL. > > Signed-off-by: Adrian Moreno Not sure if this is the patchset to start reviewing on a Friday, but I promised! Went over the first couple of patches (more than once; I hate MACRO’s), and other than some small nits they look fine. I will continue the review later... Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net v2] openvswitch: always update flow key after nat
During NAT, a tuple collision may occur. When this happens, openvswitch
will make a second pass through NAT which will perform additional packet
modification. This will update the skb data, but not the flow key that
OVS uses. This means that future flow lookups, and packet matches will
have incorrect data. This has been supported since
5d50aa83e2c8 ("openvswitch: support asymmetric conntrack").
That commit failed to properly update the sw_flow_key attributes, since
it only called the ovs_ct_nat_update_key once, rather than each time
ovs_ct_nat_execute was called. As these two operations are linked, the
ovs_ct_nat_execute() function should always make sure that the
sw_flow_key is updated after a successful call through NAT infrastructure.
Fixes: 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack")
Cc: Dumitru Ceara
Cc: Numan Siddique
Signed-off-by: Aaron Conole
---
v1->v2: removed forward decl., moved the ovs_nat_update_key function
made sure it compiles with NF_NAT disabled and enabled
net/openvswitch/conntrack.c | 118 ++--
1 file changed, 59 insertions(+), 59 deletions(-)
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index c07afff57dd3..4a947c13c813 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -734,6 +734,57 @@ static bool skb_nfct_cached(struct net *net,
}
#if IS_ENABLED(CONFIG_NF_NAT)
+static void ovs_nat_update_key(struct sw_flow_key *key,
+ const struct sk_buff *skb,
+ enum nf_nat_manip_type maniptype)
+{
+ if (maniptype == NF_NAT_MANIP_SRC) {
+ __be16 src;
+
+ key->ct_state |= OVS_CS_F_SRC_NAT;
+ if (key->eth.type == htons(ETH_P_IP))
+ key->ipv4.addr.src = ip_hdr(skb)->saddr;
+ else if (key->eth.type == htons(ETH_P_IPV6))
+ memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
+ sizeof(key->ipv6.addr.src));
+ else
+ return;
+
+ if (key->ip.proto == IPPROTO_UDP)
+ src = udp_hdr(skb)->source;
+ else if (key->ip.proto == IPPROTO_TCP)
+ src = tcp_hdr(skb)->source;
+ else if (key->ip.proto == IPPROTO_SCTP)
+ src = sctp_hdr(skb)->source;
+ else
+ return;
+
+ key->tp.src = src;
+ } else {
+ __be16 dst;
+
+ key->ct_state |= OVS_CS_F_DST_NAT;
+ if (key->eth.type == htons(ETH_P_IP))
+ key->ipv4.addr.dst = ip_hdr(skb)->daddr;
+ else if (key->eth.type == htons(ETH_P_IPV6))
+ memcpy(&key->ipv6.addr.dst, &ipv6_hdr(skb)->daddr,
+ sizeof(key->ipv6.addr.dst));
+ else
+ return;
+
+ if (key->ip.proto == IPPROTO_UDP)
+ dst = udp_hdr(skb)->dest;
+ else if (key->ip.proto == IPPROTO_TCP)
+ dst = tcp_hdr(skb)->dest;
+ else if (key->ip.proto == IPPROTO_SCTP)
+ dst = sctp_hdr(skb)->dest;
+ else
+ return;
+
+ key->tp.dst = dst;
+ }
+}
+
/* Modelled after nf_nat_ipv[46]_fn().
* range is only used for new, uninitialized NAT state.
* Returns either NF_ACCEPT or NF_DROP.
@@ -741,7 +792,7 @@ static bool skb_nfct_cached(struct net *net,
static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
enum ip_conntrack_info ctinfo,
const struct nf_nat_range2 *range,
- enum nf_nat_manip_type maniptype)
+ enum nf_nat_manip_type maniptype, struct
sw_flow_key *key)
{
int hooknum, nh_off, err = NF_ACCEPT;
@@ -813,58 +864,11 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct
nf_conn *ct,
push:
skb_push_rcsum(skb, nh_off);
- return err;
-}
-
-static void ovs_nat_update_key(struct sw_flow_key *key,
- const struct sk_buff *skb,
- enum nf_nat_manip_type maniptype)
-{
- if (maniptype == NF_NAT_MANIP_SRC) {
- __be16 src;
-
- key->ct_state |= OVS_CS_F_SRC_NAT;
- if (key->eth.type == htons(ETH_P_IP))
- key->ipv4.addr.src = ip_hdr(skb)->saddr;
- else if (key->eth.type == htons(ETH_P_IPV6))
- memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
- sizeof(key->ipv6.addr.src));
- else
- return;
-
- if (key->ip.proto == IPPROTO_UDP)
- src = udp_hdr(skb)->source;
- else if (key->ip.proto == IP
Re: [ovs-dev] [PATCH v20 5/8] dpif-offload-netlink: Implement dpif-offload-provider API
On 17 Mar 2022, at 2:01, Chris Mi wrote:
> On 2022-03-11 8:53 PM, Eelco Chaudron wrote:
>>
>
> @@ -449,6 +462,7 @@ dpif_close(struct dpif *dpif)
>if (dpif) {
>struct registered_dpif_class *rc;
>
> +dpif_offload_close(dpif);
** Not sure I understand, but why are we destroying the offload dpif class
here, it can be used by another dpif type.
** I guess this is all because your design has a 1:1 mapping? Guess it
should be two dpif_types that could share the same offload class type.
>>> Now it is moved to dpif_netlink_close().
>>>
>>> Except the 1:1 mapping comment which I think need Ilya's feedback, I have
>>> addressed your other comments.
>>> Thanks for your comments. The dpif-offload for dummy is not needed and
>>> removed.
>>> If needed, I can send v21.
>>
>> Thanks for taking care of the questions and fixing them in your sandbox.
>> I would prefer for you to not send any more revisions until we have a clear
>> answer from Ilya.
> Since Ilya didn't reply, I'll send a new version to reflect the latest change.
Well, my goal was to not do any more reviews until Ilya would reply, as every
review cycle takes up quite some time.
However I could not get v21 to apply cleanly, so I will hold off on any further
reviews of this series until we have a clear direction from Ilya.
//Eelco
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 18/18] python: add unit tests for filtering engine
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > Add unit test for OFFilter class. > > Acked-by: Eelco Chaudron > Signed-off-by: Adrian Moreno > --- This was the final patch in the series, and it all looks good. One small question on patch 3 as I noticed the code change post-ack. Thanks, Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 07/18] python: introduce OpenFlow Flow parsing
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > Introduce OFPFlow class and all its decoders. > > Most of the decoders are generic (from decoders.py). Some have special > syntax and need a specific implementation. > > Decoders for nat are moved to the common decoders.py because it's syntax > is shared with other types of flows (e.g: dpif flows). > > Signed-off-by: Adrian Moreno Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 17/18] python: add unit tests to datapath parsing
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > Signed-off-by: Adrian Moreno > --- Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 16/18] python: add unit tests for openflow parsing
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > Add unit tests for OFPFlow class and ip-port range decoder > > Signed-off-by: Adrian Moreno > --- Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 13/18] python: detect changes in flow formatting code
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > In order to minimize the risk of having the python flow parsing code and > the C flow formatting code divert, add a target that checks if the > formatting code has been changed since the last revision and warn the > developer if it has. > > The script also makes it easy to update the dependency file so hopefully > it will not cause too much trouble for a developer that has modifed the > file without changing the flow string format. > > Signed-off-by: Adrian Moreno > --- Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 05/18] build-aux: generate ofp field decoders
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > Based on meta-field information extracted by extract_ofp_fields, > autogenerate the right decoder to be used. > > Acked-by: Eelco Chaudron > Signed-off-by: Adrian Moreno Additional v3 changes look good to me. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 04/18] build-aux: split extract-ofp-fields
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > In order to be able to reuse the core extraction logic, split the command > in two parts. The core extraction logic is moved to python/build while > the command that writes the different files out of the extracted field > info is kept in build-aux. > > Signed-off-by: Adrian Moreno Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 03/18] python: add list parser
On 11 Mar 2022, at 16:21, Adrian Moreno wrote:
> Some openflow or dpif flows encode their arguments in lists, eg:
> "some_action(arg1,arg2,arg3)". In order to decode this in a way that can
> be then stored and queried, add ListParser and ListDecoders classes
> that parse lists into KeyValue instances.
>
> The ListParser / ListDecoders mechanism is quite similar to KVParser and
> KVDecoders. Since the "key" of the different KeyValue objects is now
> ommited, it has to be provided by ListDecoders.
>
> For example, take the openflow action "resubmit" that can be written as:
>
> resubmit([port],[table][,ct])
>
> Can be decoded by creating a ListDecoders instance such as:
>
> ListDecoders([
> ("port", decode_default),
> ("table", decode_int),
> ("ct", decode_flag),
> ])
>
> Naturally, the order of the decoders must be kept.
>
> Acked-by: Eelco Chaudron
> Signed-off-by: Adrian Moreno
> ---
> python/automake.mk | 1 +
> python/ovs/flows/list.py | 121 +++
> 2 files changed, 122 insertions(+)
> create mode 100644 python/ovs/flows/list.py
>
> diff --git a/python/automake.mk b/python/automake.mk
> index 7ce842d66..73438d615 100644
> --- a/python/automake.mk
> +++ b/python/automake.mk
> @@ -29,6 +29,7 @@ ovs_pyfiles = \
> python/ovs/flows/__init__.py \
> python/ovs/flows/decoders.py \
> python/ovs/flows/kv.py \
> + python/ovs/flows/list.py \
> python/ovs/json.py \
> python/ovs/jsonrpc.py \
> python/ovs/ovsuuid.py \
> diff --git a/python/ovs/flows/list.py b/python/ovs/flows/list.py
> new file mode 100644
> index 0..57e7c5908
> --- /dev/null
> +++ b/python/ovs/flows/list.py
> @@ -0,0 +1,121 @@
> +import re
> +
> +from ovs.flows.kv import KeyValue, KeyMetadata, ParseError
> +from ovs.flows.decoders import decode_default
> +
> +
> +class ListDecoders(object):
> +"""ListDecoders is used by ListParser to decode the elements in the list.
> +
> +A decoder is a function that accepts a value and returns its decoded
> +object.
> +
> +ListDecoders is initialized with a list of tuples that contains the
> +keyword and the decoding function associated with each position in the
> +list. The order is, therefore, important.
> +
> +Args:
> +decoders (list of tuples): Optional; a list of tuples.
> +The first element in the tuple is the keyword associated with the
> +value. The second element in the tuple is the decoder function.
> +"""
> +
> +def __init__(self, decoders=None):
> +self._decoders = decoders or list()
> +
> +def decode(self, index, value_str):
> +"""Decode the index'th element of the list.
> +
> +Args:
> +index (int): The position in the list of the element to decode.
> +value_str (str): The value string to decode.
> +"""
> +if index < 0 or index >= len(self._decoders):
> +return self._default_decoder(index, value_str)
> +
> +try:
> +key = self._decoders[index][0]
> +value = self._decoders[index][1](value_str)
> +return key, value
> +except Exception as e:
> +raise ParseError(
> +"Failed to decode value_str {}: {}".format(value_str, str(e))
> +)
> +
> +@staticmethod
> +def _default_decoder(index, value):
> +key = "elem_{}".format(index)
> +return key, decode_default(value)
> +
> +
> +class ListParser(object):
> +"""ListParser parses a list of values and stores them as key-value pairs.
> +
> +It uses a ListDecoders instance to decode each element in the list.
> +
> +Args:
> +string (str): The string to parse.
> +decoders (ListDecoders): Optional, the decoders to use.
> +delims (list): Optional, list of delimiters of the list. Defaults to
> +[','].
> +"""
> +def __init__(self, string, decoders=None, delims=[","]):
> +self._string = string
> +self._decoders = decoders or ListDecoders()
> +self._keyval = list()
> +self._regexp = r"({})".format("|".join(delims))
> +
> +def kv(self):
> +return self._keyval
> +
> +def __iter__(self):
> +return iter(self._keyval)
> +
> +def parse(self):
> +"""Parse the list in string.
> +
> +Raises:
> +ParseError if any parsing error occurs.
> +"""
> +kpos = 0
> +index = 0
> +while kpos < len(self._string) and self._string[kpos] != "\n":
> +split_parts = re.split(self._regexp, self._string[kpos:], 1)
Was there a specific reason why the below code was removed? Are we accepting
the exception in this case?
-+if len(split_parts) == 0:
-+break
> +value_str = split_parts[0]
> +
> +key, value = self._decoders.decode(index,
Re: [ovs-dev] [PATCH v3 02/18] python: add mask, ip and eth decoders
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > Add more decoders that can be used by KVParser. > > For IPv4 and IPv6 addresses, create a new class that wraps > netaddr.IPAddress. > For Ethernet addresses, create a new class that wraps netaddr.EUI. > For Integers, create a new class that performs basic bitwise mask > comparisons > > Acked-by: Eelco Chaudron > Signed-off-by: Adrian Moreno > --- Additional v3 changes look good to me. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 01/18] python: add generic Key-Value parser
On 11 Mar 2022, at 16:21, Adrian Moreno wrote: > Most of ofproto and dpif flows are based on key-value pairs. These > key-value pairs can be represented in several ways, eg: key:value, > key=value, key(value). > > Add the following classes that allow parsing of key-value strings: > * KeyValue: holds a key-value pair > * KeyMetadata: holds some metadata associated with a KeyValue such as > the original key and value strings and their position in the global > string > * KVParser: is able to parse a string and extract it's key-value pairs > as KeyValue instances. Before creating the KeyValue instance it tries > to decode the value via the KVDecoders > * KVDecoders holds a number of decoders that KVParser can use to decode > key-value pairs. It accepts a dictionary of keys and callables to > allow users to specify what decoder (i.e: callable) to use for each > key > > Also, flake8 seems to be incorrectly reporting an error (E203) in: > "slice[index + offset : index + offset]" which is PEP8 compliant. So, > ignore this error. > > Signed-off-by: Adrian Moreno Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
