[ovs-dev] [PATCH v4 1/2] userspace: Respect tso/gso segment size.
From: Flavio Leitner
Currently OVS will calculate the segment size based on the
MTU of the egress port. That usually happens to be correct
when the ports share the same MTU, but that is not always true.
Therefore, if the segment size is provided, then use that and
make sure the over sized packets are dropped.
Signed-off-by: Flavio Leitner
Co-authored-by: Mike Pattrick
Signed-off-by: Mike Pattrick
---
v3: Corrected sparse error
---
lib/dp-packet.c| 3 ++
lib/dp-packet.h| 26
lib/netdev-dpdk.c | 12 +++-
lib/netdev-linux.c | 76 --
4 files changed, 94 insertions(+), 23 deletions(-)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 27114a9a9..4d92a2e5b 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -34,6 +34,7 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum
dp_packet_source so
pkt_metadata_init(>md, 0);
dp_packet_reset_cutlen(b);
dp_packet_reset_offload(b);
+dp_packet_set_tso_segsz(b, 0);
/* Initialize implementation-specific fields of dp_packet. */
dp_packet_init_specific(b);
/* By default assume the packet type to be Ethernet. */
@@ -199,6 +200,8 @@ dp_packet_clone_with_headroom(const struct dp_packet
*buffer, size_t headroom)
*dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer);
*dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK;
+dp_packet_set_tso_segsz(new_buffer, dp_packet_get_tso_segsz(buffer));
+
if (dp_packet_rss_valid(buffer)) {
dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer));
}
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 70ddf8aa4..6a924f3ff 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -126,6 +126,7 @@ struct dp_packet {
uint32_t ol_flags; /* Offloading flags. */
uint32_t rss_hash; /* Packet hash. */
uint32_t flow_mark; /* Packet flow mark. */
+uint16_t tso_segsz; /* TCP TSO segment size. */
#endif
enum dp_packet_source source; /* Source of memory allocated as 'base'. */
@@ -166,6 +167,9 @@ static inline void dp_packet_set_size(struct dp_packet *,
uint32_t);
static inline uint16_t dp_packet_get_allocated(const struct dp_packet *);
static inline void dp_packet_set_allocated(struct dp_packet *, uint16_t);
+static inline uint16_t dp_packet_get_tso_segsz(const struct dp_packet *);
+static inline void dp_packet_set_tso_segsz(struct dp_packet *, uint16_t);
+
void *dp_packet_resize_l2(struct dp_packet *, int increment);
void *dp_packet_resize_l2_5(struct dp_packet *, int increment);
static inline void *dp_packet_eth(const struct dp_packet *);
@@ -644,6 +648,17 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
b->mbuf.buf_len = s;
}
+static inline uint16_t
+dp_packet_get_tso_segsz(const struct dp_packet *p)
+{
+return p->mbuf.tso_segsz;
+}
+
+static inline void
+dp_packet_set_tso_segsz(struct dp_packet *p, uint16_t s)
+{
+p->mbuf.tso_segsz = s;
+}
#else /* DPDK_NETDEV */
static inline void
@@ -700,6 +715,17 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
b->allocated_ = s;
}
+static inline uint16_t
+dp_packet_get_tso_segsz(const struct dp_packet *p)
+{
+return p->tso_segsz;
+}
+
+static inline void
+dp_packet_set_tso_segsz(struct dp_packet *p, uint16_t s)
+{
+p->tso_segsz = s;
+}
#endif /* DPDK_NETDEV */
static inline void
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 63dac689e..1c100c48e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2302,6 +2302,7 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev,
struct rte_mbuf *mbuf)
if (mbuf->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
struct tcp_header *th = dp_packet_l4(pkt);
+int hdr_len;
if (!th) {
VLOG_WARN_RL(, "%s: TCP Segmentation without L4 header"
@@ -2311,7 +2312,15 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev,
struct rte_mbuf *mbuf)
mbuf->l4_len = TCP_OFFSET(th->tcp_ctl) * 4;
mbuf->ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
+hdr_len = mbuf->l2_len + mbuf->l3_len + mbuf->l4_len;
mbuf->tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;
+if (OVS_UNLIKELY((hdr_len + mbuf->tso_segsz) > dev->max_packet_len)) {
+VLOG_WARN_RL(, "%s: Oversized TSO packet. "
+ "hdr: %"PRIu32", gso: %"PRIu32", max len: %"PRIu32"",
+ dev->up.name, hdr_len, mbuf->tso_segsz,
+ dev->max_packet_len);
+return false;
+}
if (mbuf->ol_flags & RTE_MBUF_F_TX_IPV4) {
mbuf->ol_flags |= RTE_MBUF_F_TX_IP_CKSUM;
@@ -2586,7 +2595,8 @@ netdev_dpdk_filter_packet_len(struct netdev_dpdk *dev,
struct rte_mbuf **pkts,
int cnt = 0;
struct rte_mbuf *pkt;
-/* Filter oversized packets, unless are marked for TSO. */
+/* Filter oversized packets. The TSO packets are
[ovs-dev] [PATCH v4 2/2] userspace: Add Generic Segmentation Offloading.
From: Flavio Leitner
This provides a software implementation in the case
the egress netdev doesn't support segmentation in hardware.
The challenge here is to guarantee packet ordering in the
original batch that may be full of TSO packets. Each TSO
packet can go up to ~64kB, so with segment size of 1440
that means about 44 packets for each TSO. Each batch has
32 packets, so the total batch amounts to 1408 normal
packets.
The segmentation estimates the total number of packets
and then the total number of batches. Then allocate
enough memory and finally do the work.
Finally each batch is sent in order to the netdev.
Signed-off-by: Flavio Leitner
Co-authored-by: Mike Pattrick
Signed-off-by: Mike Pattrick
---
v4:
- Various formatting changes
- Fixed memory leak in soft-gso code if packet is flagged
for GSO but incorrectly lacks segment size.
---
lib/automake.mk | 2 +
lib/dp-packet-gso.c | 173
lib/dp-packet-gso.h | 23 ++
lib/dp-packet.h | 7 ++
lib/netdev-dpdk.c | 44 ---
lib/netdev-linux.c | 58 ---
lib/netdev.c| 134 +-
lib/packets.c | 4 +-
8 files changed, 322 insertions(+), 123 deletions(-)
create mode 100644 lib/dp-packet-gso.c
create mode 100644 lib/dp-packet-gso.h
diff --git a/lib/automake.mk b/lib/automake.mk
index e64ee76ce..49a92958d 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
lib/dpctl.h \
lib/dp-packet.h \
lib/dp-packet.c \
+ lib/dp-packet-gso.c \
+ lib/dp-packet-gso.h \
lib/dpdk.h \
lib/dpif-netdev-extract-study.c \
lib/dpif-netdev-lookup.h \
diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
new file mode 100644
index 0..6a007fdad
--- /dev/null
+++ b/lib/dp-packet-gso.c
@@ -0,0 +1,173 @@
+/*
+ * Copyright (c) 2023 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+#include
+#include
+
+#include "coverage.h"
+#include "dp-packet.h"
+#include "dp-packet-gso.h"
+#include "netdev-provider.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(dp_packet_gso);
+
+COVERAGE_DEFINE(soft_seg_good);
+COVERAGE_DEFINE(soft_seg_drop);
+
+/* Retuns a new packet that is a segment of packet 'p'.
+ *
+ * The new packet is initialized with 'hdr_len' bytes from the
+ * start of packet 'p' and then appended with 'data_len' bytes
+ * from the 'data' buffer.
+ *
+ * Note: The packet headers are not updated. */
+static struct dp_packet *
+dp_packet_gso_seg_new(const struct dp_packet *p, size_t hdr_len,
+ const char *data, size_t data_len)
+{
+struct dp_packet *seg = dp_packet_new_with_headroom(hdr_len + data_len,
+dp_packet_headroom(p));
+
+/* Append the original packet headers and then the payload. */
+dp_packet_put(seg, dp_packet_data(p), hdr_len);
+dp_packet_put(seg, data, data_len);
+
+/* The new segment should have the same offsets. */
+seg->l2_5_ofs = p->l2_5_ofs;
+seg->l3_ofs = p->l3_ofs;
+seg->l4_ofs = p->l4_ofs;
+
+/* The protocol headers remain the same, so preserve hash and mark. */
+*dp_packet_rss_ptr(seg) = dp_packet_get_rss_hash(p);
+*dp_packet_flow_mark_ptr(seg) = *dp_packet_flow_mark_ptr(p);
+
+/* The segment should inherit all the offloading flags from the
+ * original packet, except for the TCP segmentation, external
+ * buffer and indirect buffer flags. */
+*dp_packet_ol_flags_ptr(seg) = *dp_packet_ol_flags_ptr(p)
+& DP_PACKET_OL_SUPPORTED_MASK;
+
+dp_packet_hwol_reset_tcp_seg(seg);
+
+return seg;
+}
+
+/* Returns the calculated number of TCP segments in packet 'p'. */
+int
+dp_packet_gso_nr_segs(struct dp_packet *p)
+{
+uint16_t segsz = dp_packet_get_tso_segsz(p);
+const char *data_tail;
+const char *data_pos;
+
+data_pos = dp_packet_get_tcp_payload(p);
+data_tail = (char *) dp_packet_tail(p) - dp_packet_l2_pad_size(p);
+
+return DIV_ROUND_UP((data_tail - data_pos), segsz);
+
+}
+
+/* Perform software segmentation on packet 'p'.
+ *
+ * Returns all the segments added to the array of preallocated
+ * batches in 'batches' starting at batch position 'batch_pos'. */
+bool
+dp_packet_gso(struct dp_packet *p, struct dp_packet_batch **batches)
+{
+static struct vlog_rate_limit rl =
Re: [ovs-dev] [PATCH v3 2/2] userspace: Add Generic Segmentation Offloading.
On Tue, Jul 4, 2023 at 9:00 PM Ilya Maximets wrote:
>
> On 6/21/23 22:36, Mike Pattrick wrote:
> > From: Flavio Leitner
> >
> > This provides a software implementation in the case
> > the egress netdev doesn't support segmentation in hardware.
> >
> > The challenge here is to guarantee packet ordering in the
> > original batch that may be full of TSO packets. Each TSO
> > packet can go up to ~64kB, so with segment size of 1440
> > that means about 44 packets for each TSO. Each batch has
> > 32 packets, so the total batch amounts to 1408 normal
> > packets.
> >
> > The segmentation estimates the total number of packets
> > and then the total number of batches. Then allocate
> > enough memory and finally do the work.
> >
> > Finally each batch is sent in order to the netdev.
> >
> > Signed-off-by: Flavio Leitner
> > Co-authored-by: Mike Pattrick
> > Signed-off-by: Mike Pattrick
> > ---
> > lib/automake.mk | 2 +
> > lib/dp-packet-gso.c | 172
> > lib/dp-packet-gso.h | 24 +++
> > lib/dp-packet.h | 11 +++
> > lib/netdev-dpdk.c | 46
> > lib/netdev-linux.c | 58 ---
> > lib/netdev.c| 120 ++-
> > lib/packets.c | 4 +-
> > 8 files changed, 314 insertions(+), 123 deletions(-)
> > create mode 100644 lib/dp-packet-gso.c
> > create mode 100644 lib/dp-packet-gso.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index e64ee76ce..49a92958d 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> > lib/dpctl.h \
> > lib/dp-packet.h \
> > lib/dp-packet.c \
> > + lib/dp-packet-gso.c \
> > + lib/dp-packet-gso.h \
> > lib/dpdk.h \
> > lib/dpif-netdev-extract-study.c \
> > lib/dpif-netdev-lookup.h \
> > diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
> > new file mode 100644
> > index 0..bc72e2f90
> > --- /dev/null
> > +++ b/lib/dp-packet-gso.c
> > @@ -0,0 +1,172 @@
> > +/*
> > + * Copyright (c) 2021 Red Hat, Inc.
>
> Need to adjust the year. :)
>
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include "coverage.h"
> > +#include "dp-packet.h"
> > +#include "dp-packet-gso.h"
> > +#include "netdev-provider.h"
> > +#include "openvswitch/vlog.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(dp_packet_gso);
> > +
> > +COVERAGE_DEFINE(soft_seg_good);
> > +
> > +/* Retuns a new packet that is a segment of packet 'p'.
> > + *
> > + * The new packet is initialized with 'hdr_len' bytes from the
> > + * start of packet 'p' and then appended with 'data_len' bytes
> > + * from the 'data' buffer.
> > + *
> > + * Note: The packet headers are not updated. */
> > +static struct dp_packet *
> > +dp_packet_gso_seg_new(const struct dp_packet *p, size_t hdr_len,
> > + const char *data, size_t data_len)
> > +{
> > +struct dp_packet *seg = dp_packet_new_with_headroom(hdr_len + data_len,
> > +
> > dp_packet_headroom(p));
> > +
> > +/* Append the original packet headers and then the payload. */
> > +dp_packet_put(seg, dp_packet_data(p), hdr_len);
> > +dp_packet_put(seg, data, data_len);
> > +
> > +/* The new segment should have the same offsets. */
> > +seg->l2_5_ofs = p->l2_5_ofs;
> > +seg->l3_ofs = p->l3_ofs;
> > +seg->l4_ofs = p->l4_ofs;
> > +
> > +/* The protocol headers remain the same, so preserve hash and mark. */
> > +*dp_packet_rss_ptr(seg) = dp_packet_get_rss_hash(p);
> > +*dp_packet_flow_mark_ptr(seg) = *dp_packet_flow_mark_ptr(p);
> > +
> > +/* The segment should inherit all the offloading flags from the
> > + * original packet, except for the TCP segmentation, external
> > + * buffer and indirect buffer flags. */
> > +*dp_packet_ol_flags_ptr(seg) = *dp_packet_ol_flags_ptr(p)
> > +& ~(DP_PACKET_OL_TX_TCP_SEG | DP_PACKET_OL_EXTERNAL
> > +| DP_PACKET_OL_INDIRECT);
>
> We should inherit DP_PACKET_OL_SUPPORTED_MASK & ~DP_PACKET_OL_TX_TCP_SEG.
> So,
>
> *dp_packet_ol_flags_ptr(seg) &= DP_PACKET_OL_SUPPORTED_MASK
> & ~DP_PACKET_OL_TX_TCP_SEG;
>
> Is that right?
>
> > +
> > +dp_packet_hwol_reset_tcp_seg(seg);
>
> Also, this function just resets the
[ovs-dev] [PATCH v5] python: Add async DNS support
This adds a Python version of the async DNS support added in:
771680d96 DNS: Add basic support for asynchronous DNS resolving
The above version uses the unbound C library, and this
implimentation uses the SWIG-wrapped Python version of that.
In the event that the Python unbound library is not available,
a warning will be logged and the resolve() method will just
return None. For the case where inet_parse_active() is passed
an IP address, it will not try to resolve it, so existing
behavior should be preserved in the case that the unbound
library is unavailable.
Intentional differences from the C version are as follows:
OVS_HOSTS_FILE environment variable can bet set to override
the system 'hosts' file. This is primarily to allow testing to
be done without requiring network connectivity.
Since resolution can still be done via hosts file lookup, DNS
lookups are not disabled when resolv.conf cannot be loaded.
The Python socket_util module has fallen behind its C equivalent.
The bare minimum change was done to inet_parse_active() to support
sync/async dns, as there is no equivalent to
parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
was added to bring socket_util.py up to equivalency to the C
version.
Signed-off-by: Terry Wilson
---
.github/workflows/build-and-test.yml| 4 +-
Documentation/intro/install/general.rst | 4 +-
Documentation/intro/install/rhel.rst| 2 +-
Documentation/intro/install/windows.rst | 2 +-
NEWS| 3 +
debian/control.in | 1 +
m4/openvswitch.m4 | 8 +-
python/TODO.rst | 7 +
python/automake.mk | 2 +
python/ovs/dns_resolve.py | 286
python/ovs/socket_util.py | 21 +-
python/ovs/stream.py| 2 +-
python/ovs/tests/test_dns_resolve.py| 280 +++
python/setup.py | 6 +-
rhel/openvswitch-fedora.spec.in | 2 +-
tests/vlog.at | 2 +
16 files changed, 615 insertions(+), 17 deletions(-)
create mode 100644 python/ovs/dns_resolve.py
create mode 100644 python/ovs/tests/test_dns_resolve.py
diff --git a/.github/workflows/build-and-test.yml
b/.github/workflows/build-and-test.yml
index f66ab43b0..47d239f10 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -183,10 +183,10 @@ jobs:
run: sudo apt update || true
- name: install common dependencies
run: sudo apt install -y ${{ env.dependencies }}
-- name: install libunbound libunwind
+- name: install libunbound libunwind python3-unbound
# GitHub Actions doesn't have 32-bit versions of these libraries.
if: matrix.m32 == ''
- run: sudo apt install -y libunbound-dev libunwind-dev
+ run: sudo apt install -y libunbound-dev libunwind-dev python3-unbound
- name: install 32-bit libraries
if: matrix.m32 != ''
run: sudo apt install -y gcc-multilib
diff --git a/Documentation/intro/install/general.rst
b/Documentation/intro/install/general.rst
index 42b5682fd..19e360d47 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -90,7 +90,7 @@ need the following software:
If libcap-ng is installed, then Open vSwitch will automatically build with
support for it.
-- Python 3.4 or later.
+- Python 3.6 or later.
- Unbound library, from http://www.unbound.net, is optional but recommended if
you want to enable ovs-vswitchd and other utilities to use DNS names when
@@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the
following software:
from iproute2 (part of all major distributions and available at
https://wiki.linuxfoundation.org/networking/iproute2).
-- Python 3.4 or later.
+- Python 3.6 or later.
On Linux you should ensure that ``/dev/urandom`` exists. To support TAP
devices, you must also ensure that ``/dev/net/tun`` exists.
diff --git a/Documentation/intro/install/rhel.rst
b/Documentation/intro/install/rhel.rst
index d1fc42021..f2151d890 100644
--- a/Documentation/intro/install/rhel.rst
+++ b/Documentation/intro/install/rhel.rst
@@ -92,7 +92,7 @@ Once that is completed, remove the file ``/tmp/ovs.spec``.
If python3-sphinx package is not available in your version of RHEL, you can
install it via pip with 'pip install sphinx'.
-Open vSwitch requires python 3.4 or newer which is not available in older
+Open vSwitch requires python 3.6 or newer which is not available in older
distributions. In the case of RHEL 6.x and its derivatives, one option is
to install python34 from `EPEL`_.
diff --git a/Documentation/intro/install/windows.rst
b/Documentation/intro/install/windows.rst
index 78f60f35a..fce099d5d 100644
--- a/Documentation/intro/install/windows.rst
+++ b/Documentation/intro/install/windows.rst
@@
Re: [ovs-dev] [PATCH v12 6/8] ovs-vsctl: Fix crash when routing is enabled
On 7/11/23 12:22, Eelco Chaudron wrote: > > > On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: > >> In the case where routing is enabled, the bridge member of the >> `vsctl_port` structs is not populated. This can cause a crash if we >> attempt to access it. This patch fixes the crash by checking if the >> bridge member is valid before attempting to access it. In the >> `check_conflicts` function, we print both the port name and the bridge >> name if routing is disabled and we only print the port name if routing >> is enabled. >> >> Signed-off-by: James Raphael Tiovalen >> Reviewed-by: Simon Horman > > Thank for fixing this James, this patch looks good to me. > > Acked-by: Eelco Chaudron Thanks, James, Simon and Eelco! Applied this one. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 5/8] file, monitor: Add null pointer assertions for old and new ovsdb_rows
On 7/11/23 17:08, Eelco Chaudron wrote: > > > On 11 Jul 2023, at 16:38, Ilya Maximets wrote: > >> On 7/11/23 12:17, Eelco Chaudron wrote: >>> >>> >>> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: >>> This commit adds non-null pointer assertions in some code that performs some decisions based on old and new input ovsdb_rows. Signed-off-by: James Raphael Tiovalen Reviewed-by: Simon Horman >>> >>> What about error messages/argument checking instead of asserts here? >> >> File transactions must have some data. Every row must have at least >> uuid and version. New rows should have 'new' versions of the data, >> deleted should have 'old', and modified should have both. It's a >> bug somewhere if we have a row without any data. > > > Thanks for confirming/clarification. With this; > > Acked-by: Eelco Chaudron Thanks, James, Simon and Eelco! Applied this one. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 4/8] ovsdb: Assert and check return values of `ovsdb_table_schema_get_column`
On 7/11/23 16:41, Eelco Chaudron wrote: > > > On 11 Jul 2023, at 16:33, Ilya Maximets wrote: > >> On 7/11/23 12:13, Eelco Chaudron wrote: >>> >>> >>> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: >>> This commit adds a few null pointer assertions and checks to some return values of `ovsdb_table_schema_get_column`. If a null pointer is encountered in these blocks, either the assertion will fail or the control flow will now be redirected to alternative paths which will output the appropriate error messages. A few ovsdb-rbac and ovsdb-server tests are also updated to verify the expected warning logs by adding said logs to the ALLOWLIST of the OVSDB_SERVER_SHUTDOWN statements. Signed-off-by: James Raphael Tiovalen >>> >>> In general, this looks good, however, I’m not a ovsdb guy, so I’m wondering >>> if the asserts could not cause any additional crashes that can be avoided >>> by a different type of error handling. Also is there an easy way to make >>> this crash happen by giving some invalid input? >>> >>> Ilya any comments here? >> >> '_uuid' and '_version' are internal columns that must always exist. >> They are not part of the schema, ovsdb-server generates them. So, >> it must be an internal bug if they do not exist. > > Thanks for confirming/clarification. With this; > > Acked-by: Eelco Chaudron Thanks, James, Simon and Eelco! Applied this one. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values
On 7/6/23 09:25, Adrian Moreno wrote:
>
>
> On 7/5/23 22:02, Sayali Naval (sanaval) via dev wrote:
>> As per the Open vSwitch Manual
>> (http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge
>> IPFIX parameters can be passed as follows:
>>
>> ovs-vsctl -- set Bridge br0 ipfix=@i \
>>-- --id=@i create IPFIX targets=\"192.168.0.34:4739\"
>> obs_do‐
>>main_id=123 obs_point_id=456
>> cache_active_timeout=60
>>cache_max_flows=13 \
>>other_config:enable-input-sampling=false \
>>other_config:enable-output-sampling=false
>>
>> where the default values are:
>> enable_input_sampling: true
>> enable_output_sampling: true
>>
>> But in the existing code
>> https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1563-L1567,
>> these 2 parameters take up unexpected values in some scenarios.
>>
>> be_opts.enable_input_sampling =
>> !smap_get_bool(_cfg->other_config,
>>"enable-input-sampling",
>> false);
>>
>> be_opts.enable_output_sampling =
>> !smap_get_bool(_cfg->other_config,
>>"enable-output-sampling",
>> false);
>>
>> Here, the function smap_get_bool is being used with a negation.
>>
>> smap_get_bool is defined as below:
>> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232)
>>
>> /* Gets the value associated with 'key' in 'smap' and converts it to a
>> boolean.
>> * If 'key' is not in 'smap', or its value is neither "true" nor "false",
>> * returns 'def'. */
>> bool
>> smap_get_bool(const struct smap *smap, const char *key, bool def)
>> {
>> const char *value = smap_get_def(smap, key, "");
>> if (def) {
>> return strcasecmp("false", value) != 0;
>> } else {
>> return !strcasecmp("true", value);
>> }
>> }
>>
>> This returns expected values for the default case (since the above code will
>> negate “false” we get from smap_get bool function and return the value
>> “true”) but unexpected values for the case where the sampling value is
>> passed through the CLI.
>> For example, if we pass "true" for other_config:enable-input-sampling in the
>> CLI, the above code will negate the “true” value we get from the smap_bool
>> function and return the value “false”. Same would be the case for
>> enable_output_sampling.
>>
>> Signed-off-by: Sayali Naval \(sanaval\)
>
> LGTM.
>
> Acked-by: Adrian Moreno
Thanks, Sayali and Adrian!
I re-formatted the commit message and the subject line to make them
more readable in the git log and applied the change.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 8/8] treewide: Add `ovs_assert` to check for null pointers
On 7/11/23 13:40, Eelco Chaudron wrote:
>> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
>> index 5361b3c76..a3ca48a7b 100644
>> --- a/ovsdb/jsonrpc-server.c
>> +++ b/ovsdb/jsonrpc-server.c
>> @@ -1131,6 +1131,8 @@ static void
>> ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *s, struct ovsdb
>> *db,
>> struct jsonrpc_msg *request)
>> {
>
> For the below ones, please see earlier comments on ovsdb related code, Ilya?
Some of these are fine, though I'm not a fan of constant checking of all the
input arguments. Some are strange, see inline.
>
>> +ovs_assert(db);
>> +
>> /* Check for duplicate ID. */
>> size_t hash = json_hash(request->id, 0);
>> struct ovsdb_jsonrpc_trigger *t
>> @@ -1391,6 +1393,8 @@ ovsdb_jsonrpc_monitor_create(struct
>> ovsdb_jsonrpc_session *s, struct ovsdb *db,
>> enum ovsdb_monitor_version version,
>> const struct json *request_id)
>> {
>> +ovs_assert(db);
>> +
>> struct ovsdb_jsonrpc_monitor *m = NULL;
>> struct ovsdb_monitor *dbmon = NULL;
>> struct json *monitor_id, *monitor_requests;
>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>> index b560b0745..2180f9e2d 100644
>> --- a/ovsdb/monitor.c
>> +++ b/ovsdb/monitor.c
>> @@ -1327,6 +1327,7 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor
>> *dbmon,
>> struct ovsdb_monitor_table * mt;
>>
>> mt = shash_find_data(>tables, table->schema->name);
>> +ovs_assert(mt);
>> mt->select |= select;
>> }
>>
>> @@ -1710,6 +1711,8 @@ ovsdb_monitor_hash(const struct ovsdb_monitor *dbmon,
>> size_t basis)
>> for (i = 0; i < n; i++) {
>> struct ovsdb_monitor_table *mt = nodes[i]->data;
>>
>> +ovs_assert(mt);
>> +
>> basis = hash_pointer(mt->table, basis);
>> basis = hash_3words(mt->select, mt->n_columns, basis);
>>
>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>> index 9bad0c8dd..87da41f17 100644
>> --- a/ovsdb/ovsdb-server.c
>> +++ b/ovsdb/ovsdb-server.c
>> @@ -2229,6 +2229,8 @@ save_config(struct server_config *config)
>> static void
>> sset_from_json(struct sset *sset, const struct json *array)
>> {
>> +ovs_assert(array);
>> +
>> size_t i;
>>
>> sset_clear(sset);
>> diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
>> index f67b836d7..625b4ca68 100644
>> --- a/ovsdb/ovsdb.c
>> +++ b/ovsdb/ovsdb.c
>> @@ -40,6 +40,7 @@
>> #include "transaction-forward.h"
>> #include "trigger.h"
>> #include "unixctl.h"
>> +#include "util.h"
>>
>> #include "openvswitch/vlog.h"
>> VLOG_DEFINE_THIS_MODULE(ovsdb);
>> @@ -229,7 +230,7 @@ root_set_size(const struct ovsdb_schema *schema)
>> struct ovsdb_error *
>> ovsdb_schema_from_json(const struct json *json, struct ovsdb_schema
>> **schemap)
>> {
>> -struct ovsdb_schema *schema;
>> +struct ovsdb_schema *schema = NULL;
>> const struct json *name, *tables, *version_json, *cksum;
>> struct ovsdb_error *error;
>> struct shash_node *node;
>> @@ -249,6 +250,9 @@ ovsdb_schema_from_json(const struct json *json, struct
>> ovsdb_schema **schemap)
>> return error;
>> }
>>
>> +ovs_assert(name);
>> +ovs_assert(tables);
These are not optional for a json parser and ovsdb_parser_finish()
checks and fails if they do not exist. I don't think we should
check them here. If we can't trust the parser code, we'll need to
add this kind of checks in many other places as well to be consistent.
I'd like not to do that.
>> +
>> if (version_json) {
>> version = json_string(version_json);
>> if (!ovsdb_is_valid_version(version)) {
>> @@ -282,6 +286,8 @@ ovsdb_schema_from_json(const struct json *json, struct
>> ovsdb_schema **schemap)
>> shash_add(>tables, table->name, table);
>> }
>>
>> +ovs_assert(schema);
If the schema didn't exist, we would crash in the loop above.
Also the ovsdb_schema_create() can't fail, so we should not
need to check it.
>> +
>> /* "isRoot" was not part of the original schema definition. Before it
>> was
>> * added, there was no support for garbage collection. So, for backward
>> * compatibility, if the root set is empty then assume that every table
>> is
>> diff --git a/ovsdb/query.c b/ovsdb/query.c
>> index eebe56412..29cc93093 100644
>> --- a/ovsdb/query.c
>> +++ b/ovsdb/query.c
>> @@ -21,6 +21,7 @@
>> #include "condition.h"
>> #include "row.h"
>> #include "table.h"
>> +#include "util.h"
>>
>> void
>> ovsdb_query(struct ovsdb_table *table, const struct ovsdb_condition *cnd,
>> @@ -91,6 +92,7 @@ ovsdb_query_distinct(struct ovsdb_table *table,
>> struct ovsdb_row_hash hash;
>>
>> ovsdb_row_hash_init(, columns);
>> +ovs_assert(condition);
Here it's also not clear why we're checking in this branch and not
in the other. If we're checking condition, why not checking the
table, for example.
>> ovsdb_query(table,
[ovs-dev] [PATCH v4] python: Add async DNS support
This adds a Python version of the async DNS support added in:
771680d96 DNS: Add basic support for asynchronous DNS resolving
The above version uses the unbound C library, and this
implimentation uses the SWIG-wrapped Python version of that.
In the event that the Python unbound library is not available,
a warning will be logged and the resolve() method will just
return None. For the case where inet_parse_active() is passed
an IP address, it will not try to resolve it, so existing
behavior should be preserved in the case that the unbound
library is unavailable.
Intentional differences from the C version are as follows:
OVS_HOSTS_FILE environment variable can bet set to override
the system 'hosts' file. This is primarily to allow testing to
be done without requiring network connectivity.
Since resolution can still be done via hosts file lookup, DNS
lookups are not disabled when resolv.conf cannot be loaded.
The Python socket_util module has fallen behind its C equivalent.
The bare minimum change was done to inet_parse_active() to support
sync/async dns, as there is no equivalent to
parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
was added to bring socket_util.py up to equivalency to the C
version.
Signed-off-by: Terry Wilson
---
.github/workflows/build-and-test.yml| 4 +-
Documentation/intro/install/general.rst | 4 +-
Documentation/intro/install/rhel.rst| 2 +-
Documentation/intro/install/windows.rst | 2 +-
NEWS| 3 +
debian/control.in | 1 +
m4/openvswitch.m4 | 8 +-
python/TODO.rst | 7 +
python/automake.mk | 2 +
python/ovs/dns_resolve.py | 286
python/ovs/socket_util.py | 21 +-
python/ovs/stream.py| 2 +-
python/ovs/tests/test_dns_resolve.py| 280 +++
python/setup.py | 6 +-
rhel/openvswitch-fedora.spec.in | 2 +-
tests/vlog.at | 2 +
16 files changed, 615 insertions(+), 17 deletions(-)
create mode 100644 python/ovs/dns_resolve.py
create mode 100644 python/ovs/tests/test_dns_resolve.py
diff --git a/.github/workflows/build-and-test.yml
b/.github/workflows/build-and-test.yml
index f66ab43b0..47d239f10 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -183,10 +183,10 @@ jobs:
run: sudo apt update || true
- name: install common dependencies
run: sudo apt install -y ${{ env.dependencies }}
-- name: install libunbound libunwind
+- name: install libunbound libunwind python3-unbound
# GitHub Actions doesn't have 32-bit versions of these libraries.
if: matrix.m32 == ''
- run: sudo apt install -y libunbound-dev libunwind-dev
+ run: sudo apt install -y libunbound-dev libunwind-dev python3-unbound
- name: install 32-bit libraries
if: matrix.m32 != ''
run: sudo apt install -y gcc-multilib
diff --git a/Documentation/intro/install/general.rst
b/Documentation/intro/install/general.rst
index 42b5682fd..19e360d47 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -90,7 +90,7 @@ need the following software:
If libcap-ng is installed, then Open vSwitch will automatically build with
support for it.
-- Python 3.4 or later.
+- Python 3.6 or later.
- Unbound library, from http://www.unbound.net, is optional but recommended if
you want to enable ovs-vswitchd and other utilities to use DNS names when
@@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the
following software:
from iproute2 (part of all major distributions and available at
https://wiki.linuxfoundation.org/networking/iproute2).
-- Python 3.4 or later.
+- Python 3.6 or later.
On Linux you should ensure that ``/dev/urandom`` exists. To support TAP
devices, you must also ensure that ``/dev/net/tun`` exists.
diff --git a/Documentation/intro/install/rhel.rst
b/Documentation/intro/install/rhel.rst
index d1fc42021..f2151d890 100644
--- a/Documentation/intro/install/rhel.rst
+++ b/Documentation/intro/install/rhel.rst
@@ -92,7 +92,7 @@ Once that is completed, remove the file ``/tmp/ovs.spec``.
If python3-sphinx package is not available in your version of RHEL, you can
install it via pip with 'pip install sphinx'.
-Open vSwitch requires python 3.4 or newer which is not available in older
+Open vSwitch requires python 3.6 or newer which is not available in older
distributions. In the case of RHEL 6.x and its derivatives, one option is
to install python34 from `EPEL`_.
diff --git a/Documentation/intro/install/windows.rst
b/Documentation/intro/install/windows.rst
index 78f60f35a..fce099d5d 100644
--- a/Documentation/intro/install/windows.rst
+++ b/Documentation/intro/install/windows.rst
@@
Re: [ovs-dev] [PATCH v12 3/8] shash, simap, smap: Add assertions to `*_count` functions
On 7/11/23 12:05, Eelco Chaudron wrote:
>
>
> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
>
>> This commit adds assertions in the functions `shash_count`,
>> `simap_count`, and `smap_count` to ensure that the corresponding input
>> struct pointer is not NULL.
>>
>> This ensures that if the return values of `shash_sort`, `simap_sort`,
>> or `smap_sort` are NULL, then the following for loops would not attempt
>> to access the pointer, which might result in segmentation faults or
>> undefined behavior.
>>
>> Signed-off-by: James Raphael Tiovalen
>> Reviewed-by: Simon Horman
>> ---
>> lib/shash.c | 2 ++
>> lib/simap.c | 2 ++
>> lib/smap.c | 1 +
>> 3 files changed, 5 insertions(+)
>>
>> diff --git a/lib/shash.c b/lib/shash.c
>> index a7b2c6458..2bfc8eb50 100644
>> --- a/lib/shash.c
>> +++ b/lib/shash.c
>> @@ -17,6 +17,7 @@
>> #include
>> #include "openvswitch/shash.h"
>> #include "hash.h"
>> +#include "util.h"
>>
>> static struct shash_node *shash_find__(const struct shash *,
>> const char *name, size_t name_len,
>> @@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash)
>> size_t
>> shash_count(const struct shash *shash)
>> {
>> +ovs_assert(shash);
>
> My preference would be to return 0, in these instances. What do others think?
Calling these function with a NULL argument doesn't make much sense to me.
free()-like functions should generally accept NULL pointers, but functions
that actually do work on a datastructure shouldn't, IMO.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovn] ovn-controller high memory consumption with sbrec_server_has_##table##_table - enabled codepath
On 7/11/23 19:01, Dumitru Ceara wrote:
> On 7/11/23 18:33, Vladislav Odintsov wrote:
>> Hi Dumitru,
>>
>> The system on which I reproduced this issue is running 22.09.x version. I’ve
>> tried to upgrade ovn-controller to main branch + your patch. Please, note
>> that it has test error: [1].
>> After two minutes after upgrade it still consumed 3.3G.
>>
>
> Ack, I need to re-think the patch then. Maybe a hard deadline to run
> malloc_trim() at least once every X seconds. I'll see what I can come
> up with.
>
>> I tried to backport your patch to 22.09, it required to backport also this
>> commit: [2] and it failed some tests: [3].
>>
>> But I’ve got general question: prior to commit that I mentioned in initial
>> mail, ovn-controller even didn’t try load such amount of data. And now it
>> does and IIUC, your patch just releases memory that was freed after
>> ovn-controller fully loaded.
>> I’m wonder wether it should load that excess data at all? Seems like it did.
>>
>
> Looking again at 1b0dbde94070 ("ovn-controller: Only set monitor
> conditions on available tables.") it's kind of expected indeed:
>
> Initially all tables are "unavailable" because we didn't get the schema
> so we don't set any condition for any table.
>
> After ovn-controller connects to the SB for the first time it will
> determine that the SB tables are in the schema so it will explicitly add
> them to the monitor condition and restrict the SB data it is interested in.
>
> Maybe we need to change the IDL/CS modules to wait with the
> monitor_cond/monitor_cond_since until instructed by the client
> (ovn-controller). Ilya do you have any thoughts on this matter?
So, AFAICT, the issue is that we're running with 'monitor_everything_by_default'
option, the default condition is 'true' and the monitor request for the main
database is sent out immediately after receiving the schema, so the application
has no time to react.
I think, there are few possible solutions for this:
1. Introduce a new state in the CS state machine, e.g.
CS_S_SERVER_SCHEMA_RCEIVED, and move out from this state in the run()
callback. This way the application will have a chance to set up conditions
before they are sent. Slightly not intuitive.
2. A variation on what you suggested, i.e. enter the CS_S_SERVER_SCHEMA_RCEIVED
state and wait for some sort of the signal from the application to proceed.
Sounds a bit counter-intuitive for an IDL user.
3. Introduce an application callback that can be called from the
ovsdb_idl_compose_monitor_request() the same way as this function is getting
called form the ovsdb_cs_send_monitor_request(). An application will be
able to influence conditions before they are sent.
Might be tricky due to new->req->ack state transition.
4. Make the default condition configurable, e.g. by an additional argument
'default_condition' = true/false for an ovsdb_idl_create(). This way the
application will not get any data until conditions are actually set.
5. Or it maybe just a separate config function that will set default conditions
to 'false' and will need to be called before the first run().
6. Change behavior of 'monitor_everything_by_default' argument. Make it
actually add all the tables to the monitor, but with the 'false' condition.
Result should technically be the same. Might be tricky to get right though
with all the backward compatibility.
Option 5 might be the better option of these.
What do you think?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
Eric Garver writes: > On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote: >> On 7/8/23 00:06, Jakub Kicinski wrote: >> > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote: >> That already exists, right? Johannes added it in the last release for >> WiFi. >> >>> >> >>> I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves >> >>> similarly >> >>> to that on a surface. However, looking closer, any value that can be >> >>> passed >> >>> into ieee80211_rx_handlers_result() and ends up in the >> >>> kfree_skb_reason() is >> >>> kind of defined in net/mac80211/drop.h, unless I'm missing something >> >>> (very >> >>> possible, because I don't really know wifi code). >> >>> >> >>> The difference, I guess, is that for openvswitch values will be provided >> >>> by >> >>> the userpsace application via netlink interface. It'll be just a number >> >>> not >> >>> defined anywhere in the kernel. Only the subsystem itself will be >> >>> defined >> >>> in order to occupy the range. Garbage in, same garbage out, from the >> >>> kernel's >> >>> perspective. >> >> >> >> To be clear, I think, not defining them in this particular case is better. >> >> Definition of every reason that userspace can come up with will add extra >> >> uAPI maintenance cost/issues with no practical benefits. Values are not >> >> going to be used for anything outside reporting a drop reason and >> >> subsystem >> >> offset is not part of uAPI anyway. >> > >> > Ah, I see. No, please don't stuff user space defined values into >> > the drop reason. The reasons are for debugging the kernel stack >> > itself. IOW it'd be abuse not reuse. >> >> Makes sense. I wasn't sure that's a good solution from a kernel perspective >> either. It's better than defining all these reasons, IMO, but it's not good >> enough to be considered acceptable, I agree. >> >> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and >> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? >> One for an explicit drop action with a zero argument and one for an explicit >> drop with non-zero argument. >> >> The exact reason for the error can be retrieved by other means, i.e by >> looking >> at the datapath flow dump or OVS logs/traces. >> >> This way we can give a user who is catching packet drop traces a signal that >> there was something wrong with an OVS flow and they can look up exact details >> from the userspace / flow dump. >> >> The point being, most of the flows will have a zero as a drop action >> argument, >> i.e. a regular explicit packet drop. It will be hard to figure out which >> flow >> exactly we're hitting without looking at the full flow dump. And if the >> value >> is non-zero, then it should be immediately obvious which flow is to blame >> from >> the dump, as we should not have a lot of such flows. >> >> This would still allow us to avoid a maintenance burden of defining every >> case, >> which are fairly meaningless for the kernel itself, while having 99% of the >> information we may need. >> >> Jakub, do you think this will be acceptable? >> >> Eric, Adrian, Aaron, do you see any problems with such implementation? > > I see no problems. I'm content with this approach. +1 >> P.S. There is a plan to add more drop reasons for other places in openvswitch >> module to catch more regular types of drops like memory issues or upcall >> failures. So, the drop reason subsystem can be extended later. >> The explicit drop action is a bit of an odd case here. >> >> Best regards, Ilya Maximets. >> ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 3/3] checkpatch: print subject field if misspelled or missing
Eelco Chaudron writes:
> On 7 Jul 2023, at 22:07, Chandan Somani wrote:
>
>> This narrows down spelling errors that are in the commit
>> subject. In v2, it also provides a subject if the subject
>> line is missing. The provisional subject is the name of the
>> patch file, which should provide some context about the patch.
>>
>> Signed-off-by: Chandan Somani
>
> Hi Chandan,
>
> Thanks for the patch! The robot found a problem, with a trailing white space.
>
> However I can fix this at commit time, also one small change
> below. Let me know if you agree and will fix this up during commit.
>
> In addition, would it be possible to send an additional patch(series)
> to also do this on git-based patches, i.e. if you do something like
> ‘checkapatch.py -S -2’?
> Maybe there should be an error if the subject is missing, independent if
> spellcheck is enabled or not?
>
>
> Cheers,
>
> Eelco
>
>
>> ---
>> utilities/checkpatch.py | 8
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index e5d5029f2..f34e4bf2a 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -1024,6 +1024,14 @@ def ovs_checkpatch_file(filename):
>> result = ovs_checkpatch_parse(part.get_payload(decode=False), filename,
>>mail.get('Author', mail['From']),
>>mail['Commit'])
>> +if spellcheck:
>> +if not mail['Subject'].strip():
>> +mail.replace_header('Subject', sys.argv[-1])
>> +print("Subject missing! Your provisional subject is",
>> + mail['Subject'])
>
> If the subject is missing from the patch it would error out.
> What about the following:
>
> +if spellcheck:
> +if not mail['Subject'] or not mail['Subject'].strip():
> +if mail['Subject']:
> +mail.replace_header('Subject', sys.argv[-1])
> +else:
> +mail.add_header('Subject', sys.argv[-1])
> +
> +print("Subject missing! Your provisional subject is",
> + mail['Subject'])
> +
> +if check_spelling(mail['Subject'], False):
> +print("Subject: %s" % mail['Subject'])
> +
With the block above, and whitespace changes:
Acked-by: Aaron Conole
>> +if check_spelling(mail['Subject'], False):
>> +print("Subject: %s" % mail['Subject'])
>> +
>> ovs_checkpatch_print_result()
>> return result
>>
>> --
>> 2.26.3
>>
>> ___
>> 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/3] checkpatch: reorganize flagged words using a list
Chandan Somani writes:
> Single out flagged words and allow for more useful
> details, like spelling suggestions. Fixed syntax
> error from v1
>
> Signed-off-by: Chandan Somani
> ---
Thanks!
Acked-by: Aaron Conole
> utilities/checkpatch.py | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 64f0efeb4..acf9a0102 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -411,6 +411,8 @@ def check_spelling(line, comment):
> words = filter_comments(line, True) if comment else line
> words = words.replace(':', ' ').split(' ')
>
> +flagged_words = []
> +
> for word in words:
> skip = False
> strword = re.subn(r'\W+', '', word)[0].replace(',', '')
> @@ -435,9 +437,13 @@ def check_spelling(line, comment):
> skip = True
>
> if not skip:
> -print_warning("Check for spelling mistakes (e.g. \"%s\")"
> - % strword)
> -return True
> +flagged_words.append(strword)
> +
> +if len(flagged_words) > 0:
> +for mistake in flagged_words:
> +print_warning("Possible misspelled word: \"%s\"" % mistake)
> +
> +return True
>
> return False
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 3/3] checkpatch: print subject field if misspelled or missing
On Tue, Jul 11, 2023 at 12:38 AM Eelco Chaudron wrote:
>
>
> On 7 Jul 2023, at 22:07, Chandan Somani wrote:
>
> > This narrows down spelling errors that are in the commit
> > subject. In v2, it also provides a subject if the subject
> > line is missing. The provisional subject is the name of the
> > patch file, which should provide some context about the patch.
> >
> > Signed-off-by: Chandan Somani
>
> Hi Chandan,
>
> Thanks for the patch! The robot found a problem, with a trailing white
> space.
>
> However I can fix this at commit time, also one small change below. Let me
> know if you agree and will fix this up during commit.
>
>Agreed, thanks.
> In addition, would it be possible to send an additional patch(series) to
> also do this on git-based patches, i.e. if you do something like
> ‘checkapatch.py -S -2’?
> Maybe there should be an error if the subject is missing, independent if
> spellcheck is enabled or not?
>
> Yes, I'll work on this.
>
> Cheers,
>
> Eelco
>
>
> > ---
> > utilities/checkpatch.py | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> > index e5d5029f2..f34e4bf2a 100755
> > --- a/utilities/checkpatch.py
> > +++ b/utilities/checkpatch.py
> > @@ -1024,6 +1024,14 @@ def ovs_checkpatch_file(filename):
> > result = ovs_checkpatch_parse(part.get_payload(decode=False),
> filename,
> >mail.get('Author', mail['From']),
> >mail['Commit'])
> > +if spellcheck:
> > +if not mail['Subject'].strip():
> > +mail.replace_header('Subject', sys.argv[-1])
> > +print("Subject missing! Your provisional subject is",
> > + mail['Subject'])
>
> If the subject is missing from the patch it would error out.
> What about the following:
>
> +if spellcheck:
> +if not mail['Subject'] or not mail['Subject'].strip():
> +if mail['Subject']:
> +mail.replace_header('Subject', sys.argv[-1])
> +else:
> +mail.add_header('Subject', sys.argv[-1])
> +
> +print("Subject missing! Your provisional subject is",
> + mail['Subject'])
> +
> +if check_spelling(mail['Subject'], False):
> +print("Subject: %s" % mail['Subject'])
> +
>
>
> > +if check_spelling(mail['Subject'], False):
> > +print("Subject: %s" % mail['Subject'])
> > +
> > ovs_checkpatch_print_result()
> > return result
> >
> > --
> > 2.26.3
> >
> > ___
> > 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] [ovn] ovn-controller high memory consumption with sbrec_server_has_##table##_table - enabled codepath
On 7/11/23 18:33, Vladislav Odintsov wrote:
> Hi Dumitru,
>
> The system on which I reproduced this issue is running 22.09.x version. I’ve
> tried to upgrade ovn-controller to main branch + your patch. Please, note
> that it has test error: [1].
> After two minutes after upgrade it still consumed 3.3G.
>
Ack, I need to re-think the patch then. Maybe a hard deadline to run
malloc_trim() at least once every X seconds. I'll see what I can come
up with.
> I tried to backport your patch to 22.09, it required to backport also this
> commit: [2] and it failed some tests: [3].
>
> But I’ve got general question: prior to commit that I mentioned in initial
> mail, ovn-controller even didn’t try load such amount of data. And now it
> does and IIUC, your patch just releases memory that was freed after
> ovn-controller fully loaded.
> I’m wonder wether it should load that excess data at all? Seems like it did.
>
Looking again at 1b0dbde94070 ("ovn-controller: Only set monitor
conditions on available tables.") it's kind of expected indeed:
Initially all tables are "unavailable" because we didn't get the schema
so we don't set any condition for any table.
After ovn-controller connects to the SB for the first time it will
determine that the SB tables are in the schema so it will explicitly add
them to the monitor condition and restrict the SB data it is interested in.
Maybe we need to change the IDL/CS modules to wait with the
monitor_cond/monitor_cond_since until instructed by the client
(ovn-controller). Ilya do you have any thoughts on this matter?
Thanks,
Dumitru
> 1: https://github.com/odivlad/ovn/actions/runs/5519795488/jobs/10065618661
> 2:
> https://github.com/ovn-org/ovn/commit/b4c593d23bd959e98fcc9ada4a973ac933579ded
> 3: https://github.com/odivlad/ovn/actions/runs/5519892057
>
>> On 11 Jul 2023, at 14:39, Dumitru Ceara wrote:
>>
>> On 7/10/23 22:20, Vladislav Odintsov wrote:
>>> Hi Dumitru,
>>>
>>> thanks for digging into this! I highly appreciate your help!
>>>
>>
>> No worries, my pleasure! :)
>>
>>> Please, see my answers inline.
>>>
On 10 Jul 2023, at 15:28, Dumitru Ceara wrote:
On 7/10/23 12:57, Dumitru Ceara wrote:
> On 7/6/23 13:00, Vladislav Odintsov wrote:
>>
>>
>>> On 5 Jul 2023, at 20:07, Vladislav Odintsov wrote:
>>>
>>> Hi Dumitru,
>>>
>>> thanks for the quick response!
>>>
On 5 Jul 2023, at 19:53, Dumitru Ceara wrote:
On 7/5/23 17:14, Vladislav Odintsov wrote:
> Hi,
>
Hi Vladislav,
> we’ve noticed there is a huge ovn-controller memory consumption
> introduced with [0] comparing to version without its changes in
> ovn-controller.c part (just OVS submodule bump without ovn-controller
> changes doesn’t trigger such behaviour).
>
Thanks for reporting this!
> On an empty host connected to working cluster ovn-controller normally
> consumes ~175 MiB RSS, and the same host updated to a version with
> commit [0] consumes 3.3 GiB RSS. The start time and CPU consumption
> during process start of an affected version is higher that for the
> normal version.
>
> Before upgrade (normal functioning):
>
> #ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof
> ovn-controller)| grep ovn
> idl-cells-OVN_Southbound:343855 idl-cells-Open_vSwitch:14131
> ofctrl_desired_flow_usage-KB:76 ofctrl_installed_flow_usage-KB:60
> ofctrl_sb_flow_ref_usage-KB:18
> 174.2 MiB + 327.5 KiB = 174.5 MiB ovn-controller
>
> After upgrade to an affected version I’ve checked memory report while
> ovn-controller was starting and at some time there was a bigger
> amount of OVN_Southbound cells comparing to "after start" time:
>
> during start:
>
> # ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof
> ovn-controller)| grep ovn
> idl-cells-OVN_Southbound:11388742 idl-cells-Open_vSwitch:14131
> idl-outstanding-txns-Open_vSwitch:1
> 3.3 GiB + 327.0 KiB = 3.3 GiB ovn-controller
>
> after start:
>
> # ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof
> ovn-controller)| grep ovn
> idl-cells-OVN_Southbound:343896 idl-cells-Open_vSwitch:14131
> idl-outstanding-txns-Open_vSwitch:1
> 3.3 GiB + 327.0 KiB = 3.3 GiB ovn-controller
>
>
> cells during start:
> 11388742
>
> cells after start:
> 343896
>
Are you running with ovn-monitor-all=true on this host?
>>>
>>> No, it has default false.
>>>
I guess it's unlikely but I'll try just in case: would it be possible
to
share the SB database?
Re: [ovs-dev] [ovn] ovn-controller high memory consumption with sbrec_server_has_##table##_table - enabled codepath
Hi Dumitru, The system on which I reproduced this issue is running 22.09.x version. I’ve tried to upgrade ovn-controller to main branch + your patch. Please, note that it has test error: [1]. After two minutes after upgrade it still consumed 3.3G. I tried to backport your patch to 22.09, it required to backport also this commit: [2] and it failed some tests: [3]. But I’ve got general question: prior to commit that I mentioned in initial mail, ovn-controller even didn’t try load such amount of data. And now it does and IIUC, your patch just releases memory that was freed after ovn-controller fully loaded. I’m wonder wether it should load that excess data at all? Seems like it did. 1: https://github.com/odivlad/ovn/actions/runs/5519795488/jobs/10065618661 2: https://github.com/ovn-org/ovn/commit/b4c593d23bd959e98fcc9ada4a973ac933579ded 3: https://github.com/odivlad/ovn/actions/runs/5519892057 > On 11 Jul 2023, at 14:39, Dumitru Ceara wrote: > > On 7/10/23 22:20, Vladislav Odintsov wrote: >> Hi Dumitru, >> >> thanks for digging into this! I highly appreciate your help! >> > > No worries, my pleasure! :) > >> Please, see my answers inline. >> >>> On 10 Jul 2023, at 15:28, Dumitru Ceara wrote: >>> >>> On 7/10/23 12:57, Dumitru Ceara wrote: On 7/6/23 13:00, Vladislav Odintsov wrote: > > >> On 5 Jul 2023, at 20:07, Vladislav Odintsov wrote: >> >> Hi Dumitru, >> >> thanks for the quick response! >> >>> On 5 Jul 2023, at 19:53, Dumitru Ceara wrote: >>> >>> On 7/5/23 17:14, Vladislav Odintsov wrote: Hi, >>> >>> Hi Vladislav, >>> we’ve noticed there is a huge ovn-controller memory consumption introduced with [0] comparing to version without its changes in ovn-controller.c part (just OVS submodule bump without ovn-controller changes doesn’t trigger such behaviour). >>> >>> Thanks for reporting this! >>> On an empty host connected to working cluster ovn-controller normally consumes ~175 MiB RSS, and the same host updated to a version with commit [0] consumes 3.3 GiB RSS. The start time and CPU consumption during process start of an affected version is higher that for the normal version. Before upgrade (normal functioning): #ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof ovn-controller)| grep ovn idl-cells-OVN_Southbound:343855 idl-cells-Open_vSwitch:14131 ofctrl_desired_flow_usage-KB:76 ofctrl_installed_flow_usage-KB:60 ofctrl_sb_flow_ref_usage-KB:18 174.2 MiB + 327.5 KiB = 174.5 MiB ovn-controller After upgrade to an affected version I’ve checked memory report while ovn-controller was starting and at some time there was a bigger amount of OVN_Southbound cells comparing to "after start" time: during start: # ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof ovn-controller)| grep ovn idl-cells-OVN_Southbound:11388742 idl-cells-Open_vSwitch:14131 idl-outstanding-txns-Open_vSwitch:1 3.3 GiB + 327.0 KiB = 3.3 GiBovn-controller after start: # ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof ovn-controller)| grep ovn idl-cells-OVN_Southbound:343896 idl-cells-Open_vSwitch:14131 idl-outstanding-txns-Open_vSwitch:1 3.3 GiB + 327.0 KiB = 3.3 GiBovn-controller cells during start: 11388742 cells after start: 343896 >>> >>> Are you running with ovn-monitor-all=true on this host? >> >> No, it has default false. >> >>> >>> I guess it's unlikely but I'll try just in case: would it be possible to >>> share the SB database? >> >> Unfortunately, no. But I can say it’s about 450 M in size after >> compaction. And there are about 1M mac_bindings if it’s important :). > I tried in a sandbox, before and after the commit in question, with a script that adds 1M mac bindings on top of the sample topology built by tutorial/ovn-setup.sh. I see ovn-controller memory usage going to >3GB in before the commit you blamed and to >1.9GB after the same commit. So it looks different than what you reported but still worrying that we use so much memory for mac bindings. I'm assuming however that quite a few of the 1M mac bindings in your setup are stale so would it be possible to enable mac_binding aging? It needs to be enabled per router with something like: $ ovn-nbctl set logical_router RTR options:mac_binding_age_threshold=60 The threshold is in seconds and is a hard limit (for now)
Re: [ovs-dev] [OVSCONF] OVS+OVN '23: Call for Participation
Just a small update,
We have moved the conference dates to December 6-7, 2023 so as to not overlap
with another networking conference happening around the same time.
Stay tuned for further updates!
Thank you!
Conference Team
On Fri, Jul 7, 2023 at 1:23 PM Michael Santana wrote:
>
> Hello everyone!
>
> We are happy to announce this year's Open vSwitch and OVN conference!
> The conference will be on November 1-2, 2023 and will be a fully virtual
> event.
>
> We are seeking long and short ("lightning") talks on topics related to
> Open vSwitch and OVN. We expect long talks to last 25 minutes with an
> additional 5 minutes for questions, and short talks to last 10 minutes.
>
> We will use an online format. Talks will be pre-recorded and played
> during the conference. During the playback, text chat will give the
> attendees and the presenters an opportunity to interact. During the
> question period, the presenters will answer questions live over audio
> (and video, at the presenters' discretion). Video will also be
> available for offline viewing.
>
>
> How to propose a talk
> -
>
> We are soliciting proposals for full talks and short ("lightning")
> talks in a single round. You may propose a talk as a full talk, a
> lightning talk, or for either one at the committee's discretion.
>
> We will also accept proposals for panel discussions. Please submit
> them as full talks and make it clear in the description that it is a
> panel.
>
> Please submit proposals to ovs...@openvswitch.org by September 15.
> Proposals should include:
>
> * Title and abstract.
>
> * Speaker names, email addresses, and affiliations.
>
> * Whether you are proposing a full talk or a lightning talk or
> either one at the committee's discretion.
>
> Speakers will be notified of acceptance by September 29.
>
>
> How to attend
> -
>
> General registration is not yet open. We will announce when it opens.
> The event will be fully virtual. Will be available after registering.
>
> At the moment we are unsure about whether attendance will
> be free of cost this year. We will have a follow up soon on this.
>
>
>
> Topics that may be considered, among others, include:
> -
>
> * The future of Open vSwitch (e.g., AF_XDP, P4, eBPF).
>
> * NAT, DPI, and stateful processing with Open vSwitch.
>
> * Deploying and using OVN.
>
> * Testing and scaling OVN.
>
> * NIC acceleration of Open vSwitch.
>
> * Using Open vSwitch to realize NFV and service chaining.
>
> * Porting Open vSwitch to new operating systems, hypervisors,
>or container systems.
>
> * Integrating Open vSwitch into larger systems.
>
> * Troubleshooting and debugging Open vSwitch installations.
>
> * Open vSwitch development and testing practices.
>
> * Performance measurements or approaches to improving
>performance.
>
> * End-user or service provider experiences with Open vSwitch.
>
> * Hardware ports of Open vSwitch (existing, in progress, or
>speculative).
>
> * The relationship between OpenFlow and Open vSwitch.
>
> * Using, developing, or administering OpenFlow controllers in
>conjunction with Open vSwitch.
>
> * Comparisons to other implementations of features found in
>Open vSwitch (e.g. other OpenFlow implementations, other
>software switches, etc.).
>
> * Increasing the size and diversity of the Open vSwitch user
>and developer base.
>
> * Tutorials and walkthroughs of use cases.
>
> * Demos.
>
>
> More information
>
> To reach the organizers, email ovs...@openvswitch.org. For general
> discussion of the conference, please use the ovs-discuss mailing list
> at ovs-disc...@openvswitch.org
>
>
> Thank you,
> Conference Team
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 5/8] file, monitor: Add null pointer assertions for old and new ovsdb_rows
On 11 Jul 2023, at 16:38, Ilya Maximets wrote:
> On 7/11/23 12:17, Eelco Chaudron wrote:
>>
>>
>> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
>>
>>> This commit adds non-null pointer assertions in some code that performs
>>> some decisions based on old and new input ovsdb_rows.
>>>
>>> Signed-off-by: James Raphael Tiovalen
>>> Reviewed-by: Simon Horman
>>
>> What about error messages/argument checking instead of asserts here?
>
> File transactions must have some data. Every row must have at least
> uuid and version. New rows should have 'new' versions of the data,
> deleted should have 'old', and modified should have both. It's a
> bug somewhere if we have a row without any data.
Thanks for confirming/clarification. With this;
Acked-by: Eelco Chaudron
>>
>> //Eelco
>>
>>> ---
>>> ovsdb/file.c| 2 ++
>>> ovsdb/monitor.c | 4 +++-
>>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/ovsdb/file.c b/ovsdb/file.c
>>> index 2d887e53e..b1d386e76 100644
>>> --- a/ovsdb/file.c
>>> +++ b/ovsdb/file.c
>>> @@ -522,7 +522,9 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
>>> }
>>>
>>> if (row) {
>>> +ovs_assert(new || old);
>>> struct ovsdb_table *table = new ? new->table : old->table;
>>> +ovs_assert(table);
>>> char uuid[UUID_LEN + 1];
>>>
>>> if (table != ftxn->table) {
>>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>>> index 4afaa89f4..c32af7b02 100644
>>> --- a/ovsdb/monitor.c
>>> +++ b/ovsdb/monitor.c
>>> @@ -1372,8 +1372,10 @@ ovsdb_monitor_changes_update(const struct ovsdb_row
>>> *old,
>>> const struct ovsdb_monitor_table *mt,
>>> struct ovsdb_monitor_change_set_for_table
>>> *mcst)
>>> {
>>> +ovs_assert(new || old);
>>> const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
>>> -struct ovsdb_monitor_row *change;
>>> +ovs_assert(uuid);
>>> +struct ovsdb_monitor_row *change = NULL;
>>>
>>> change = ovsdb_monitor_changes_row_find(mcst, uuid);
>>> if (!change) {
>>> --
>>> 2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 4/8] ovsdb: Assert and check return values of `ovsdb_table_schema_get_column`
On 11 Jul 2023, at 16:33, Ilya Maximets wrote: > On 7/11/23 12:13, Eelco Chaudron wrote: >> >> >> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: >> >>> This commit adds a few null pointer assertions and checks to some return >>> values of `ovsdb_table_schema_get_column`. If a null pointer is >>> encountered in these blocks, either the assertion will fail or the >>> control flow will now be redirected to alternative paths which will >>> output the appropriate error messages. >>> >>> A few ovsdb-rbac and ovsdb-server tests are also updated to verify the >>> expected warning logs by adding said logs to the ALLOWLIST of the >>> OVSDB_SERVER_SHUTDOWN statements. >>> >>> Signed-off-by: James Raphael Tiovalen >> >> In general, this looks good, however, I’m not a ovsdb guy, so I’m wondering >> if the asserts could not cause any additional crashes that can be avoided by >> a different type of error handling. Also is there an easy way to make this >> crash happen by giving some invalid input? >> >> Ilya any comments here? > > '_uuid' and '_version' are internal columns that must always exist. > They are not part of the schema, ovsdb-server generates them. So, > it must be an internal bug if they do not exist. Thanks for confirming/clarification. With this; Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 0/8] northd: I-P for load balancer and lb groups
On Sat, Jul 8, 2023 at 1:27 AM Mark Michelson wrote: > > Hi Numan, > > I gave the series a look. I've looked at the code but haven't yet run > any tests with it. The main reason for this is that the series does not > apply cleanly to OVN main. > > Overall, I only have small notes. I've replied to patches 2 and 3 with > the specifics. Thanks for the review Mark. I'll address those comments and any other in v3. I'll wait for a bit before submitting v3 if you or others have more comments. I did rebase before submitting. Maybe some mistake from my side. But looks like ovsrobot was able to successfully apply the patches and run the tests. If you want to continue reviewing can you please pull it from here - https://github.com/numansiddique/ovn/tree/northd_ip_refactor_lb_ip_v2_p8 or - https://github.com/ovsrobot/ovn/tree/series_362800 Thanks Numan > > On 7/7/23 01:51, num...@ovn.org wrote: > > From: Numan Siddique > > > > This patch series adds the support to handle load balancer and > > load balancer group changes incrementally in the "northd" engine > > node. "flow" engine node doesn't support I-P yet and falls back > > to full recompute. > > > > Note: I still need to do some scale testing and measure the > > improvements. I'll first attempt adding I-P in the "flow" > > engine node before attempting some scale tests. Neverthless > > this patch series is good enough for reviews. > > > > > > v1 -> v2 > > > >* Resolved the conflicts and rebased > >* Fixed a bug in p6. > > > > Numan Siddique (8): > >northd I-P: Sync SB load balancers in a separate engine node. > >northd: Add a new engine node - northd_lb_data. > >northd: Add initial I-P for load balancer and load balancer groups > >northd: Refactor the 'northd' node code which handles logical switch > > changes. > >northd: Handle load balancer changes for a logical switch. > >northd: Handle load balancer group changes for a logical switch. > >northd: Sync SB Port bindings NAT column in a separate engine node. > >northd: Handle load balancer changes for a logical router. > > > > lib/lb.c | 356 ++- > > lib/lb.h | 113 ++- > > northd/automake.mk |2 + > > northd/en-lflow.c |8 +- > > northd/en-northd-lb-data.c | 308 ++ > > northd/en-northd-lb-data.h | 56 ++ > > northd/en-northd.c | 57 +- > > northd/en-northd.h |2 + > > northd/en-sync-sb.c| 60 ++ > > northd/en-sync-sb.h|9 + > > northd/inc-proc-northd.c | 27 +- > > northd/northd.c| 1858 +--- > > northd/northd.h| 44 +- > > tests/ovn-northd.at| 131 +++ > > 14 files changed, 2354 insertions(+), 677 deletions(-) > > create mode 100644 northd/en-northd-lb-data.c > > create mode 100644 northd/en-northd-lb-data.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 v12 5/8] file, monitor: Add null pointer assertions for old and new ovsdb_rows
On 7/11/23 12:17, Eelco Chaudron wrote:
>
>
> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
>
>> This commit adds non-null pointer assertions in some code that performs
>> some decisions based on old and new input ovsdb_rows.
>>
>> Signed-off-by: James Raphael Tiovalen
>> Reviewed-by: Simon Horman
>
> What about error messages/argument checking instead of asserts here?
File transactions must have some data. Every row must have at least
uuid and version. New rows should have 'new' versions of the data,
deleted should have 'old', and modified should have both. It's a
bug somewhere if we have a row without any data.
>
> //Eelco
>
>> ---
>> ovsdb/file.c| 2 ++
>> ovsdb/monitor.c | 4 +++-
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/ovsdb/file.c b/ovsdb/file.c
>> index 2d887e53e..b1d386e76 100644
>> --- a/ovsdb/file.c
>> +++ b/ovsdb/file.c
>> @@ -522,7 +522,9 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
>> }
>>
>> if (row) {
>> +ovs_assert(new || old);
>> struct ovsdb_table *table = new ? new->table : old->table;
>> +ovs_assert(table);
>> char uuid[UUID_LEN + 1];
>>
>> if (table != ftxn->table) {
>> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
>> index 4afaa89f4..c32af7b02 100644
>> --- a/ovsdb/monitor.c
>> +++ b/ovsdb/monitor.c
>> @@ -1372,8 +1372,10 @@ ovsdb_monitor_changes_update(const struct ovsdb_row
>> *old,
>> const struct ovsdb_monitor_table *mt,
>> struct ovsdb_monitor_change_set_for_table
>> *mcst)
>> {
>> +ovs_assert(new || old);
>> const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
>> -struct ovsdb_monitor_row *change;
>> +ovs_assert(uuid);
>> +struct ovsdb_monitor_row *change = NULL;
>>
>> change = ovsdb_monitor_changes_row_find(mcst, uuid);
>> if (!change) {
>> --
>> 2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 4/8] ovsdb: Assert and check return values of `ovsdb_table_schema_get_column`
On 7/11/23 12:13, Eelco Chaudron wrote: > > > On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: > >> This commit adds a few null pointer assertions and checks to some return >> values of `ovsdb_table_schema_get_column`. If a null pointer is >> encountered in these blocks, either the assertion will fail or the >> control flow will now be redirected to alternative paths which will >> output the appropriate error messages. >> >> A few ovsdb-rbac and ovsdb-server tests are also updated to verify the >> expected warning logs by adding said logs to the ALLOWLIST of the >> OVSDB_SERVER_SHUTDOWN statements. >> >> Signed-off-by: James Raphael Tiovalen > > In general, this looks good, however, I’m not a ovsdb guy, so I’m wondering > if the asserts could not cause any additional crashes that can be avoided by > a different type of error handling. Also is there an easy way to make this > crash happen by giving some invalid input? > > Ilya any comments here? '_uuid' and '_version' are internal columns that must always exist. They are not part of the schema, ovsdb-server generates them. So, it must be an internal bug if they do not exist. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] ovs-vsctl: Exit with error if postdb checks report errors.
On 7/5/23 01:29, Flavio Leitner wrote: > Today the exit code refers to the execution of the change > in the database. However, when not using parameter --no-wait > (default), the ovs-vsctl also checks if OVSDB transactions > are successfully recorded and reload by ovs-vswitchd. In this > case, an error message is printed if there is a problem during > the reload, like for example the one below: > > # ovs-vsctl add-port br0 gre0 -- \ > set interface gre0 type=ip6gre options:key=100 \ > options:remote_ip=2001::2 > ovs-vsctl: Error detected while setting up 'gre0': could not \ > add network device gre0 to ofproto (Address family not supported\ > by protocol). See ovs-vswitchd log for details. > ovs-vsctl: The default log directory is "/var/log/openvswitch". > # echo $? > 0 > > This patch changes to exit with specific error code 160 > (ERROR_BAD_ARGUMENTS) on Windows and 65 (EX_DATAERR) on > Linux or BSD if errors were reported during the reload. > > This change may break existing scripts because ovs-vsctl will > start to fail when before it was succeeding. However, if an > error is printed, then it is likely that the change was not > functional anyway. > > Reported-at: https://bugzilla.redhat.com/1731553 > Signed-off-by: Flavio Leitner > --- > > v3: Fixed the Windows build issue reported by Ilya. > Return ERROR_BAD_ARGUMENTS on Windows. > v2: > Followed Aaron's suggestion to return EX_DATAERR. > > NEWS | 5 + > tests/ovs-vsctl.at | 30 -- > tests/ovs-vswitchd.at| 6 +- > tests/tunnel.at | 8 +++- > utilities/ovs-vsctl.8.in | 4 > utilities/ovs-vsctl.c| 35 +-- > 6 files changed, 78 insertions(+), 10 deletions(-) > > diff --git a/NEWS b/NEWS > index 6a990c921..8c733e417 100644 > --- a/NEWS > +++ b/NEWS > @@ -30,6 +30,11 @@ Post-v3.1.0 > * Added new options --[ovsdb-server|ovs-vswitchd]-umask=MODE to set > umask > value when starting OVS daemons. E.g., use --ovsdb-server-umask=0002 > in order to create OVSDB sockets with access mode of 0770. > + - ovs-vsctl: > + * Exit with error code 160 (ERROR_BAD_ARGUMENTS) on Windows or > + 65 (EX_DATAERR) on other platforms if errors were reported while > + checking if OVSDB transactions are successfully recorded and reload > + by ovs-vswitchd. This description is confusing to me and will likely be confusing for OVS users. 'if errors were reported while checking if OVSDB transactions are successfully recorded' is not correct, because the database transaction already succeeded (i.e. recorded) at the moment the check is happening, so the data is already successfully written into the database. The 'reload by ovs-vswitchd' is not defined, i.e. it's not clear what it means. The man page is using the 'waiting for ovs-vswitchd to reconfigure itself' or 'waits until ovs-vswitchd has finished reconfiguring itself' wording. We should probably base the description on similar terms. > - QoS: > * Added new configuration option 'jitter' for a linux-netem QoS type. > - DPDK: > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > index a368bff6e..a8274734f 100644 > --- a/tests/ovs-vsctl.at > +++ b/tests/ovs-vsctl.at > @@ -1522,7 +1522,11 @@ cat >experr < ovs-vsctl: Error detected while setting up 'reserved_name'. See > ovs-vswitchd log for details. > ovs-vsctl: The default log directory is "$OVS_RUNDIR". > EOF > -AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr]) > +if test "$IS_WIN32" = "yes"; then > +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [160], [], [experr]) > +else > +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [65], [], [experr]) > +fi > # Prevent race. > OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1]) > # Detect the warning log message > @@ -1560,7 +1564,11 @@ cat >experr < ovs-vsctl: Error detected while setting up 'reserved_name'. See > ovs-vswitchd log for details. > ovs-vsctl: The default log directory is "$OVS_RUNDIR". > EOF > -AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr]) > +if test "$IS_WIN32" = "yes"; then > +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [160], [], [experr]) > +else > +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [65], [], [experr]) > +fi > # Prevent race. > OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1]) > # Detect the warning log message > @@ -1737,3 +1745,21 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns > _uuid,name list bridge tst1], [0] > > OVS_VSCTL_CLEANUP > AT_CLEANUP > + > +AT_SETUP([ovs-vsctl -- return error if OVSDB reload issues are reported]) > +OVS_VSWITCHD_START > + > +cat >experr << EOF > +ovs-vsctl: Error detected while setting up 'gre0': gre0: bad ip6gre > 'remote_ip' > +gre0: ip6gre type requires valid 'remote_ip' argument. See ovs-vswitchd log > for details. > +ovs-vsctl: The default
Re: [ovs-dev] [PATCH v5] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions
On 10 Jul 2023, at 17:20, Mike Pattrick wrote:
> Several xlate actions used in recursive translation currently store a
> large amount of information on the stack. This can result in handler
> threads quickly running out of stack space despite before
> xlate_resubmit_resource_check() is able to terminate translation. This
> patch reduces stack usage by over 3kb from several translation actions.
>
> This patch also moves some trace function from do_xlate_actions into its
> own function.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2104779
> Signed-off-by: Mike Pattrick
> Reviewed-by: Simon Horman
Thanks Mike, however, I guess this was supposed to be v6, as it got me confused
;)
Small nit below, the rest looks good.
Acked-by: Eelco Chaudron
> ---
> Since v1:
> - Refactored code into specialized functions and renamed variables for
> improved readability.
> Since v2:
> - Removed inline keywords
> Since v3:
> - Improved formatting.
> Since v4:
> - v4 patch was an incorrect revision
> Since v5:
> - Moved trace portmap to heap
> - Moved additional flow_tnl mf_subvalue structs to heap
>
> Signed-off-by: Mike Pattrick
> ---
> ofproto/ofproto-dpif-xlate.c | 259 ++-
> 1 file changed, 164 insertions(+), 95 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 29f4daa63..2ecab08fb 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -501,6 +501,84 @@ ctx_cancel_freeze(struct xlate_ctx *ctx)
>
> static void finish_freezing(struct xlate_ctx *ctx);
>
> +/* These functions and structure are used to save stack space in actions that
> + * need to retain a large amount of xlate_ctx state. */
> +struct xretained_state {
> +union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
> +uint64_t actset_stub[1024 / 8];
> +struct ofpbuf old_stack;
> +struct ofpbuf old_action_set;
> +struct flow old_flow;
> +struct flow old_base;
> +struct flow_tnl flow_tnl_mask;
> +};
> +
> +/* The return of this function must be freed by
> + * xretain_state_restore_and_free(). */
> +static struct xretained_state *
> +xretain_state_save(struct xlate_ctx *ctx)
> +{
> +struct xretained_state *retained = xmalloc(sizeof *retained);
> +
> +retained->old_flow = ctx->xin->flow;
> +retained->old_stack = ctx->stack;
> +retained->old_action_set = ctx->action_set;
> +ofpbuf_use_stub(>stack, retained->new_stack,
> +sizeof retained->new_stack);
> +ofpbuf_use_stub(>action_set, retained->actset_stub,
> +sizeof retained->actset_stub);
> +
> +return retained;
> +}
> +
> +static void
> +xretain_tunnel_mask_save(const struct xlate_ctx *ctx,
> + struct xretained_state *retained)
> +{
> +retained->flow_tnl_mask = ctx->wc->masks.tunnel;
> +}
> +
> +static void
> +xretain_base_flow_save(const struct xlate_ctx *ctx,
> + struct xretained_state *retained)
> +{
> +retained->old_base = ctx->base_flow;
> +}
> +
> +static void
> +xretain_base_flow_restore(struct xlate_ctx *ctx,
> + const struct xretained_state *retained)
> +{
> +ctx->base_flow = retained->old_base;
> +}
> +
> +static void
> +xretain_flow_restore(struct xlate_ctx *ctx,
> + const struct xretained_state *retained)
> +{
> +ctx->xin->flow = retained->old_flow;
> +}
> +
> +static void
> +xretain_tunnel_mask_restore(struct xlate_ctx *ctx,
> +const struct xretained_state *retained)
> +{
> +ctx->wc->masks.tunnel = retained->flow_tnl_mask;
> +}
> +
> +static void
> +xretain_state_restore_and_free(struct xlate_ctx *ctx,
> + struct xretained_state *retained)
> +{
> +ctx->xin->flow = retained->old_flow;
> +ofpbuf_uninit(>action_set);
> +ctx->action_set = retained->old_action_set;
> +ofpbuf_uninit(>stack);
> +ctx->stack = retained->old_stack;
> +
> +free(retained);
> +}
> +
> /* A controller may use OFPP_NONE as the ingress port to indicate that
> * it did not arrive on a "real" port. 'ofpp_none_bundle' exists for
> * when an input bundle is needed for validation (e.g., mirroring or
> @@ -3915,20 +3993,17 @@ static void
> patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
>struct xport *out_dev, bool is_last_action)
> {
> +bool old_was_mpls = ctx->was_mpls;
> struct flow *flow = >xin->flow;
> -struct flow old_flow = ctx->xin->flow;
> -struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel;
> bool old_conntrack = ctx->conntracked;
> -bool old_was_mpls = ctx->was_mpls;
> -ovs_version_t old_version = ctx->xin->tables_version;
> -struct ofpbuf old_stack = ctx->stack;
> -uint8_t new_stack[1024];
> -struct ofpbuf old_action_set = ctx->action_set;
> +struct xretained_state *retained_state;
> struct ovs_list
Re: [ovs-dev] [PATCH v4] ofproto-dpif-upcall: Mirror packets that are modified
On 10 Jul 2023, at 17:34, Mike Pattrick wrote:
> Currently OVS keeps track of which mirrors that each packet has been
> sent to for the purpose of deduplication. However, this doesn't consider
> that openflow rules can make significant changes to packets after
> ingress.
>
> For example, OVN can create OpenFlow rules that turn an echo request
> into an echo response by flipping source/destination addresses and
> setting the ICMP type to Reply. When a mirror is configured, only the
> request gets mirrored even though a response is recieved.
>
> This can cause a false impression of the actual traffic on wire if
> someone inspects the mirror and doesn't see an echo reply even though
> one has been sent.
>
> This patch resets the mirrors every time a packet is modified, so
> mirrors will recieve every copy of a packet that is sent for output.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2155579
> Signed-off-by: Mike Pattrick
Thanks Mike for fixing my v3 comments. This patch looks good to me.
Acked-by: Eelco Chaudron
> ---
> v2: Refactored all code into a single function
> v3: Cleaned up a code change that was incorrectly retained in v2 but
> not needed
> v4: Removed the default case from reset_mirror_ctx()
> ---
> ofproto/ofproto-dpif-xlate.c | 86
> tests/ofproto-dpif.at| 6 +--
> 2 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 29f4daa63..aba0cdb6e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6989,6 +6989,90 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx,
> "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie);
> }
>
> +/* Reset the mirror context if we modify the packet and would like to mirror
> + * the new copy. */
> +static void
> +reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, uint8_t
> type)
> +{
> +switch (type) {
> +case OFPACT_STRIP_VLAN:
> +case OFPACT_PUSH_VLAN:
> +case OFPACT_SET_ETH_SRC:
> +case OFPACT_SET_ETH_DST:
> +case OFPACT_PUSH_MPLS:
> +case OFPACT_POP_MPLS:
> +case OFPACT_SET_MPLS_LABEL:
> +case OFPACT_SET_MPLS_TC:
> +case OFPACT_SET_MPLS_TTL:
> +case OFPACT_DEC_MPLS_TTL:
> +case OFPACT_DEC_NSH_TTL:
> +case OFPACT_DEC_TTL:
> +case OFPACT_SET_FIELD:
> +case OFPACT_SET_VLAN_VID:
> +case OFPACT_SET_VLAN_PCP:
> +case OFPACT_ENCAP:
> +case OFPACT_DECAP:
> +case OFPACT_NAT:
> +ctx->mirrors = 0;
> +return;
> +case OFPACT_SET_IPV4_SRC:
> +case OFPACT_SET_IPV4_DST:
> +if (flow->dl_type == htons(ETH_TYPE_IP)) {
> +ctx->mirrors = 0;
> +}
> +return;
> +case OFPACT_SET_IP_DSCP:
> +case OFPACT_SET_IP_ECN:
> +case OFPACT_SET_IP_TTL:
> +if (is_ip_any(flow)) {
> +ctx->mirrors = 0;
> +}
> +return;
> +case OFPACT_SET_L4_SRC_PORT:
> +case OFPACT_SET_L4_DST_PORT:
> +if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
> +ctx->mirrors = 0;
> +}
> +return;
> +case OFPACT_OUTPUT_REG:
> +case OFPACT_OUTPUT_TRUNC:
> +case OFPACT_GROUP:
> +case OFPACT_OUTPUT:
> +case OFPACT_CONTROLLER:
> +case OFPACT_RESUBMIT:
> +case OFPACT_GOTO_TABLE:
> +case OFPACT_WRITE_METADATA:
> +case OFPACT_SET_TUNNEL:
> +case OFPACT_REG_MOVE:
> +case OFPACT_STACK_PUSH:
> +case OFPACT_STACK_POP:
> +case OFPACT_LEARN:
> +case OFPACT_ENQUEUE:
> +case OFPACT_SET_QUEUE:
> +case OFPACT_POP_QUEUE:
> +case OFPACT_MULTIPATH:
> +case OFPACT_BUNDLE:
> +case OFPACT_EXIT:
> +case OFPACT_UNROLL_XLATE:
> +case OFPACT_FIN_TIMEOUT:
> +case OFPACT_CLEAR_ACTIONS:
> +case OFPACT_WRITE_ACTIONS:
> +case OFPACT_METER:
> +case OFPACT_SAMPLE:
> +case OFPACT_CLONE:
> +case OFPACT_DEBUG_RECIRC:
> +case OFPACT_DEBUG_SLOW:
> +case OFPACT_CT:
> +case OFPACT_CT_CLEAR:
> +case OFPACT_CHECK_PKT_LARGER:
> +case OFPACT_DELETE_FIELD:
> +case OFPACT_NOTE:
> +case OFPACT_CONJUNCTION:
> +return;
> +}
> +OVS_NOT_REACHED();
> +}
> +
> static void
> do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> struct xlate_ctx *ctx, bool is_last_action,
> @@ -7030,6 +7114,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
> }
>
> +reset_mirror_ctx(ctx, flow, a->type);
> +
> if (OVS_UNLIKELY(ctx->xin->trace)) {
> struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER();
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 6824ce0bb..f242f77f3 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5349,7 +5349,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
>
Re: [ovs-dev] [PATCH v12 8/8] treewide: Add `ovs_assert` to check for null pointers
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This patch adds an assortment of `ovs_assert` statements to check for
> null pointers. We use assertions since it should be impossible for any
> of these pointers to be NULL.
>
> Signed-off-by: James Raphael Tiovalen
> Reviewed-by: Simon Horman
Hi James,
See comments inline below.
Cheers,
Eelco
> ---
> lib/dp-packet.c| 1 +
> lib/dpctl.c| 4
> lib/odp-execute.c | 4
> lib/rtnetlink.c| 4 ++--
> lib/shash.c| 2 +-
> lib/sset.c | 2 ++
> ovsdb/jsonrpc-server.c | 4
> ovsdb/monitor.c| 3 +++
> ovsdb/ovsdb-server.c | 2 ++
> ovsdb/ovsdb.c | 8 +++-
> ovsdb/query.c | 2 ++
> ovsdb/transaction.c| 2 ++
> utilities/ovs-vsctl.c | 3 +++
> vtep/vtep-ctl.c| 5 -
> 14 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index ca29b1f90..059db48ba 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -171,6 +171,7 @@ dp_packet_new_with_headroom(size_t size, size_t headroom)
> struct dp_packet *
> dp_packet_clone(const struct dp_packet *buffer)
> {
> +ovs_assert(buffer);
> return dp_packet_clone_with_headroom(buffer, 0);
> }
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 15950bd50..51cd6c88c 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -336,6 +336,8 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[],
> value = "";
> }
>
> +ovs_assert(key);
> +
I think we should not assert, but do proper error handling here with a
dpctl_error()
> if (!strcmp(key, "type")) {
> type = value;
> } else if (!strcmp(key, "port_no")) {
> @@ -454,6 +456,8 @@ dpctl_set_if(int argc, const char *argv[], struct
> dpctl_params *dpctl_p)
> value = "";
> }
>
> +ovs_assert(key);
> +
Same as above.
> if (!strcmp(key, "type")) {
> if (strcmp(value, type)) {
> dpctl_error(dpctl_p, 0,
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 5cf6fbec0..44b2cd360 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -147,6 +147,8 @@ odp_set_ipv4(struct dp_packet *packet, const struct
> ovs_key_ipv4 *key,
> uint8_t new_tos;
> uint8_t new_ttl;
>
> +ovs_assert(nh);
> +
> if (mask->ipv4_src) {
> ip_src_nh = get_16aligned_be32(>ip_src);
> new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src);
> @@ -276,6 +278,8 @@ set_arp(struct dp_packet *packet, const struct
> ovs_key_arp *key,
> {
> struct arp_eth_header *arp = dp_packet_l3(packet);
>
> +ovs_assert(arp);
> +
> if (!mask) {
> arp->ar_op = key->arp_op;
> arp->ar_sha = key->arp_sha;
> diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
> index f67352603..37078d00e 100644
> --- a/lib/rtnetlink.c
> +++ b/lib/rtnetlink.c
> @@ -112,7 +112,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct
> rtnetlink_change *change)
> if (parsed) {
> const struct ifinfomsg *ifinfo;
>
> -ifinfo = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifinfo);
> +ifinfo = ofpbuf_at_assert(buf, NLMSG_HDRLEN, sizeof *ifinfo);
>
> /* Wireless events can be spammy and cause a
> * lot of unnecessary churn and CPU load in
> @@ -175,7 +175,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct
> rtnetlink_change *change)
> if (parsed) {
> const struct ifaddrmsg *ifaddr;
>
> -ifaddr = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifaddr);
> +ifaddr = ofpbuf_at_assert(buf, NLMSG_HDRLEN, sizeof *ifaddr);
>
> change->nlmsg_type = nlmsg->nlmsg_type;
> change->if_index = ifaddr->ifa_index;
> diff --git a/lib/shash.c b/lib/shash.c
> index b72b5bf27..c712b3769 100644
> --- a/lib/shash.c
> +++ b/lib/shash.c
> @@ -270,7 +270,7 @@ void *
> shash_find_and_delete_assert(struct shash *sh, const char *name)
> {
> void *data = shash_find_and_delete(sh, name);
> -ovs_assert(data != NULL);
> +ovs_assert(data);
> return data;
> }
>
> diff --git a/lib/sset.c b/lib/sset.c
> index aa1790020..afdc4392c 100644
> --- a/lib/sset.c
> +++ b/lib/sset.c
> @@ -20,6 +20,7 @@
>
> #include "openvswitch/dynamic-string.h"
> #include "hash.h"
> +#include "util.h"
>
> static uint32_t
> hash_name__(const char *name, size_t length)
> @@ -261,6 +262,7 @@ char *
> sset_pop(struct sset *set)
> {
> const char *name = SSET_FIRST(set);
> +ovs_assert(name);
I think if we do a pop from an empty set, we should just return NULL, not
assert.
> char *copy = xstrdup(name);
> sset_delete(set, SSET_NODE_FROM_NAME(name));
> return copy;
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 5361b3c76..a3ca48a7b 100644
> --- a/ovsdb/jsonrpc-server.c
> +++
Re: [ovs-dev] [ovn] ovn-controller high memory consumption with sbrec_server_has_##table##_table - enabled codepath
On 7/10/23 22:20, Vladislav Odintsov wrote: > Hi Dumitru, > > thanks for digging into this! I highly appreciate your help! > No worries, my pleasure! :) > Please, see my answers inline. > >> On 10 Jul 2023, at 15:28, Dumitru Ceara wrote: >> >> On 7/10/23 12:57, Dumitru Ceara wrote: >>> On 7/6/23 13:00, Vladislav Odintsov wrote: > On 5 Jul 2023, at 20:07, Vladislav Odintsov wrote: > > Hi Dumitru, > > thanks for the quick response! > >> On 5 Jul 2023, at 19:53, Dumitru Ceara wrote: >> >> On 7/5/23 17:14, Vladislav Odintsov wrote: >>> Hi, >>> >> >> Hi Vladislav, >> >>> we’ve noticed there is a huge ovn-controller memory consumption >>> introduced with [0] comparing to version without its changes in >>> ovn-controller.c part (just OVS submodule bump without ovn-controller >>> changes doesn’t trigger such behaviour). >>> >> >> Thanks for reporting this! >> >>> On an empty host connected to working cluster ovn-controller normally >>> consumes ~175 MiB RSS, and the same host updated to a version with >>> commit [0] consumes 3.3 GiB RSS. The start time and CPU consumption >>> during process start of an affected version is higher that for the >>> normal version. >>> >>> Before upgrade (normal functioning): >>> >>> #ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof >>> ovn-controller)| grep ovn >>> idl-cells-OVN_Southbound:343855 idl-cells-Open_vSwitch:14131 >>> ofctrl_desired_flow_usage-KB:76 ofctrl_installed_flow_usage-KB:60 >>> ofctrl_sb_flow_ref_usage-KB:18 >>> 174.2 MiB + 327.5 KiB = 174.5 MiB ovn-controller >>> >>> After upgrade to an affected version I’ve checked memory report while >>> ovn-controller was starting and at some time there was a bigger amount >>> of OVN_Southbound cells comparing to "after start" time: >>> >>> during start: >>> >>> # ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof >>> ovn-controller)| grep ovn >>> idl-cells-OVN_Southbound:11388742 idl-cells-Open_vSwitch:14131 >>> idl-outstanding-txns-Open_vSwitch:1 >>> 3.3 GiB + 327.0 KiB = 3.3 GiB ovn-controller >>> >>> after start: >>> >>> # ovn-appctl -t ovn-controller memory/show ;ps-mem -p $(pidof >>> ovn-controller)| grep ovn >>> idl-cells-OVN_Southbound:343896 idl-cells-Open_vSwitch:14131 >>> idl-outstanding-txns-Open_vSwitch:1 >>> 3.3 GiB + 327.0 KiB = 3.3 GiB ovn-controller >>> >>> >>> cells during start: >>> 11388742 >>> >>> cells after start: >>> 343896 >>> >> >> Are you running with ovn-monitor-all=true on this host? > > No, it has default false. > >> >> I guess it's unlikely but I'll try just in case: would it be possible to >> share the SB database? > > Unfortunately, no. But I can say it’s about 450 M in size after > compaction. And there are about 1M mac_bindings if it’s important :). >>> >>> I tried in a sandbox, before and after the commit in question, with a >>> script that adds 1M mac bindings on top of the sample topology built by >>> tutorial/ovn-setup.sh. >>> >>> I see ovn-controller memory usage going to >3GB in before the commit you >>> blamed and to >1.9GB after the same commit. So it looks different than >>> what you reported but still worrying that we use so much memory for mac >>> bindings. >>> >>> I'm assuming however that quite a few of the 1M mac bindings in your >>> setup are stale so would it be possible to enable mac_binding aging? It >>> needs to be enabled per router with something like: >>> >>> $ ovn-nbctl set logical_router RTR options:mac_binding_age_threshold=60 >>> >>> The threshold is in seconds and is a hard limit (for now) after which a >>> mac binding entry is removed. There's work in progress to only clear >>> arp cache entries that are really stale [1]. > > Yeah, I know about mac_binding ageing, but we currently use our own mechanism > to delete excess records and waiting for patchset you’ve mentioned. > Ack. >>> But if you are interested in any specific information, let me know it, I can check. >>> >>> How many "local" datapaths do we have on the hosts that exhibit high >>> memory usage? >>> >>> The quickest way I can think of to get this info is to run this on the >>> hypervisor: >>> $ ovn-appctl ct-zone-list | grep snat -c >>> >>> Additional question: how many mac_bindings are "local", i.e., associated >>> to local datapaths? > > For both questions: 0. > OK, that really suggests that malloc_trim() didn't get called. There's really no reason (especially with ovn-monitor-all=false) to have such a high memory usage in ovn-controller if there's no local datapath. >>> > >> >>> I guess it could be connected with this problem. Can anyone look at >>> this and comment please? >>> >>
Re: [ovs-dev] [PATCH v12 7/8] lib, ovsdb: Add various null pointer checks
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds various null pointer checks to some files in the `lib`
> and the `ovsdb` directories to fix several Coverity defects. These
> changes are grouped together as they perform similar checks, returning
> early or skipping some action if a null pointer is encountered.
>
> Signed-off-by: James Raphael Tiovalen
> Reviewed-by: Simon Horman
Thanks for the patch James, some small nits below.
Cheers,
Eelco
> ---
> lib/dp-packet.c| 8
> lib/shash.c| 7 ++-
> ovsdb/jsonrpc-server.c | 2 +-
> ovsdb/monitor.c| 7 ++-
> ovsdb/ovsdb-client.c | 7 ++-
> ovsdb/row.c| 4 +++-
> 6 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 445cb4761..ca29b1f90 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -353,7 +353,7 @@ void *
> dp_packet_put_zeros(struct dp_packet *b, size_t size)
> {
> void *dst = dp_packet_put_uninit(b, size);
> -memset(dst, 0, size);
> +nullable_memset(dst, 0, size);
> return dst;
> }
>
> @@ -364,7 +364,7 @@ void *
> dp_packet_put(struct dp_packet *b, const void *p, size_t size)
> {
> void *dst = dp_packet_put_uninit(b, size);
> -memcpy(dst, p, size);
> +nullable_memcpy(dst, p, size);
> return dst;
> }
>
> @@ -436,7 +436,7 @@ void *
> dp_packet_push_zeros(struct dp_packet *b, size_t size)
> {
> void *dst = dp_packet_push_uninit(b, size);
> -memset(dst, 0, size);
> +nullable_memset(dst, 0, size);
> return dst;
> }
>
> @@ -447,7 +447,7 @@ void *
> dp_packet_push(struct dp_packet *b, const void *p, size_t size)
> {
> void *dst = dp_packet_push_uninit(b, size);
> -memcpy(dst, p, size);
> +nullable_memcpy(dst, p, size);
> return dst;
> }
>
> diff --git a/lib/shash.c b/lib/shash.c
> index 2bfc8eb50..b72b5bf27 100644
> --- a/lib/shash.c
> +++ b/lib/shash.c
> @@ -205,8 +205,13 @@ shash_delete(struct shash *sh, struct shash_node *node)
> char *
> shash_steal(struct shash *sh, struct shash_node *node)
> {
> -char *name = node->name;
> +char *name;
>
> +if (!node) {
> +return NULL;
> +}
> +
> +name = node->name;
Maybe just define it here (and remove it above)..
char *name = node->name;
> hmap_remove(>map, >node);
> free(node);
> return name;
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 17868f5b7..5361b3c76 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -1038,7 +1038,7 @@ ovsdb_jsonrpc_session_got_request(struct
> ovsdb_jsonrpc_session *s,
> request->id);
> } else if (!strcmp(request->method, "get_schema")) {
> struct ovsdb *db = ovsdb_jsonrpc_lookup_db(s, request, );
> -if (!reply) {
> +if (db && !reply) {
> reply = jsonrpc_create_reply(ovsdb_schema_to_json(db->schema),
> request->id);
> }
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index c32af7b02..b560b0745 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -483,7 +483,9 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
> struct ovsdb_monitor_column *c;
>
> mt = shash_find_data(>tables, table->schema->name);
> -
> +if (!mt) {
> +return NULL;
> +}
Line feed.
> /* Check for column duplication. Return duplicated column name. */
> if (mt->columns_index_map[column->index] != -1) {
> return column->name;
> @@ -810,6 +812,9 @@ ovsdb_monitor_table_condition_update(
>
> struct ovsdb_monitor_table_condition *mtc =
> shash_find_data(>tables, table->schema->name);
> +if (!mtc) {
> +return NULL;
> +}
Looks a bit odd in the middle of the definitions. Maybe move it down?
struct ovsdb_error *error;
struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER();
struct ovsdb_monitor_table_condition *mtc =
shash_find_data(>tables, table->schema->name);
if (!mtc) {
return NULL;
}
Or add surrounding new lines:
struct ovsdb_monitor_table_condition *mtc =
shash_find_data(>tables, table->schema->name);
if (!mtc) {
return NULL;
}
struct ovsdb_error *error;
struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER();
> struct ovsdb_error *error;
> struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER();
>
> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> index 46484630d..2b12907b6 100644
> --- a/ovsdb/ovsdb-client.c
> +++ b/ovsdb/ovsdb-client.c
> @@ -1873,6 +1873,10 @@ do_dump(struct jsonrpc *rpc, const char *database,
> n_tables = shash_count(>tables);
> }
>
> +if (!tables) {
> +goto end;
> +}
> +
> /* Construct transaction to retrieve entire database. */
> transaction = json_array_create_1(json_string_create(database));
Re: [ovs-dev] [PATCH v12 6/8] ovs-vsctl: Fix crash when routing is enabled
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: > In the case where routing is enabled, the bridge member of the > `vsctl_port` structs is not populated. This can cause a crash if we > attempt to access it. This patch fixes the crash by checking if the > bridge member is valid before attempting to access it. In the > `check_conflicts` function, we print both the port name and the bridge > name if routing is disabled and we only print the port name if routing > is enabled. > > Signed-off-by: James Raphael Tiovalen > Reviewed-by: Simon Horman Thank for fixing this James, this patch looks 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 v12 5/8] file, monitor: Add null pointer assertions for old and new ovsdb_rows
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds non-null pointer assertions in some code that performs
> some decisions based on old and new input ovsdb_rows.
>
> Signed-off-by: James Raphael Tiovalen
> Reviewed-by: Simon Horman
What about error messages/argument checking instead of asserts here?
//Eelco
> ---
> ovsdb/file.c| 2 ++
> ovsdb/monitor.c | 4 +++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ovsdb/file.c b/ovsdb/file.c
> index 2d887e53e..b1d386e76 100644
> --- a/ovsdb/file.c
> +++ b/ovsdb/file.c
> @@ -522,7 +522,9 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
> }
>
> if (row) {
> +ovs_assert(new || old);
> struct ovsdb_table *table = new ? new->table : old->table;
> +ovs_assert(table);
> char uuid[UUID_LEN + 1];
>
> if (table != ftxn->table) {
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index 4afaa89f4..c32af7b02 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -1372,8 +1372,10 @@ ovsdb_monitor_changes_update(const struct ovsdb_row
> *old,
> const struct ovsdb_monitor_table *mt,
> struct ovsdb_monitor_change_set_for_table *mcst)
> {
> +ovs_assert(new || old);
> const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
> -struct ovsdb_monitor_row *change;
> +ovs_assert(uuid);
> +struct ovsdb_monitor_row *change = NULL;
>
> change = ovsdb_monitor_changes_row_find(mcst, uuid);
> if (!change) {
> --
> 2.40.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 v12 4/8] ovsdb: Assert and check return values of `ovsdb_table_schema_get_column`
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds a few null pointer assertions and checks to some return
> values of `ovsdb_table_schema_get_column`. If a null pointer is
> encountered in these blocks, either the assertion will fail or the
> control flow will now be redirected to alternative paths which will
> output the appropriate error messages.
>
> A few ovsdb-rbac and ovsdb-server tests are also updated to verify the
> expected warning logs by adding said logs to the ALLOWLIST of the
> OVSDB_SERVER_SHUTDOWN statements.
>
> Signed-off-by: James Raphael Tiovalen
In general, this looks good, however, I’m not a ovsdb guy, so I’m wondering if
the asserts could not cause any additional crashes that can be avoided by a
different type of error handling. Also is there an easy way to make this crash
happen by giving some invalid input?
Ilya any comments here?
//Eelco
> ---
> ovsdb/condition.c | 5 -
> ovsdb/ovsdb-client.c | 7 +--
> ovsdb/ovsdb-util.c| 6 ++
> tests/ovsdb-rbac.at | 4 +++-
> tests/ovsdb-server.at | 8 ++--
> 5 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/ovsdb/condition.c b/ovsdb/condition.c
> index 09c89b2a0..5a3eb4e8a 100644
> --- a/ovsdb/condition.c
> +++ b/ovsdb/condition.c
> @@ -47,7 +47,10 @@ ovsdb_clause_from_json(const struct ovsdb_table_schema *ts,
>
> /* Column and arg fields are not being used with boolean functions.
> * Use dummy values */
> -clause->column = ovsdb_table_schema_get_column(ts, "_uuid");
> +const struct ovsdb_column *uuid_column =
> + ovsdb_table_schema_get_column(ts, "_uuid");
> +ovs_assert(uuid_column);
> +clause->column = uuid_column;
> clause->index = clause->column->index;
> ovsdb_datum_init_default(>arg, >column->type);
> return NULL;
> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> index bae2c5f04..46484630d 100644
> --- a/ovsdb/ovsdb-client.c
> +++ b/ovsdb/ovsdb-client.c
> @@ -1232,8 +1232,11 @@ parse_monitor_columns(char *arg, const char *server,
> const char *database,
> }
> free(nodes);
>
> -add_column(server, ovsdb_table_schema_get_column(table, "_version"),
> - columns, columns_json);
> +const struct ovsdb_column *version_column =
> +ovsdb_table_schema_get_column(table, "_version");
> +
> +ovs_assert(version_column);
> +add_column(server, version_column, columns, columns_json);
> }
>
> if (!initial || !insert || !delete || !modify) {
> diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
> index 303191dc8..0b9e1df54 100644
> --- a/ovsdb/ovsdb-util.c
> +++ b/ovsdb/ovsdb-util.c
> @@ -291,9 +291,15 @@ ovsdb_util_write_string_string_column(struct ovsdb_row
> *row,
> size_t i;
>
> column = ovsdb_table_schema_get_column(row->table->schema, column_name);
> +if (!column) {
> +VLOG_WARN("No %s column present in the %s table",
> + column_name, row->table->schema->name);
> +goto unwind;
> +}
> datum = ovsdb_util_get_datum(row, column_name, OVSDB_TYPE_STRING,
> OVSDB_TYPE_STRING, UINT_MAX);
> if (!datum) {
> +unwind:
> for (i = 0; i < n; i++) {
> free(keys[i]);
> free(values[i]);
> diff --git a/tests/ovsdb-rbac.at b/tests/ovsdb-rbac.at
> index 7de3711fb..3172e4bf5 100644
> --- a/tests/ovsdb-rbac.at
> +++ b/tests/ovsdb-rbac.at
> @@ -371,5 +371,7 @@ cat stdout >> output
> AT_CHECK([uuidfilt stdout], [0], [[[{"count":1}]]
> ], [ignore])
>
> -OVSDB_SERVER_SHUTDOWN
> +OVSDB_SERVER_SHUTDOWN(["
> + /No status column present in the Connection table/d
> +"])
> AT_CLEANUP
> diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
> index b53ab8f52..8ccec80bc 100644
> --- a/tests/ovsdb-server.at
> +++ b/tests/ovsdb-server.at
> @@ -428,7 +428,9 @@ AT_CHECK(
>
> [[[{"rows":[{"managers":"punix:socket1"}]},{"rows":[{"is_connected":false,"target":"punix:socket2"}]}]
> ]],
>[ignore])
> -OVSDB_SERVER_SHUTDOWN
> +OVSDB_SERVER_SHUTDOWN(["
> + /No status column present in the Manager table/d
> +"])
> AT_CLEANUP
>
> AT_SETUP([ovsdb-server/add-remote and remove-remote])
> @@ -2110,7 +2112,9 @@ AT_CHECK([ovsdb-client transact tcp:127.0.0.1:$TCP_PORT
> \
> cat stdout >> output
> AT_CHECK([uuidfilt output], [0], [[[{"details":"insert operation not allowed
> when database server is in read only mode","error":"not allowed"}]]
> ], [ignore])
> -OVSDB_SERVER_SHUTDOWN
> +OVSDB_SERVER_SHUTDOWN(["
> + /No status column present in the Manager table/d
> +"])
> AT_CLEANUP
>
> AT_SETUP([ovsdb-server replication with schema mismatch])
> --
> 2.40.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 3/8] shash, simap, smap: Add assertions to `*_count` functions
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds assertions in the functions `shash_count`,
> `simap_count`, and `smap_count` to ensure that the corresponding input
> struct pointer is not NULL.
>
> This ensures that if the return values of `shash_sort`, `simap_sort`,
> or `smap_sort` are NULL, then the following for loops would not attempt
> to access the pointer, which might result in segmentation faults or
> undefined behavior.
>
> Signed-off-by: James Raphael Tiovalen
> Reviewed-by: Simon Horman
> ---
> lib/shash.c | 2 ++
> lib/simap.c | 2 ++
> lib/smap.c | 1 +
> 3 files changed, 5 insertions(+)
>
> diff --git a/lib/shash.c b/lib/shash.c
> index a7b2c6458..2bfc8eb50 100644
> --- a/lib/shash.c
> +++ b/lib/shash.c
> @@ -17,6 +17,7 @@
> #include
> #include "openvswitch/shash.h"
> #include "hash.h"
> +#include "util.h"
>
> static struct shash_node *shash_find__(const struct shash *,
> const char *name, size_t name_len,
> @@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash)
> size_t
> shash_count(const struct shash *shash)
> {
> +ovs_assert(shash);
My preference would be to return 0, in these instances. What do others think?
> return hmap_count(>map);
> }
>
> diff --git a/lib/simap.c b/lib/simap.c
> index 0ee08d74d..1c01d4ebe 100644
> --- a/lib/simap.c
> +++ b/lib/simap.c
> @@ -17,6 +17,7 @@
> #include
> #include "simap.h"
> #include "hash.h"
> +#include "util.h"
>
> static size_t hash_name(const char *, size_t length);
> static struct simap_node *simap_find__(const struct simap *,
> @@ -84,6 +85,7 @@ simap_is_empty(const struct simap *simap)
> size_t
> simap_count(const struct simap *simap)
> {
> +ovs_assert(simap);
> return hmap_count(>map);
> }
>
> diff --git a/lib/smap.c b/lib/smap.c
> index 47fb34502..122adca27 100644
> --- a/lib/smap.c
> +++ b/lib/smap.c
> @@ -300,6 +300,7 @@ smap_is_empty(const struct smap *smap)
> size_t
> smap_count(const struct smap *smap)
> {
> +ovs_assert(smap);
> return hmap_count(>map);
> }
>
> --
> 2.40.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 v12 2/8] lib, ovs-vsctl: Add zero-initializations
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds zero-initializations by changing `SFL_ALLOC` from
> `malloc` to `xzalloc`, adding a `memset` call to `sflAlloc`,
> initializing a `pollfd` struct variable with zeroes, and changing some
> calls to `xmalloc` to `xzalloc`. This is to prevent potential data leaks
> or undefined behavior from potentially uninitialized variables.
>
> Some variables would always be initialized by either the code flow or
> the compiler. Thus, some of the associated Coverity reports might be
> false positives. That said, it is still considered best practice to
> zero-initialize variables upfront just in case to ensure the overall
> resilience and security of OVS, as long as they do not impact
> performance-critical code. As a bonus, it would also make static
> analyzer tools, such as Coverity, happy.
Thanks for the changes, some comments below.
Cheers,
Eelco
> Signed-off-by: James Raphael Tiovalen
> ---
> lib/latch-unix.c | 2 +-
> lib/sflow_agent.c | 12 ++--
> lib/sflow_api.h | 2 +-
> utilities/ovs-vsctl.c | 5 ++---
> 4 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/lib/latch-unix.c b/lib/latch-unix.c
> index f4d10c39a..c62bb024b 100644
> --- a/lib/latch-unix.c
> +++ b/lib/latch-unix.c
> @@ -71,7 +71,7 @@ latch_set(struct latch *latch)
> bool
> latch_is_set(const struct latch *latch)
> {
> -struct pollfd pfd;
> +struct pollfd pfd = {0};
> int retval;
>
> pfd.fd = latch->fds[0];
> diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c
> index c95f654a5..b15758ae1 100644
> --- a/lib/sflow_agent.c
> +++ b/lib/sflow_agent.c
> @@ -510,8 +510,16 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName,
> char *msg)
>
> static void * sflAlloc(SFLAgent *agent, size_t bytes)
> {
> -if(agent->allocFn) return (*agent->allocFn)(agent->magic, agent, bytes);
> -else return SFL_ALLOC(bytes);
> +void *alloc;
Add a new line here.
> +if (agent->allocFn) {
> +alloc = (*agent->allocFn)(agent->magic, agent, bytes);
> +ovs_assert(alloc);
> +memset(alloc, 0, bytes);
> +} else {
> +alloc = SFL_ALLOC(bytes);
> +ovs_assert(alloc);
Assert should not be needed as xzalloc will do this.
> +}
> +return alloc;
> }
>
> static void sflFree(SFLAgent *agent, void *obj)
> diff --git a/lib/sflow_api.h b/lib/sflow_api.h
> index a0530b37a..eb23e2acd 100644
> --- a/lib/sflow_api.h
> +++ b/lib/sflow_api.h
> @@ -337,7 +337,7 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName,
> char *msg);
>
> u_int32_t sfl_receiver_samplePacketsSent(SFLReceiver *receiver);
>
> -#define SFL_ALLOC malloc
> +#define SFL_ALLOC xzalloc
> #define SFL_FREE free
>
> #endif /* SFLOW_API_H */
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 2f5ac1a26..b3c75f8ba 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -575,7 +575,7 @@ add_bridge_to_cache(struct vsctl_context *vsctl_ctx,
> struct ovsrec_bridge *br_cfg, const char *name,
> struct vsctl_bridge *parent, int vlan)
> {
> -struct vsctl_bridge *br = xmalloc(sizeof *br);
> +struct vsctl_bridge *br = xzalloc(sizeof *br);
> br->br_cfg = br_cfg;
> br->name = xstrdup(name);
> ovs_list_init(>ports);
> @@ -659,7 +659,7 @@ static struct vsctl_port *
> add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge
> *parent,
>struct ovsrec_port *port_cfg)
> {
> -struct vsctl_port *port;
> +struct vsctl_port *port = xzalloc(sizeof *port);
>
> if (port_cfg->tag
> && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) {
> @@ -671,7 +671,6 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct
> vsctl_bridge *parent,
> }
> }
>
> -port = xmalloc(sizeof *port);
> ovs_list_push_back(>ports, >ports_node);
> ovs_list_init(>ifaces);
> port->port_cfg = port_cfg;
> --
> 2.40.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 ovn v2] ci: Remove '--recheck' in CI.
On 7/11/23 10:30, Ales Musil wrote:
> On Tue, Jul 11, 2023 at 7:14 AM Ales Musil wrote:
>
>>
>>
>> On Mon, Jul 10, 2023 at 5:26 PM Dumitru Ceara wrote:
>>
>>> If we want to catch new failures faster we have a better chance if CI
>>> doesn't auto-retry (once).
>>>
>>> There are some tests that are still "unstable" and fail every now and
>>> then. In order to reduce the number of false negatives keep the
>>> --recheck for them. To achieve that we use a new macro, TAG_UNSTABLE,
>>> to tag all these tests. The list of "unstable" tests is compiled based
>>> on the following discussion:
>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405465.html
>>>
>>> In order to avoid new GitHub actions jobs, we re-purpose the last job of
>>> each target type to also run the unstable tests. These jobs were
>>> already running less tests than others so the additional run time should
>>> not be an issue.
>>>
>>> Signed-off-by: Dumitru Ceara
>>> ---
>>> V2:
>>> - Addressed Ales' comments:
>>> - always run stable and unstable tests before declaring pass/fail
>>> Changes in v1 (since RFC):
>>> - kept recheck for unstable tests
>>> - introduced TAG_UNSTABLE
>>> - changed test.yml to run unstable tests in the last batch of every
>>> test target type.
>>> ---
>>> .ci/ci.sh | 2 +-
>>> .ci/linux-build.sh | 76 ++
>>> .github/workflows/test.yml | 15
>>> tests/ovn-ic.at| 1 +
>>> tests/ovn-ipsec.at | 1 +
>>> tests/ovn-macros.at| 5 +++
>>> tests/ovn-northd.at| 1 +
>>> tests/ovn-performance.at | 1 +
>>> tests/ovn.at | 13 +++
>>> 9 files changed, 92 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/.ci/ci.sh b/.ci/ci.sh
>>> index 10f11939c5..a500aba764 100755
>>> --- a/.ci/ci.sh
>>> +++ b/.ci/ci.sh
>>> @@ -101,7 +101,7 @@ function run_tests() {
>>> && \
>>> ARCH=$ARCH CC=$CC LIBS=$LIBS OPTS=$OPTS TESTSUITE=$TESTSUITE \
>>> TEST_RANGE=$TEST_RANGE SANITIZERS=$SANITIZERS DPDK=$DPDK \
>>> -./.ci/linux-build.sh
>>> +UNSTABLE=$UNSTABLE ./.ci/linux-build.sh
>>>
>>
> I've missed one thing, please add the recheck here as well.
>
>
Good catch! I'll send a v3. I think I also need to add at least one
more test to the list of unstable ones: "system-ovn.at:11332 Tiered ACLs
-- ovn-northd -- parallelization=yes -- ovn_monitor_all=yes"
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 1/8] lib: Add non-null assertions to some return values of `dp_packet_data`
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds some `ovs_assert()` checks to some return values of
> `dp_packet_data()` to ensure that they are not NULL and to prevent
> null-pointer dereferences, which might lead to unwanted crashes. We use
> assertions since it should be impossible for these calls to
> `dp_packet_data()` to return NULL.
>
> Signed-off-by: James Raphael Tiovalen
> Reviewed-by: Simon Horman
This patch looks good, however, it needs a re-work due to changes to tunnel
checksum handling.
//Eelco
> ---
> lib/dp-packet.c | 15 ++-
> lib/netdev-native-tnl.c | 17 +++--
> lib/pcap-file.c | 4 +++-
> 3 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index ae8ab5800..445cb4761 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -183,9 +183,12 @@ dp_packet_clone_with_headroom(const struct dp_packet
> *buffer, size_t headroom)
> struct dp_packet *new_buffer;
> uint32_t mark;
>
> -new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
> - dp_packet_size(buffer),
> - headroom);
> +const void *data_dp = dp_packet_data(buffer);
> +ovs_assert(data_dp);
> +
> +new_buffer = dp_packet_clone_data_with_headroom(data_dp,
> +dp_packet_size(buffer),
> +headroom);
> /* Copy the following fields into the returned buffer: l2_pad_size,
> * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
> memcpy(_buffer->l2_pad_size, >l2_pad_size,
> @@ -322,8 +325,10 @@ dp_packet_shift(struct dp_packet *b, int delta)
> : true);
>
> if (delta != 0) {
> -char *dst = (char *) dp_packet_data(b) + delta;
> -memmove(dst, dp_packet_data(b), dp_packet_size(b));
> +const void *data_dp = dp_packet_data(b);
> +ovs_assert(data_dp);
> +char *dst = (char *) data_dp + delta;
> +memmove(dst, data_dp, dp_packet_size(b));
> dp_packet_set_data(b, dst);
> }
> }
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index c2c6ca559..7781b162d 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -43,6 +43,7 @@
> #include "seq.h"
> #include "unaligned.h"
> #include "unixctl.h"
> +#include "util.h"
> #include "openvswitch/vlog.h"
>
> VLOG_DEFINE_THIS_MODULE(native_tnl);
> @@ -222,12 +223,13 @@ netdev_tnl_calc_udp_csum(struct udp_header *udp, struct
> dp_packet *packet,
> {
> uint32_t csum;
>
> -if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> -csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr(
> - dp_packet_data(packet)));
> +void *data_dp = dp_packet_data(packet);
> +ovs_assert(data_dp);
> +
> +if (netdev_tnl_is_header_ipv6(data_dp)) {
> +csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr(data_dp));
> } else {
> -csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr(
> -dp_packet_data(packet)));
> +csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr(data_dp));
> }
>
> csum = csum_continue(csum, udp, ip_tot_size);
> @@ -428,7 +430,10 @@ netdev_gre_pop_header(struct dp_packet *packet)
> struct flow_tnl *tnl = >tunnel;
> int hlen = sizeof(struct eth_header) + 4;
>
> -hlen += netdev_tnl_is_header_ipv6(dp_packet_data(packet)) ?
> +const void *data_dp = dp_packet_data(packet);
> +ovs_assert(data_dp);
> +
> +hlen += netdev_tnl_is_header_ipv6(data_dp) ?
> IPV6_HEADER_LEN : IP_HEADER_LEN;
>
> pkt_metadata_init_tnl(md);
> diff --git a/lib/pcap-file.c b/lib/pcap-file.c
> index 3ed7ea488..9f4e2e1e2 100644
> --- a/lib/pcap-file.c
> +++ b/lib/pcap-file.c
> @@ -284,6 +284,8 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet
> *buf)
> struct timeval tv;
>
> ovs_assert(dp_packet_is_eth(buf));
> +const void *data_dp = dp_packet_data(buf);
> +ovs_assert(data_dp);
>
> xgettimeofday();
> prh.ts_sec = tv.tv_sec;
> @@ -291,7 +293,7 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet
> *buf)
> prh.incl_len = dp_packet_size(buf);
> prh.orig_len = dp_packet_size(buf);
> ignore(fwrite(, sizeof prh, 1, p_file->file));
> -ignore(fwrite(dp_packet_data(buf), dp_packet_size(buf), 1,
> p_file->file));
> +ignore(fwrite(data_dp, dp_packet_size(buf), 1, p_file->file));
> fflush(p_file->file);
> }
>
> --
> 2.40.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
Re: [ovs-dev] [PATCH ovn v2] ci: Remove '--recheck' in CI.
On Tue, Jul 11, 2023 at 7:14 AM Ales Musil wrote:
>
>
> On Mon, Jul 10, 2023 at 5:26 PM Dumitru Ceara wrote:
>
>> If we want to catch new failures faster we have a better chance if CI
>> doesn't auto-retry (once).
>>
>> There are some tests that are still "unstable" and fail every now and
>> then. In order to reduce the number of false negatives keep the
>> --recheck for them. To achieve that we use a new macro, TAG_UNSTABLE,
>> to tag all these tests. The list of "unstable" tests is compiled based
>> on the following discussion:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405465.html
>>
>> In order to avoid new GitHub actions jobs, we re-purpose the last job of
>> each target type to also run the unstable tests. These jobs were
>> already running less tests than others so the additional run time should
>> not be an issue.
>>
>> Signed-off-by: Dumitru Ceara
>> ---
>> V2:
>> - Addressed Ales' comments:
>> - always run stable and unstable tests before declaring pass/fail
>> Changes in v1 (since RFC):
>> - kept recheck for unstable tests
>> - introduced TAG_UNSTABLE
>> - changed test.yml to run unstable tests in the last batch of every
>> test target type.
>> ---
>> .ci/ci.sh | 2 +-
>> .ci/linux-build.sh | 76 ++
>> .github/workflows/test.yml | 15
>> tests/ovn-ic.at| 1 +
>> tests/ovn-ipsec.at | 1 +
>> tests/ovn-macros.at| 5 +++
>> tests/ovn-northd.at| 1 +
>> tests/ovn-performance.at | 1 +
>> tests/ovn.at | 13 +++
>> 9 files changed, 92 insertions(+), 23 deletions(-)
>>
>> diff --git a/.ci/ci.sh b/.ci/ci.sh
>> index 10f11939c5..a500aba764 100755
>> --- a/.ci/ci.sh
>> +++ b/.ci/ci.sh
>> @@ -101,7 +101,7 @@ function run_tests() {
>> && \
>> ARCH=$ARCH CC=$CC LIBS=$LIBS OPTS=$OPTS TESTSUITE=$TESTSUITE \
>> TEST_RANGE=$TEST_RANGE SANITIZERS=$SANITIZERS DPDK=$DPDK \
>> -./.ci/linux-build.sh
>> +UNSTABLE=$UNSTABLE ./.ci/linux-build.sh
>>
>
I've missed one thing, please add the recheck here as well.
> "
>> }
>>
>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
>> index 5a79a52daf..4c5361f3ca 100755
>> --- a/.ci/linux-build.sh
>> +++ b/.ci/linux-build.sh
>> @@ -9,6 +9,7 @@ COMMON_CFLAGS=""
>> OVN_CFLAGS=""
>> OPTS="$OPTS --enable-Werror"
>> JOBS=${JOBS:-"-j4"}
>> +RECHECK=${RECHECK:-"no"}
>>
>> function install_dpdk()
>> {
>> @@ -99,6 +100,17 @@ function configure_clang()
>> COMMON_CFLAGS="${COMMON_CFLAGS}
>> -Wno-error=unused-command-line-argument"
>> }
>>
>> +function run_tests()
>> +{
>> +if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
>> +TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=$RECHECK
>> +then
>> +# testsuite.log is necessary for debugging.
>> +cat */_build/sub/tests/testsuite.log
>> +return 1
>> +fi
>> +}
>> +
>> function execute_tests()
>> {
>> # 'distcheck' will reconfigure with required options.
>> @@ -106,27 +118,61 @@ function execute_tests()
>> configure_ovn
>>
>> export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
>> -if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
>> -TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=yes
>> -then
>> -# testsuite.log is necessary for debugging.
>> -cat */_build/sub/tests/testsuite.log
>> +
>> +local stable_rc=0
>> +local unstable_rc=0
>> +
>> +if ! SKIP_UNSTABLE=yes run_tests; then
>> +stable_rc=1
>> +fi
>> +
>> +if [ "$UNSTABLE" ]; then
>> +if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" RECHECK=yes \
>> +run_tests; then
>> +unstable_rc=1
>> +fi
>> +fi
>> +
>> +if [[ $stable_rc -ne 0 ]] || [[ $unstable_rc -ne 0 ]]; then
>> exit 1
>> fi
>> }
>>
>> +function run_system_tests()
>> +{
>> +local type=$1
>> +local log_file=$2
>> +
>> +if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE" \
>> +RECHECK=$RECHECK; then
>> +# $log_file is necessary for debugging.
>> +cat tests/$log_file
>> +return 1
>> +fi
>> +}
>> +
>> function execute_system_tests()
>> {
>> - type=$1
>> - log_file=$2
>> -
>> - configure_ovn $OPTS
>> - make $JOBS || { cat config.log; exit 1; }
>> - if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE"
>> RECHECK=yes; then
>> - # $log_file is necessary for debugging.
>> - cat tests/$log_file
>> - exit 1
>> - fi
>> +configure_ovn $OPTS
>> +make $JOBS || { cat config.log; exit 1; }
>> +
>> +local stable_rc=0
>> +local unstable_rc=0
>> +
>> +if ! SKIP_UNSTABLE=yes run_system_tests $@; then
>> +stable_rc=1
>> +fi
>> +
>> +if [ "$UNSTABLE" ]; then
>> +if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" RECHECK=yes \
>> +run_system_tests $@; then
>> +
Re: [ovs-dev] [PATCH v2 3/3] checkpatch: print subject field if misspelled or missing
On 7 Jul 2023, at 22:07, Chandan Somani wrote:
> This narrows down spelling errors that are in the commit
> subject. In v2, it also provides a subject if the subject
> line is missing. The provisional subject is the name of the
> patch file, which should provide some context about the patch.
>
> Signed-off-by: Chandan Somani
Hi Chandan,
Thanks for the patch! The robot found a problem, with a trailing white space.
However I can fix this at commit time, also one small change below. Let me know
if you agree and will fix this up during commit.
In addition, would it be possible to send an additional patch(series) to also
do this on git-based patches, i.e. if you do something like ‘checkapatch.py -S
-2’?
Maybe there should be an error if the subject is missing, independent if
spellcheck is enabled or not?
Cheers,
Eelco
> ---
> utilities/checkpatch.py | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index e5d5029f2..f34e4bf2a 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -1024,6 +1024,14 @@ def ovs_checkpatch_file(filename):
> result = ovs_checkpatch_parse(part.get_payload(decode=False), filename,
>mail.get('Author', mail['From']),
>mail['Commit'])
> +if spellcheck:
> +if not mail['Subject'].strip():
> +mail.replace_header('Subject', sys.argv[-1])
> +print("Subject missing! Your provisional subject is",
> + mail['Subject'])
If the subject is missing from the patch it would error out.
What about the following:
+if spellcheck:
+if not mail['Subject'] or not mail['Subject'].strip():
+if mail['Subject']:
+mail.replace_header('Subject', sys.argv[-1])
+else:
+mail.add_header('Subject', sys.argv[-1])
+
+print("Subject missing! Your provisional subject is",
+ mail['Subject'])
+
+if check_spelling(mail['Subject'], False):
+print("Subject: %s" % mail['Subject'])
+
> +if check_spelling(mail['Subject'], False):
> +print("Subject: %s" % mail['Subject'])
> +
> ovs_checkpatch_print_result()
> return result
>
> --
> 2.26.3
>
> ___
> 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/3] checkpatch: add suggestions to the spell checker
On 7 Jul 2023, at 22:07, Chandan Somani wrote: > This will be useful for correcting possible spelling > mistakes with ease. Suggestions limited to 3 at first, > but configurable in the future. > > Signed-off-by: Chandan Somani > Acked-by: Aaron Conole > --- This looks good, thank Chandan. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/3] checkpatch: reorganize flagged words using a list
On 7 Jul 2023, at 22:07, Chandan Somani wrote:
> Single out flagged words and allow for more useful
> details, like spelling suggestions. Fixed syntax
> error from v1
>
> Signed-off-by: Chandan Somani
Hi Chandan,
Thanks for the patch! I think it looks good, however some small comments on the
message format.
The version information should not be part of your commit message but should be
moved down (see example below). In addition, your patch subject should start
with a capital and end with a dot:
‘checkpatch: Reorganize flagged words using a list.’
Don’t worry about it for this series. I will fix it on commit.
Acked-by: Eelco Chaudron
> ---
v2: Fixed syntax error in length comparison.
> utilities/checkpatch.py | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 64f0efeb4..acf9a0102 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -411,6 +411,8 @@ def check_spelling(line, comment):
> words = filter_comments(line, True) if comment else line
> words = words.replace(':', ' ').split(' ')
>
> +flagged_words = []
> +
> for word in words:
> skip = False
> strword = re.subn(r'\W+', '', word)[0].replace(',', '')
> @@ -435,9 +437,13 @@ def check_spelling(line, comment):
> skip = True
>
> if not skip:
> -print_warning("Check for spelling mistakes (e.g. \"%s\")"
> - % strword)
> -return True
> +flagged_words.append(strword)
> +
> +if len(flagged_words) > 0:
> +for mistake in flagged_words:
> +print_warning("Possible misspelled word: \"%s\"" % mistake)
> +
> +return True
>
> return False
>
> --
> 2.26.3
>
> ___
> 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 v1 2/7] netdev-linux: use speed as max rate in tc classes
On 7/10/23 16:33, Eelco Chaudron wrote:
On 10 Jul 2023, at 16:26, Adrian Moreno wrote:
On 7/10/23 16:19, Adrian Moreno wrote:
On 7/6/23 14:50, Eelco Chaudron wrote:
On 2 Jun 2023, at 16:13, Adrian Moreno wrote:
Instead of relying on feature bits, use the speed value directly as
maximum rate for htb and hfsc classes.
There is still a limitation with the maximum rate that we can express
with a 32-bit number in bytes/s (~ 34.3Gbps), but using the actual link speed
instead of the feature bits, we can at least use an accurate maximum for
some link speeds (such as 25Gbps) which are not supported by netdev's feature
bits.
Hi Adrian,
See some comments below.
//Eelco
Signed-off-by: Adrian Moreno
---
lib/netdev-linux.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 3055f88d2..56b487eea 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -4746,11 +4746,10 @@ htb_parse_qdisc_details__(struct netdev *netdev_,
hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8;
if (!hc->max_rate) {
- enum netdev_features current;
-
netdev_linux_read_features(netdev);
- current = !netdev->get_features_error ? netdev->current : 0;
- hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8;
Re-reading my previous comments on fallback for to netdev_get_features() I
guess the new API should replace this one.
So to make a statement I would say try to remove the netdev_features_to_bps()
API, or move it to netdev_bsd_features_to_bps() for netdev_bsd_get_speed only
(with a comment) so it’s clear that people should NOT use this API from now on.
We are still not deprecating the old API (netdev_get_features() and "enum
netdev_features") since it is in line with OFP1.0 that does not have numeric speeds.
e.g: lib/ofp-port uses netdev_get_features() to convert the maximum speed to kbps when
decoding OFP1.0 port descriptions.
What I do think is a good idea is to add a comment pointing new users to the
new API if what they are looking for is a more accurate speed. WDYT?
Or maybe we could even rename it and add "legacy" in the name so that it's more
clear that it should not be used (for those who don't read the comments :-)).
What about a check patch warning when the API is used?
OK. I'll look into checkpatch.py. Thanks.
--
Adrián Moreno
--
Adrián Moreno
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] ovs-vsctl: Exit with error if postdb checks report errors.
On 5 Jul 2023, at 1:29, Flavio Leitner wrote: > Today the exit code refers to the execution of the change > in the database. However, when not using parameter --no-wait > (default), the ovs-vsctl also checks if OVSDB transactions > are successfully recorded and reload by ovs-vswitchd. In this > case, an error message is printed if there is a problem during > the reload, like for example the one below: > > # ovs-vsctl add-port br0 gre0 -- \ > set interface gre0 type=ip6gre options:key=100 \ > options:remote_ip=2001::2 > ovs-vsctl: Error detected while setting up 'gre0': could not \ > add network device gre0 to ofproto (Address family not supported\ > by protocol). See ovs-vswitchd log for details. > ovs-vsctl: The default log directory is "/var/log/openvswitch". > # echo $? > 0 > > This patch changes to exit with specific error code 160 > (ERROR_BAD_ARGUMENTS) on Windows and 65 (EX_DATAERR) on > Linux or BSD if errors were reported during the reload. > > This change may break existing scripts because ovs-vsctl will > start to fail when before it was succeeding. However, if an > error is printed, then it is likely that the change was not > functional anyway. > > Reported-at: https://bugzilla.redhat.com/1731553 > Signed-off-by: Flavio Leitner Thanks Flavio for the patch. I verified the Windows part trough AppVeyor. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
