[ovs-dev] OVN Request to update OVS submodule

[Abhiram R N]

Hi Team,

Recently a fix has gone into OVS with the commit ID as below
commit feed7f6775056b3dd55249596a7e587bc9c5fd4a
Author: Mike Pattrick 
Date:   Wed Jul 19 12:21:09 2023 -0400

https://github.com/openvswitch/ovs/commit/feed7f6775056b3dd55249596a7e587bc9c5fd4a

This we had reported while trying to add support for OVN port mirroring in
'both' directions. With the above OVS commit, we can now add necessary
support in OVN to extend port mirroring in 'both' directions. But for that
OVS dependency is there on the above fix. Hence, request you to consider
updating the submodule.

Thanks & Regards,
Abhiram R N
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVN Request to update OVS submodule

[Ales Musil]

On Thu, Aug 3, 2023 at 9:37 AM Abhiram R N  wrote:

> Hi Team,
>
> Recently a fix has gone into OVS with the commit ID as below
> commit feed7f6775056b3dd55249596a7e587bc9c5fd4a
> Author: Mike Pattrick 
> Date:   Wed Jul 19 12:21:09 2023 -0400
>
>
> https://github.com/openvswitch/ovs/commit/feed7f6775056b3dd55249596a7e587bc9c5fd4a
>
> This we had reported while trying to add support for OVN port mirroring in
> 'both' directions. With the above OVS commit, we can now add necessary
> support in OVN to extend port mirroring in 'both' directions. But for that
> OVS dependency is there on the above fix. Hence, request you to consider
> updating the submodule.
>
> Thanks & Regards,
> Abhiram R N
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Hi,

the submodule update is planned to be on tag v3.2.0 which doesn't exist yet
(it should include the mentioned patch).
Once the tag is available I'll post a bumping patch for main and other
relevant branches.

Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] [PATCH v2 ovn 0/2] sync lb applied to logical routers in sb db lb table

[Ales Musil]

On Fri, Jul 14, 2023 at 5:25 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:

> Changes since v1:
> - rebase on top of ovn main branch
> - cosmetics
> Changes since RFC v2:
> - introduce ls_datapath_group column and deprecate datapath_group one.
> Changes since RFC v1:
> - get rid of patch 1/2: northd: rename table datapath_group in
>   ls_datapath_group in load_balancer sb db table
>
> Lorenzo Bianconi (2):
>   northd: sync lb applied to logical routers in sb db lb table
>   northd: introduce ls_datapath_group column in lb sb db table
>
>  controller/chassis.c|   8 +++
>  controller/lflow.c  |  10 +++
>  controller/local_data.c |  16 +
>  controller/ovn-controller.c |  13 
>  include/ovn/features.h  |   1 +
>  lib/lb.h|   3 +-
>  northd/northd.c | 108 ++---
>  northd/northd.h |   1 +
>  ovn-sb.ovsschema|  10 ++-
>  ovn-sb.xml  |  12 
>  tests/ovn-controller.at |   4 ++
>  tests/ovn-northd.at |  31 +++---
>  tests/system-ovn.at | 117 
>  utilities/ovn-sbctl.c   |  13 
>  14 files changed, 316 insertions(+), 31 deletions(-)
>
> --
> 2.41.0
>
>
Looks good to me, thanks.

Reviewed-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] OVN Request to update OVS submodule

[Abhiram R N]

Sure Ales.
Thanks for the update

Regards,
Abhiram R N

On Thu, Aug 3, 2023 at 1:14 PM Ales Musil  wrote:

>
>
> On Thu, Aug 3, 2023 at 9:37 AM Abhiram R N  wrote:
>
>> Hi Team,
>>
>> Recently a fix has gone into OVS with the commit ID as below
>> commit feed7f6775056b3dd55249596a7e587bc9c5fd4a
>> Author: Mike Pattrick 
>> Date:   Wed Jul 19 12:21:09 2023 -0400
>>
>>
>> https://github.com/openvswitch/ovs/commit/feed7f6775056b3dd55249596a7e587bc9c5fd4a
>>
>> This we had reported while trying to add support for OVN port mirroring in
>> 'both' directions. With the above OVS commit, we can now add necessary
>> support in OVN to extend port mirroring in 'both' directions. But for that
>> OVS dependency is there on the above fix. Hence, request you to consider
>> updating the submodule.
>>
>> Thanks & Regards,
>> Abhiram R N
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Hi,
>
> the submodule update is planned to be on tag v3.2.0 which doesn't exist
> yet (it should include the mentioned patch).
> Once the tag is available I'll post a bumping patch for main and other
> relevant branches.
>
> Thanks,
> Ales
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA 
>
> amu...@redhat.comIM: amusil
> 
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] QoS: Properly set qos when ovs db is read only

[Ales Musil]

On Wed, Jul 26, 2023 at 3:44 PM Xavier Simonart  wrote:

> QoS was not configured in OVS db when db was read only: the configuration
> was just ignored and not done later when OVS db became writable.
> It was sometimes set later, if/when a recompute happened.
> This is now fixed: when OVS db is read only, the ports on which qos
> must be applied and stored and qos will be applied when OVS db becomes
> writable.
> To avoid race conditions between delayed qos and new qos changes (e.g. a
> qos
> configuration delayed in one loop as ovs is ro, followed in next loop,
> when ovs
> becomes rw, by another qos on the same port), all qos changes are done at
> the
> same time.
>
> This issue was identified by some random failures in system test
> "egress qos".
>
> Signed-off-by: Xavier Simonart 
>

Hi Xavier,
I have two small comments below.

---
>  controller/binding.c| 131 
>  controller/binding.h|   8 +++
>  controller/ovn-controller.c |   7 ++
>  tests/ovn.at|   1 -
>  tests/system-ovn.at |  22 ++
>  5 files changed, 124 insertions(+), 45 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 9aa3fc6c4..0e13624c1 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -55,8 +55,13 @@ struct claimed_port {
>  long long int last_claimed;
>  };
>
> +struct qos_port {
> +bool added;
> +};
> +
>  static struct shash _claimed_ports = SHASH_INITIALIZER(&_claimed_ports);
>  static struct sset _postponed_ports = SSET_INITIALIZER(&_postponed_ports);
> +static struct shash _qos_ports = SHASH_INITIALIZER(&_qos_ports);
>
>  static void
>  remove_additional_chassis(const struct sbrec_port_binding *pb,
> @@ -218,6 +223,17 @@ get_qos_egress_port_interface(struct shash
> *bridge_mappings,
>  return NULL;
>  }
>
> +static void
> +add_or_del_qos_port(const char *ovn_port, bool add)
> +{
> +struct qos_port *qos_port = shash_find_data(&_qos_ports, ovn_port);
> +if (!qos_port) {
> +qos_port = xzalloc(sizeof *qos_port);
> +shash_add(&_qos_ports, ovn_port, qos_port);
> +}
> +qos_port->added = add;
> +}
> +
>  /* 34359738360 == (2^32 - 1) * 8.  netdev_set_qos() doesn't support
>   * 64-bit rate netlink attributes, so the maximum value is 2^32 - 1
>   * bytes. The 'max-rate' config option is in bits, so multiplying by 8.
> @@ -225,7 +241,7 @@ get_qos_egress_port_interface(struct shash
> *bridge_mappings,
>   * can be unrecognized for certain NICs or reported too low for virtual
>   * interfaces. */
>  #define OVN_QOS_MAX_RATE34359738360ULL
> -static void
> +static bool
>  add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn,
>  const struct ovsrec_port *port,
>  unsigned long long min_rate,
> @@ -239,7 +255,7 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn
> *ovs_idl_txn,
>  const struct ovsrec_qos *qos = port->qos;
>  if (qos && !smap_get_bool(&qos->external_ids, "ovn_qos", false)) {
>  /* External configured QoS, do not overwrite it. */
> -return;
> +return false;
>  }
>
>  if (!qos) {
> @@ -282,22 +298,18 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn
> *ovs_idl_txn,
>  ovsrec_queue_verify_external_ids(queue);
>  ovsrec_queue_set_external_ids(queue, &external_ids);
>  smap_destroy(&external_ids);
> +return true;
>  }
>
>  static void
> -remove_stale_qos_entry(struct ovsdb_idl_txn *ovs_idl_txn,
> -   const struct sbrec_port_binding *pb,
> +remove_stale_qos_entry( const char *logical_port,
> struct ovsdb_idl_index *ovsrec_port_by_qos,
> const struct ovsrec_qos_table *qos_table,
> struct hmap *queue_map)
>  {
> -if (!ovs_idl_txn) {
> -return;
> -}
> -
>  struct qos_queue *q = find_qos_queue(
> -queue_map, hash_string(pb->logical_port, 0),
> -pb->logical_port);
> +queue_map, hash_string(logical_port, 0),
> +logical_port);
>  if (!q) {
>  return;
>  }
> @@ -338,8 +350,12 @@ remove_stale_qos_entry(struct ovsdb_idl_txn
> *ovs_idl_txn,
>
>  static void
>  configure_qos(const struct sbrec_port_binding *pb,
> -  struct binding_ctx_in *b_ctx_in,
> -  struct binding_ctx_out *b_ctx_out)
> +  struct ovsdb_idl_txn *ovs_idl_txn,
> +  struct ovsdb_idl_index *ovsrec_port_by_qos,
> +  const struct ovsrec_qos_table *qos_table,
> +  struct hmap *qos_map,
> +  const struct ovsrec_open_vswitch_table *ovs_table,
> +  const struct ovsrec_bridge_table *bridge_table)
>  {
>  unsigned long long min_rate = smap_get_ullong(
>  &pb->options, "qos_min_rate", 0);
> @@ -351,20 +367,20 @@ configure_qos(const struct sbrec_port_binding *pb,
>
>  if ((!min_rate && !max_rate && !burst) || !queue_id) {
>  /

Re: [ovs-dev] [PATCH ovn v2] binding: handle ovs ofport update

[Ales Musil]

On Mon, Jul 31, 2023 at 7:18 PM Mohammad Heib  wrote:

> Currently when ovs interface ofport is updated
> after setting external_ids:iface_id, the ovn-controller
> will see this change but will not do much if it handles
> this change incrementally.
>
> This behavior leads to a mismatch between the ovs openflow
> flows in table=0 (inaccurate in_port) and the ofport number
> that the packet was received at which will lead to packets
> drop in table=0.
>
> This patch will resolve the above issue by triggering
> flows recompute during the I-P processing only if the
> affected port are associated with lport and has flows
> that need to be updated.
>
> Reported-at: https://issues.redhat.com/browse/FD-3063
> Signed-off-by: Mohammad Heib 
> ---
>  controller/binding.c| 15 ++
>  tests/ovn-controller.at | 45 +
>  2 files changed, 60 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 9aa3fc6c4..cc4c2b0bb 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct ovsrec_interface
> *iface_rec,
>  /* Get the (updated) b_lport again for the lbinding. */
>  b_lport = local_binding_get_primary_lport(lbinding);
>
> +/*
> + * Update the tracked_dp_bindings whenever an ofport
> + * on a specific ovs port changes.
> + * This update will trigger flow recomputation during
> + * the incremental processing run which updates the local
> + * flows in_port filed.
> + */
> +if (b_lport && ovsrec_interface_is_updated(iface_rec,
> +OVSREC_INTERFACE_COL_OFPORT)) {
> +tracked_datapath_lport_add(b_lport->pb, TRACKED_RESOURCE_UPDATED,
> +   b_ctx_out->tracked_dp_bindings);
> +b_ctx_out->local_lports_changed = true;
> +}
> +
> +
>  /* Update the child local_binding's iface (if any children) and try to
>   *  claim the container lbindings. */
>  LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index e1b6491b3..f2216d245 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2606,3 +2606,48 @@ AT_CHECK([ovn-sbctl get chassis $chassis_id
> other_config:unsupported], [1], [ign
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - ovs iface change ofport])
> +AT_KEYWORDS([ovn])
> +ovn_start
> +
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> +set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> +options:tx_pcap=hv1/vif1-tx.pcap \
> +options:rxq_pcap=hv1/vif1-rx.pcap \
> +ofport-request=1
> +
> +
> +ovn-nbctl ls-add sw0
> +
> +check ovn-nbctl lsp-add sw0 sw0-p1
> +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3
> 1000::3"
> +
> +wait_for_ports_up
> +ovn-nbctl --wait=hv sync
> +
> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c
> in_port=1], [0],[dnl
> +1
> +])
> +
> +# update the ovs interface ofport from 1 to 24
> +check as hv1 ovs-vsctl set Interface hv1-vif1 ofport-request=24
> +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface hv1-vif1 ofport` =
> x24])
> +
> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c
> in_port=24], [0],[dnl
> +1
> +])
> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c
> in_port=1], [1],[dnl
> +0
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> --
> 2.34.3
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] [PATCH v3 net-next 0/5] selftests: openvswitch: add flow programming cases

[Simon Horman]

On Tue, Aug 01, 2023 at 05:22:21PM -0400, Aaron Conole wrote:
> The openvswitch selftests currently contain a few cases for managing the
> datapath, which includes creating datapath instances, adding interfaces,
> and doing some basic feature / upcall tests.  This is useful to validate
> the control path.
> 
> Add the ability to program some of the more common flows with actions. This
> can be improved overtime to include regression testing, etc.

Thanks Aaron.

For series,

Reviewed-by: Simon Horman 

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

Re: [ovs-dev] [PATCH ovn v3] Expose distributed gateway port information in NB DB

[Ales Musil]

On Tue, Aug 1, 2023 at 4:11 PM  wrote:

> From: Lucas Alvares Gomes 
>
> In order for the CMS to know which Chassis a distributed gateway port
> is bond to, this patch updates the ovn-northd daemon to populate the
> Logical_Router_Port table with that information.
>
> To avoid changing the database schema, ovn-northd is setting a new key
> called "hosting-chassis" in the options column from the LRP table. This
> key value points to the name of the Chassis that is currently hosting
> the distributed port.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
> Signed-off-by: Lucas Alvares Gomes 
>

Hi Lucas,

I have a few comments down below.


> ---
> v2 -> v3
> * Fixed memory leak
> * Separate the logic to update the router port into their own function
> * Use attribute l3dgw_port to find the GW port
>
> v1 -> v2
> * Rebased on the main branch
> * Updated the ovnsb_db_run() and handle_port_binding_changes() functions
>   to include the LR ports as a parameter
>
>  northd/en-sync-from-sb.c |  2 +-
>  northd/northd.c  | 33 +++--
>  northd/northd.h  |  3 ++-
>  ovn-nb.xml   | 15 +++
>  tests/ovn-northd.at  | 34 ++
>  5 files changed, 83 insertions(+), 4 deletions(-)
>
> diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
> index 55ece2d16..4109aebe4 100644
> --- a/northd/en-sync-from-sb.c
> +++ b/northd/en-sync-from-sb.c
> @@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data
> OVS_UNUSED)
>  stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>  ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
>   sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
> - &nd->ls_ports);
> + &nd->ls_ports, &nd->lr_ports);
>  stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>  }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index b9605862e..b2a59ab3a 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -17780,6 +17780,25 @@ build_ha_chassis_group_ref_chassis(struct
> ovsdb_idl_index *ha_ch_grp_by_name,
>  }
>  }
>
> +/* Set the "hosting-chassis" option in the NBDB logical_router_port
> + * table indicating which chassis the distributed port is bond to. */
> +static void
> +handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
> +struct ovn_port *orp)
> +{
> +struct smap options;
> +smap_clone(&options, &orp->l3dgw_port->nbrp->options);
> +if (sb->chassis) {
> +smap_replace(&options, "hosting-chassis",
> +sb->chassis->name);
> +} else {
> +smap_remove(&options, "hosting-chassis");
> +}
> +nbrec_logical_router_port_set_options(orp->l3dgw_port->nbrp,
> +&options);
> +smap_destroy(&options);
> +}
>

We can avoid the expensive smap_clone whatsoever by replacing the
smap_replace/smap_remove
with direct operation on the DB row:
nbrec_logical_router_port_update_options_setkey and
nbrec_logical_router_port_update_options_delkey


> +
>  /* Handle changes to the 'chassis' column of the 'Port_Binding' table.
> When
>   * this column is not empty, it means we need to set the corresponding
> logical
>   * port as 'up' in the northbound DB. */
> @@ -17789,6 +17808,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
>  const struct sbrec_ha_chassis_group_table
> *sb_ha_ch_grp_table,
>  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>  struct hmap *ls_ports,
> +struct hmap *lr_ports,
>  struct shash *ha_ref_chassis_map)
>  {
>  const struct sbrec_port_binding *sb;
> @@ -17807,6 +17827,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
>  }
>
>  SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
> +
> +struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port);
> +
> +if (orp && is_cr_port(orp)) {
> +handle_cr_port_binding_changes(sb, orp);
> +continue;
> +}
> +
>  struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);
>
>  if (!op || !op->nbsp) {
> @@ -17855,7 +17883,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>   const struct sbrec_port_binding_table *sb_pb_table,
>   const struct sbrec_ha_chassis_group_table
> *sb_ha_ch_grp_table,
>   struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> - struct hmap *ls_ports)
> + struct hmap *ls_ports,
> + struct hmap *lr_ports)
>  {
>  if (!ovnnb_txn ||
>  !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
> @@ -17864,7 +17893,7 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>
>  struct shash ha_ref_chassis_map =
> SHASH_INITIALIZER(&ha_ref_chassis_map);
>  handle_port_binding_changes(ovnsb_txn, sb_

Re: [ovs-dev] [PATCH ovn 0/4] Multiple simple fixes to unit tests

[Ales Musil]

On Thu, Aug 3, 2023 at 8:59 AM Xavier Simonart  wrote:

> Xavier Simonart (4):
>   tests: fixed "Logical flows with Chassis_Template_Var reference"
>   tests: fixed multiple ovn-ic unit tests
>   tests: fixed "CT flush load balancer backends"
>   tests: fixed "check fip and lb flows"
>
>  tests/ovn-ic.at | 10 +++---
>  tests/ovn-northd.at |  2 +-
>  tests/ovn.at|  7 +--
>  3 files changed, 13 insertions(+), 6 deletions(-)
>
> --
> 2.31.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
The whole series looks good to me, thanks!

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] [PATCH v3 2/4] dpif: Probe support for OVS_ACTION_ATTR_DROP.

[Ilya Maximets]

On 7/28/23 02:04, Eric Garver wrote:
> Kernel support is being added for this action. As such, we need to probe
> the datapath for support.
> 
> Signed-off-by: Eric Garver 

Hi, Eric.  Thanks for the update.

IIRC, there was a discussion that this feature has to be mutually exclusive
with the TC hardware offload.  Otherwise, we will constantly treat flows
dumped from TC as invalid.

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

Re: [ovs-dev] [PATCH ovn v3] Expose distributed gateway port information in NB DB

[Lucas Martins]

Hi Ales,

Thank you for the insightful review!

On Thu, Aug 3, 2023 at 11:25 AM Ales Musil  wrote:
>
>
>
> On Tue, Aug 1, 2023 at 4:11 PM  wrote:
>>
>> From: Lucas Alvares Gomes 
>>
>> In order for the CMS to know which Chassis a distributed gateway port
>> is bond to, this patch updates the ovn-northd daemon to populate the
>> Logical_Router_Port table with that information.
>>
>> To avoid changing the database schema, ovn-northd is setting a new key
>> called "hosting-chassis" in the options column from the LRP table. This
>> key value points to the name of the Chassis that is currently hosting
>> the distributed port.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
>> Signed-off-by: Lucas Alvares Gomes 
>
>
> Hi Lucas,
>
> I have a few comments down below.
>
>>
>> ---
>> v2 -> v3
>> * Fixed memory leak
>> * Separate the logic to update the router port into their own function
>> * Use attribute l3dgw_port to find the GW port
>>
>> v1 -> v2
>> * Rebased on the main branch
>> * Updated the ovnsb_db_run() and handle_port_binding_changes() functions
>>   to include the LR ports as a parameter
>>
>>  northd/en-sync-from-sb.c |  2 +-
>>  northd/northd.c  | 33 +++--
>>  northd/northd.h  |  3 ++-
>>  ovn-nb.xml   | 15 +++
>>  tests/ovn-northd.at  | 34 ++
>>  5 files changed, 83 insertions(+), 4 deletions(-)
>>
>> diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
>> index 55ece2d16..4109aebe4 100644
>> --- a/northd/en-sync-from-sb.c
>> +++ b/northd/en-sync-from-sb.c
>> @@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data 
>> OVS_UNUSED)
>>  stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>>  ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
>>   sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
>> - &nd->ls_ports);
>> + &nd->ls_ports, &nd->lr_ports);
>>  stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>>  }
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index b9605862e..b2a59ab3a 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -17780,6 +17780,25 @@ build_ha_chassis_group_ref_chassis(struct 
>> ovsdb_idl_index *ha_ch_grp_by_name,
>>  }
>>  }
>>
>> +/* Set the "hosting-chassis" option in the NBDB logical_router_port
>> + * table indicating which chassis the distributed port is bond to. */
>> +static void
>> +handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
>> +struct ovn_port *orp)
>> +{
>> +struct smap options;
>> +smap_clone(&options, &orp->l3dgw_port->nbrp->options);
>> +if (sb->chassis) {
>> +smap_replace(&options, "hosting-chassis",
>> +sb->chassis->name);
>> +} else {
>> +smap_remove(&options, "hosting-chassis");
>> +}
>> +nbrec_logical_router_port_set_options(orp->l3dgw_port->nbrp,
>> +&options);
>> +smap_destroy(&options);
>> +}
>
>
> We can avoid the expensive smap_clone whatsoever by replacing the 
> smap_replace/smap_remove
> with direct operation on the DB row:
> nbrec_logical_router_port_update_options_setkey and 
> nbrec_logical_router_port_update_options_delkey
>

Great point, code will get even more simpler with the suggestion.

>>
>> +
>>  /* Handle changes to the 'chassis' column of the 'Port_Binding' table.  When
>>   * this column is not empty, it means we need to set the corresponding 
>> logical
>>   * port as 'up' in the northbound DB. */
>> @@ -17789,6 +17808,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
>> *ovnsb_txn,
>>  const struct sbrec_ha_chassis_group_table 
>> *sb_ha_ch_grp_table,
>>  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>>  struct hmap *ls_ports,
>> +struct hmap *lr_ports,
>>  struct shash *ha_ref_chassis_map)
>>  {
>>  const struct sbrec_port_binding *sb;
>> @@ -17807,6 +17827,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
>> *ovnsb_txn,
>>  }
>>
>>  SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
>> +
>> +struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port);
>> +
>> +if (orp && is_cr_port(orp)) {
>> +handle_cr_port_binding_changes(sb, orp);
>> +continue;
>> +}
>> +
>>  struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);
>>
>>  if (!op || !op->nbsp) {
>> @@ -17855,7 +17883,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>>   const struct sbrec_port_binding_table *sb_pb_table,
>>   const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
>>   struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>> - struct hmap *ls_ports)
>> + struct hmap *ls_ports,
>> + struct hmap *lr_ports)

[ovs-dev] [PATCH ovn v4] Expose distributed gateway port information in NB DB

[lmartins]

From: Lucas Alvares Gomes 

In order for the CMS to know which Chassis a distributed gateway port
is bond to, this patch updates the ovn-northd daemon to populate the
Logical_Router_Port table with that information.

To avoid changing the database schema, ovn-northd is setting a new key
called "hosting-chassis" in the options column from the LRP table. This
key value points to the name of the Chassis that is currently hosting
the distributed port.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
Signed-off-by: Lucas Alvares Gomes 
---
v3 -> v4
* Removed smap_clone for updating the options and replaced it with
  smap_replace/smap_remove
* Updated the test to also assert the removal of the hosting-chassis
  key by northd

v2 -> v3
* Fixed memory leak
* Separate the logic to update the router port into their own function
* Use attribute l3dgw_port to find the GW port

v1 -> v2
* Rebased on the main branch
* Updated the ovnsb_db_run() and handle_port_binding_changes() functions
  to include the LR ports as a parameter

 northd/en-sync-from-sb.c |  2 +-
 northd/northd.c  | 30 +--
 northd/northd.h  |  3 ++-
 ovn-nb.xml   | 15 ++
 tests/ovn-northd.at  | 44 
 5 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
index 55ece2d16..4109aebe4 100644
--- a/northd/en-sync-from-sb.c
+++ b/northd/en-sync-from-sb.c
@@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data 
OVS_UNUSED)
 stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
 ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
  sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
- &nd->ls_ports);
+ &nd->ls_ports, &nd->lr_ports);
 stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
 }

diff --git a/northd/northd.c b/northd/northd.c
index b9605862e..c6752884c 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -17780,6 +17780,22 @@ build_ha_chassis_group_ref_chassis(struct 
ovsdb_idl_index *ha_ch_grp_by_name,
 }
 }

+/* Set the "hosting-chassis" option in the NBDB logical_router_port
+ * table indicating which chassis the distributed port is bond to. */
+static void
+handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
+struct ovn_port *orp)
+{
+if (sb->chassis) {
+nbrec_logical_router_port_update_options_setkey(
+orp->l3dgw_port->nbrp, "hosting-chassis",
+sb->chassis->name);
+} else {
+nbrec_logical_router_port_update_options_delkey(
+orp->l3dgw_port->nbrp, "hosting-chassis");
+}
+}
+
 /* Handle changes to the 'chassis' column of the 'Port_Binding' table.  When
  * this column is not empty, it means we need to set the corresponding logical
  * port as 'up' in the northbound DB. */
@@ -17789,6 +17805,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
*ovnsb_txn,
 const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
 struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
 struct hmap *ls_ports,
+struct hmap *lr_ports,
 struct shash *ha_ref_chassis_map)
 {
 const struct sbrec_port_binding *sb;
@@ -17807,6 +17824,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
*ovnsb_txn,
 }

 SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
+
+struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port);
+
+if (orp && is_cr_port(orp)) {
+handle_cr_port_binding_changes(sb, orp);
+continue;
+}
+
 struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);

 if (!op || !op->nbsp) {
@@ -17855,7 +17880,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
  const struct sbrec_port_binding_table *sb_pb_table,
  const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
- struct hmap *ls_ports)
+ struct hmap *ls_ports,
+ struct hmap *lr_ports)
 {
 if (!ovnnb_txn ||
 !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
@@ -17864,7 +17890,7 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,

 struct shash ha_ref_chassis_map = SHASH_INITIALIZER(&ha_ref_chassis_map);
 handle_port_binding_changes(ovnsb_txn, sb_pb_table, sb_ha_ch_grp_table,
-sb_ha_ch_grp_by_name, ls_ports,
+sb_ha_ch_grp_by_name, ls_ports, lr_ports,
 &ha_ref_chassis_map);
 if (ovnsb_txn) {
 update_sb_ha_group_ref_chassis(sb_ha_ch_grp_table,
diff --git a/northd/northd.h b/northd/northd.h
index f3e63b1e1..44dc11009 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -326,7 +326,8 @@ void ovnsb_db_run(struct ov

Re: [ovs-dev] [PATCH ovn v4] Expose distributed gateway port information in NB DB

[Ales Musil]

On Thu, Aug 3, 2023 at 2:29 PM  wrote:

> From: Lucas Alvares Gomes 
>
> In order for the CMS to know which Chassis a distributed gateway port
> is bond to, this patch updates the ovn-northd daemon to populate the
> Logical_Router_Port table with that information.
>
> To avoid changing the database schema, ovn-northd is setting a new key
> called "hosting-chassis" in the options column from the LRP table. This
> key value points to the name of the Chassis that is currently hosting
> the distributed port.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
> Signed-off-by: Lucas Alvares Gomes 
>

Hi Lucas,
thank you for the follow up. There is one minor issue with the test see
below.


> ---
> v3 -> v4
> * Removed smap_clone for updating the options and replaced it with
>   smap_replace/smap_remove
> * Updated the test to also assert the removal of the hosting-chassis
>   key by northd
>
> v2 -> v3
> * Fixed memory leak
> * Separate the logic to update the router port into their own function
> * Use attribute l3dgw_port to find the GW port
>
> v1 -> v2
> * Rebased on the main branch
> * Updated the ovnsb_db_run() and handle_port_binding_changes() functions
>   to include the LR ports as a parameter
>
>  northd/en-sync-from-sb.c |  2 +-
>  northd/northd.c  | 30 +--
>  northd/northd.h  |  3 ++-
>  ovn-nb.xml   | 15 ++
>  tests/ovn-northd.at  | 44 
>  5 files changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
> index 55ece2d16..4109aebe4 100644
> --- a/northd/en-sync-from-sb.c
> +++ b/northd/en-sync-from-sb.c
> @@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data
> OVS_UNUSED)
>  stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>  ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
>   sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
> - &nd->ls_ports);
> + &nd->ls_ports, &nd->lr_ports);
>  stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>  }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index b9605862e..c6752884c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -17780,6 +17780,22 @@ build_ha_chassis_group_ref_chassis(struct
> ovsdb_idl_index *ha_ch_grp_by_name,
>  }
>  }
>
> +/* Set the "hosting-chassis" option in the NBDB logical_router_port
> + * table indicating which chassis the distributed port is bond to. */
> +static void
> +handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
> +struct ovn_port *orp)
> +{
> +if (sb->chassis) {
> +nbrec_logical_router_port_update_options_setkey(
> +orp->l3dgw_port->nbrp, "hosting-chassis",
> +sb->chassis->name);
> +} else {
> +nbrec_logical_router_port_update_options_delkey(
> +orp->l3dgw_port->nbrp, "hosting-chassis");
> +}
> +}
> +
>  /* Handle changes to the 'chassis' column of the 'Port_Binding' table.
> When
>   * this column is not empty, it means we need to set the corresponding
> logical
>   * port as 'up' in the northbound DB. */
> @@ -17789,6 +17805,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
>  const struct sbrec_ha_chassis_group_table
> *sb_ha_ch_grp_table,
>  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>  struct hmap *ls_ports,
> +struct hmap *lr_ports,
>  struct shash *ha_ref_chassis_map)
>  {
>  const struct sbrec_port_binding *sb;
> @@ -17807,6 +17824,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
>  }
>
>  SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
> +
> +struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port);
> +
> +if (orp && is_cr_port(orp)) {
> +handle_cr_port_binding_changes(sb, orp);
> +continue;
> +}
> +
>  struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);
>
>  if (!op || !op->nbsp) {
> @@ -17855,7 +17880,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>   const struct sbrec_port_binding_table *sb_pb_table,
>   const struct sbrec_ha_chassis_group_table
> *sb_ha_ch_grp_table,
>   struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> - struct hmap *ls_ports)
> + struct hmap *ls_ports,
> + struct hmap *lr_ports)
>  {
>  if (!ovnnb_txn ||
>  !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
> @@ -17864,7 +17890,7 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>
>  struct shash ha_ref_chassis_map =
> SHASH_INITIALIZER(&ha_ref_chassis_map);
>  handle_port_binding_changes(ovnsb_txn, sb_pb_table,
> sb_ha_ch_grp_table,
> -sb_ha_ch_grp_by_name, ls_ports,
> +  

Re: [ovs-dev] [PATCH ovn v4] Expose distributed gateway port information in NB DB

[Lucas Martins]

Hi,

Thanks Ales!

My bad on the tests there, will push a new version fixing it!

On Thu, Aug 3, 2023 at 1:55 PM Ales Musil  wrote:
>
>
>
> On Thu, Aug 3, 2023 at 2:29 PM  wrote:
>>
>> From: Lucas Alvares Gomes 
>>
>> In order for the CMS to know which Chassis a distributed gateway port
>> is bond to, this patch updates the ovn-northd daemon to populate the
>> Logical_Router_Port table with that information.
>>
>> To avoid changing the database schema, ovn-northd is setting a new key
>> called "hosting-chassis" in the options column from the LRP table. This
>> key value points to the name of the Chassis that is currently hosting
>> the distributed port.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
>> Signed-off-by: Lucas Alvares Gomes 
>
>
> Hi Lucas,
> thank you for the follow up. There is one minor issue with the test see below.
>
>>
>> ---
>> v3 -> v4
>> * Removed smap_clone for updating the options and replaced it with
>>   smap_replace/smap_remove
>> * Updated the test to also assert the removal of the hosting-chassis
>>   key by northd
>>
>> v2 -> v3
>> * Fixed memory leak
>> * Separate the logic to update the router port into their own function
>> * Use attribute l3dgw_port to find the GW port
>>
>> v1 -> v2
>> * Rebased on the main branch
>> * Updated the ovnsb_db_run() and handle_port_binding_changes() functions
>>   to include the LR ports as a parameter
>>
>>  northd/en-sync-from-sb.c |  2 +-
>>  northd/northd.c  | 30 +--
>>  northd/northd.h  |  3 ++-
>>  ovn-nb.xml   | 15 ++
>>  tests/ovn-northd.at  | 44 
>>  5 files changed, 90 insertions(+), 4 deletions(-)
>>
>> diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
>> index 55ece2d16..4109aebe4 100644
>> --- a/northd/en-sync-from-sb.c
>> +++ b/northd/en-sync-from-sb.c
>> @@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data 
>> OVS_UNUSED)
>>  stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>>  ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
>>   sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
>> - &nd->ls_ports);
>> + &nd->ls_ports, &nd->lr_ports);
>>  stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>>  }
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index b9605862e..c6752884c 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -17780,6 +17780,22 @@ build_ha_chassis_group_ref_chassis(struct 
>> ovsdb_idl_index *ha_ch_grp_by_name,
>>  }
>>  }
>>
>> +/* Set the "hosting-chassis" option in the NBDB logical_router_port
>> + * table indicating which chassis the distributed port is bond to. */
>> +static void
>> +handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
>> +struct ovn_port *orp)
>> +{
>> +if (sb->chassis) {
>> +nbrec_logical_router_port_update_options_setkey(
>> +orp->l3dgw_port->nbrp, "hosting-chassis",
>> +sb->chassis->name);
>> +} else {
>> +nbrec_logical_router_port_update_options_delkey(
>> +orp->l3dgw_port->nbrp, "hosting-chassis");
>> +}
>> +}
>> +
>>  /* Handle changes to the 'chassis' column of the 'Port_Binding' table.  When
>>   * this column is not empty, it means we need to set the corresponding 
>> logical
>>   * port as 'up' in the northbound DB. */
>> @@ -17789,6 +17805,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
>> *ovnsb_txn,
>>  const struct sbrec_ha_chassis_group_table 
>> *sb_ha_ch_grp_table,
>>  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>>  struct hmap *ls_ports,
>> +struct hmap *lr_ports,
>>  struct shash *ha_ref_chassis_map)
>>  {
>>  const struct sbrec_port_binding *sb;
>> @@ -17807,6 +17824,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
>> *ovnsb_txn,
>>  }
>>
>>  SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
>> +
>> +struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port);
>> +
>> +if (orp && is_cr_port(orp)) {
>> +handle_cr_port_binding_changes(sb, orp);
>> +continue;
>> +}
>> +
>>  struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);
>>
>>  if (!op || !op->nbsp) {
>> @@ -17855,7 +17880,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>>   const struct sbrec_port_binding_table *sb_pb_table,
>>   const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
>>   struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>> - struct hmap *ls_ports)
>> + struct hmap *ls_ports,
>> + struct hmap *lr_ports)
>>  {
>>  if (!ovnnb_txn ||
>>  !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
>> @@ -17864,7 +17890,7 @@ ovnsb_db_run(struct ovsdb_

[ovs-dev] [PATCH ovn v5] Expose distributed gateway port information in NB DB

[lmartins]

From: Lucas Alvares Gomes 

In order for the CMS to know which Chassis a distributed gateway port
is bond to, this patch updates the ovn-northd daemon to populate the
Logical_Router_Port table with that information.

To avoid changing the database schema, ovn-northd is setting a new key
called "hosting-chassis" in the options column from the LRP table. This
key value points to the name of the Chassis that is currently hosting
the distributed port.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
Signed-off-by: Lucas Alvares Gomes 
---
v4 -> v5
* Fix test to remove sleep and assert on the right table

v3 -> v4
* Removed smap_clone for updating the options and replaced it with
  smap_replace/smap_remove
* Updated the test to also assert the removal of the hosting-chassis
  key by northd

v2 -> v3
* Fixed memory leak
* Separate the logic to update the router port into their own function
* Use attribute l3dgw_port to find the GW port

v1 -> v2
* Rebased on the main branch
* Updated the ovnsb_db_run() and handle_port_binding_changes() functions
  to include the LR ports as a parameter

 northd/en-sync-from-sb.c |  2 +-
 northd/northd.c  | 30 +++--
 northd/northd.h  |  3 ++-
 ovn-nb.xml   | 15 +++
 tests/ovn-northd.at  | 41 
 5 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
index 55ece2d16..4109aebe4 100644
--- a/northd/en-sync-from-sb.c
+++ b/northd/en-sync-from-sb.c
@@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data 
OVS_UNUSED)
 stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
 ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
  sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
- &nd->ls_ports);
+ &nd->ls_ports, &nd->lr_ports);
 stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
 }

diff --git a/northd/northd.c b/northd/northd.c
index b9605862e..c6752884c 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -17780,6 +17780,22 @@ build_ha_chassis_group_ref_chassis(struct 
ovsdb_idl_index *ha_ch_grp_by_name,
 }
 }

+/* Set the "hosting-chassis" option in the NBDB logical_router_port
+ * table indicating which chassis the distributed port is bond to. */
+static void
+handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
+struct ovn_port *orp)
+{
+if (sb->chassis) {
+nbrec_logical_router_port_update_options_setkey(
+orp->l3dgw_port->nbrp, "hosting-chassis",
+sb->chassis->name);
+} else {
+nbrec_logical_router_port_update_options_delkey(
+orp->l3dgw_port->nbrp, "hosting-chassis");
+}
+}
+
 /* Handle changes to the 'chassis' column of the 'Port_Binding' table.  When
  * this column is not empty, it means we need to set the corresponding logical
  * port as 'up' in the northbound DB. */
@@ -17789,6 +17805,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
*ovnsb_txn,
 const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
 struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
 struct hmap *ls_ports,
+struct hmap *lr_ports,
 struct shash *ha_ref_chassis_map)
 {
 const struct sbrec_port_binding *sb;
@@ -17807,6 +17824,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
*ovnsb_txn,
 }

 SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
+
+struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port);
+
+if (orp && is_cr_port(orp)) {
+handle_cr_port_binding_changes(sb, orp);
+continue;
+}
+
 struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);

 if (!op || !op->nbsp) {
@@ -17855,7 +17880,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
  const struct sbrec_port_binding_table *sb_pb_table,
  const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
- struct hmap *ls_ports)
+ struct hmap *ls_ports,
+ struct hmap *lr_ports)
 {
 if (!ovnnb_txn ||
 !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
@@ -17864,7 +17890,7 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,

 struct shash ha_ref_chassis_map = SHASH_INITIALIZER(&ha_ref_chassis_map);
 handle_port_binding_changes(ovnsb_txn, sb_pb_table, sb_ha_ch_grp_table,
-sb_ha_ch_grp_by_name, ls_ports,
+sb_ha_ch_grp_by_name, ls_ports, lr_ports,
 &ha_ref_chassis_map);
 if (ovnsb_txn) {
 update_sb_ha_group_ref_chassis(sb_ha_ch_grp_table,
diff --git a/northd/northd.h b/northd/northd.h
index f3e63b1e1..44dc11009 100644
--- a/northd/northd.h

Re: [ovs-dev] [PATCH v3 net-next 0/5] selftests: openvswitch: add flow programming cases

[Paolo Abeni]

On Tue, 2023-08-01 at 17:22 -0400, Aaron Conole wrote:
> The openvswitch selftests currently contain a few cases for managing the
> datapath, which includes creating datapath instances, adding interfaces,
> and doing some basic feature / upcall tests.  This is useful to validate
> the control path.
> 
> Add the ability to program some of the more common flows with actions. This
> can be improved overtime to include regression testing, etc.
> 
> v2->v3:
> 1. Dropped support for ipv6 in nat() case
> 2. Fixed a spelling mistake in 2/5 commit message.
> 
> v1->v2:
> 1. Fix issue when parsing ipv6 in the NAT action
> 2. Fix issue calculating length during ctact parsing
> 3. Fix error message when invalid bridge is passed
> 4. Fold in Adrian's patch to support key masks

FTR, this apparently requires an [un?]fairly recent version of
pyroute2. Perhaps you could explicitly check for a minimum working
version and otherwise bail out (skip) the add-flow tests.

Cheers,

Paolo


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

Re: [ovs-dev] [PATCH v3 net-next 0/5] selftests: openvswitch: add flow programming cases

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni :

On Tue,  1 Aug 2023 17:22:21 -0400 you wrote:
> The openvswitch selftests currently contain a few cases for managing the
> datapath, which includes creating datapath instances, adding interfaces,
> and doing some basic feature / upcall tests.  This is useful to validate
> the control path.
> 
> Add the ability to program some of the more common flows with actions. This
> can be improved overtime to include regression testing, etc.
> 
> [...]

Here is the summary with links:
  - [v3,net-next,1/5] selftests: openvswitch: add an initial flow programming 
case
https://git.kernel.org/netdev/net-next/c/918423fda910
  - [v3,net-next,2/5] selftests: openvswitch: support key masks
https://git.kernel.org/netdev/net-next/c/9f1179fbbd84
  - [v3,net-next,3/5] selftests: openvswitch: add a test for ipv4 forwarding
https://git.kernel.org/netdev/net-next/c/05398aa40953
  - [v3,net-next,4/5] selftests: openvswitch: add basic ct test case parsing
https://git.kernel.org/netdev/net-next/c/2893ba9c1d1a
  - [v3,net-next,5/5] selftests: openvswitch: add ct-nat test case with ipv4
https://git.kernel.org/netdev/net-next/c/60f10077eec6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


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

Re: [ovs-dev] [PATCH ovn v5] Expose distributed gateway port information in NB DB

[Ales Musil]

On Thu, Aug 3, 2023 at 3:15 PM  wrote:

> From: Lucas Alvares Gomes 
>
> In order for the CMS to know which Chassis a distributed gateway port
> is bond to, this patch updates the ovn-northd daemon to populate the
> Logical_Router_Port table with that information.
>
> To avoid changing the database schema, ovn-northd is setting a new key
> called "hosting-chassis" in the options column from the LRP table. This
> key value points to the name of the Chassis that is currently hosting
> the distributed port.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
> Signed-off-by: Lucas Alvares Gomes 
> ---
> v4 -> v5
> * Fix test to remove sleep and assert on the right table
>
> v3 -> v4
> * Removed smap_clone for updating the options and replaced it with
>   smap_replace/smap_remove
> * Updated the test to also assert the removal of the hosting-chassis
>   key by northd
>
> v2 -> v3
> * Fixed memory leak
> * Separate the logic to update the router port into their own function
> * Use attribute l3dgw_port to find the GW port
>
> v1 -> v2
> * Rebased on the main branch
> * Updated the ovnsb_db_run() and handle_port_binding_changes() functions
>   to include the LR ports as a parameter
>
>  northd/en-sync-from-sb.c |  2 +-
>  northd/northd.c  | 30 +++--
>  northd/northd.h  |  3 ++-
>  ovn-nb.xml   | 15 +++
>  tests/ovn-northd.at  | 41 
>  5 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
> index 55ece2d16..4109aebe4 100644
> --- a/northd/en-sync-from-sb.c
> +++ b/northd/en-sync-from-sb.c
> @@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data
> OVS_UNUSED)
>  stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>  ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
>   sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
> - &nd->ls_ports);
> + &nd->ls_ports, &nd->lr_ports);
>  stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>  }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index b9605862e..c6752884c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -17780,6 +17780,22 @@ build_ha_chassis_group_ref_chassis(struct
> ovsdb_idl_index *ha_ch_grp_by_name,
>  }
>  }
>
> +/* Set the "hosting-chassis" option in the NBDB logical_router_port
> + * table indicating which chassis the distributed port is bond to. */
> +static void
> +handle_cr_port_binding_changes(const struct sbrec_port_binding *sb,
> +struct ovn_port *orp)
> +{
> +if (sb->chassis) {
> +nbrec_logical_router_port_update_options_setkey(
> +orp->l3dgw_port->nbrp, "hosting-chassis",
> +sb->chassis->name);
> +} else {
> +nbrec_logical_router_port_update_options_delkey(
> +orp->l3dgw_port->nbrp, "hosting-chassis");
> +}
> +}
> +
>  /* Handle changes to the 'chassis' column of the 'Port_Binding' table.
> When
>   * this column is not empty, it means we need to set the corresponding
> logical
>   * port as 'up' in the northbound DB. */
> @@ -17789,6 +17805,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
>  const struct sbrec_ha_chassis_group_table
> *sb_ha_ch_grp_table,
>  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>  struct hmap *ls_ports,
> +struct hmap *lr_ports,
>  struct shash *ha_ref_chassis_map)
>  {
>  const struct sbrec_port_binding *sb;
> @@ -17807,6 +17824,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
>  }
>
>  SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
> +
> +struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port);
> +
> +if (orp && is_cr_port(orp)) {
> +handle_cr_port_binding_changes(sb, orp);
> +continue;
> +}
> +
>  struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);
>
>  if (!op || !op->nbsp) {
> @@ -17855,7 +17880,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>   const struct sbrec_port_binding_table *sb_pb_table,
>   const struct sbrec_ha_chassis_group_table
> *sb_ha_ch_grp_table,
>   struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> - struct hmap *ls_ports)
> + struct hmap *ls_ports,
> + struct hmap *lr_ports)
>  {
>  if (!ovnnb_txn ||
>  !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
> @@ -17864,7 +17890,7 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>
>  struct shash ha_ref_chassis_map =
> SHASH_INITIALIZER(&ha_ref_chassis_map);
>  handle_port_binding_changes(ovnsb_txn, sb_pb_table,
> sb_ha_ch_grp_table,
> -sb_ha_ch_grp_by_name, ls_ports,
> +s

Re: [ovs-dev] [PATCH v3 2/4] dpif: Probe support for OVS_ACTION_ATTR_DROP.

[Eric Garver]

On Thu, Aug 03, 2023 at 01:43:40PM +0200, Ilya Maximets wrote:
> On 7/28/23 02:04, Eric Garver wrote:
> > Kernel support is being added for this action. As such, we need to probe
> > the datapath for support.
> > 
> > Signed-off-by: Eric Garver 
> 
> Hi, Eric.  Thanks for the update.
> 
> IIRC, there was a discussion that this feature has to be mutually exclusive
> with the TC hardware offload.  Otherwise, we will constantly treat flows
> dumped from TC as invalid.

I think I misunderstood the conversation. I thought it landed on "let's
ignore TC for now", but really it landed on "let's ignore the case of
enabling HW/TC offload at runtime" (since docs say a restart is
required).

I'll work on a v4 then which sets
ofproto->backer->rt_support.explicit_drop_action with regards to TC.

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

[ovs-dev] [PATCH] ci: Fix OPTS not being passed to OSX builds.

[Ilya Maximets]

Before GHA, OPTS were always passed as an argument for a *-build.sh
script.  But that changed.  Linux builds are using the OPTS variable
from the environment, but OSX script does not, so the options are
currently ignored.

That wasn't a big issue until now, because SSL was not available or
the build actually worked on newer branches.  But GHA recently updated
OpenSSl to 3.0+ and we have deprecation warnings on branches that do
not support OpenSSL 3.0+ (branch 2.16) and that breaks the build.

Fixes: 6cb2f5a630e3 ("github: Add GitHub Actions workflow.")
Signed-off-by: Ilya Maximets 
---
 .ci/osx-build.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.ci/osx-build.sh b/.ci/osx-build.sh
index 09df61826..b81744ec9 100755
--- a/.ci/osx-build.sh
+++ b/.ci/osx-build.sh
@@ -10,7 +10,7 @@ function configure_ovs()
 ./boot.sh && ./configure $*
 }
 
-configure_ovs $EXTRA_OPTS $*
+configure_ovs $EXTRA_OPTS $OPTS $*
 
 if [ "$CC" = "clang" ]; then
 make CFLAGS="$CFLAGS -Wno-error=unused-command-line-argument"
-- 
2.40.1

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

Re: [ovs-dev] [PATCH v2 ovn 0/2] sync lb applied to logical routers in sb db lb table

[Ilya Maximets]

On 8/3/23 09:46, Ales Musil wrote:
> 
> 
> On Fri, Jul 14, 2023 at 5:25 PM Lorenzo Bianconi  > wrote:
> 
> Changes since v1:
> - rebase on top of ovn main branch
> - cosmetics
> Changes since RFC v2:
> - introduce ls_datapath_group column and deprecate datapath_group one.
> Changes since RFC v1:
> - get rid of patch 1/2: northd: rename table datapath_group in
>   ls_datapath_group in load_balancer sb db table
> 
> Lorenzo Bianconi (2):
>   northd: sync lb applied to logical routers in sb db lb table
>   northd: introduce ls_datapath_group column in lb sb db table
> 
>  controller/chassis.c        |   8 +++
>  controller/lflow.c          |  10 +++
>  controller/local_data.c     |  16 +
>  controller/ovn-controller.c |  13 
>  include/ovn/features.h      |   1 +
>  lib/lb.h                    |   3 +-
>  northd/northd.c             | 108 ++---
>  northd/northd.h             |   1 +
>  ovn-sb.ovsschema            |  10 ++-
>  ovn-sb.xml                  |  12 
>  tests/ovn-controller.at      |   4 ++
>  tests/ovn-northd.at          |  31 +++---
>  tests/system-ovn.at          | 117 
> 
>  utilities/ovn-sbctl.c       |  13 
>  14 files changed, 316 insertions(+), 31 deletions(-)
> 
> -- 
> 2.41.0
> 
> 
> Looks good to me, thanks.
> 
> Reviewed-by: Ales Musil mailto:amu...@redhat.com>>

I'd guess, this change may have a significant negative performance impact.
Do we have some scale testing numbers for it?  Not saying we shouldn't
fix the issue, but we should probably know what we're going into.

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

Re: [ovs-dev] backporting 8c341b9d704cdf002126699527308203319954f0 to 22.03 ?

[Nikhil Kshirsagar]

Hi Dumitru,

thanks for writing back. Yes, I needed to first switch the ovn branch
and then get the ovs version for that one.

I tried backporting it to 22.09 first, and when I cherry picked
8c341b9d704cdf002126699527308203319954f0 and then 27a92cc272 followed
by a42c808f30 with manual conflict resolution in all, then I was able
to get it to build but it failed several unit tests. (more than 30
iirc).

I also tried backporting it to 22.03.2 and in the right order, or so I
assumed, i.e cherry picked first 27a92cc272, then a42c808f30 and then
8c341b9d704cdf002126699527308203319954f0 but I could not even build it
(perhaps my manual resolves of the conflicts were not correct..) and
seemed like I further needed 4dc4bc7fdb (due to
"controller/ovn-controller.c:1485:39: error: invalid use of undefined
type ‘struct ed_type_postponed_ports’)
and even further, ee20c48c2f, but that also did not help, pasting just
the start of the build errors,

controller/binding.c: In function ‘remove_additional_encap_for_chassis’:
controller/binding.c:1035:30: error: ‘const struct sbrec_port_binding’
has no member named ‘n_additional_encap’
 1035 | for (size_t i = 0; i < pb->n_additional_encap; i++) {
  |  ^~
controller/binding.c:1036:23: error: ‘const struct sbrec_port_binding’
has no member named ‘additional_encap’
 1036 | if (!strcmp(pb->additional_encap[i]->chassis_name,
  |   ^~
controller/binding.c:1038:13: warning: implicit declaration of
function ‘sbrec_port_binding_update_additional_encap_delvalue’; did
you mean ‘sbrec_port_binding_update_encap_delvalue’?
[-Wimplicit-function-declaration]
 1038 | sbrec_port_binding_update_additional_encap_delvalue(
  | ^~~
  | sbrec_port_binding_update_encap_delvalue
controller/binding.c:1039:23: error: ‘const struct sbrec_port_binding’
has no member named ‘additional_encap’
 1039 | pb, pb->additional_encap[i]);
  |   ^~
controller/binding.c: In function ‘update_port_additional_encap_if_needed’:
controller/binding.c:1054:34: error: ‘const struct sbrec_port_binding’
has no member named ‘n_additional_encap’
 1054 | for (size_t i = 0; i < pb->n_additional_encap; i++) {
  |  ^~



root@lunar1:/home/nikhil/Downloads/ovn_upstream/ovn# git branch
* (HEAD detached from v22.03.2)
  main
root@lunar1:/home/nikhil/Downloads/ovn_upstream/ovn# git status
HEAD detached from v22.03.2
nothing to commit, working tree clean
root@lunar1:/home/nikhil/Downloads/ovn_upstream/ovn# git log
commit 5f761b894ff37da95cc4b8a036f14c440e2b5225 (HEAD)
Author: Ihar Hrachyshka 
Date:   Tue Aug 9 18:25:03 2022 +

controller: throttle port claim attempts

When multiple chassis are fighting for the same port (requested-chassis
is not set, e.g. for gateway ports), they may produce an unreasonable
number of chassis field updates in a very short time frame (hundreds of
updates in several seconds). This puts unnecessary load on OVN as well
as any db notification consumers trying to keep up with the barrage.

This patch throttles port claim attempts so that they don't happen more
frequently than once per 0.5 seconds.

Reported: https://bugzilla.redhat.com/show_bug.cgi?id=1974898
Signed-off-by: Ihar Hrachyshka 
Acked-by: Mark Michelson 
Signed-off-by: Numan Siddique 

commit ea177f1f93bc5ac52a17b6252901403c58708380
Author: Ihar Hrachyshka 
Date:   Sat Jun 18 00:54:49 2022 +

Implement RARP activation strategy for ports

When options:activation-strategy is set to "rarp" for LSP, when used in
combination with multiple chassis names listed in
options:requested-chassis, additional chassis will install special flows
that would block all ingress and egress traffic for the port until a
special activation event happens.

For "rarp" strategy, an observation of a RARP packet sent from the port
on the additional chassis is such an event. When it occurs, a special
flow passes control to a controller() action handler that eventually
removes the installed blocking flows and also marks the port as
options:additional-chassis-activated in southbound db.

This feature is useful in live migration scenarios where it's not
advisable to unlock the destination port location prematurily to avoid
duplicate packets originating from the port.

Signed-off-by: Ihar Hrachyshka 
Acked-by: Mark Michelson 
Signed-off-by: Numan Siddique 

commit 231c712dac0727316f457346f10bfdfdefb977e2
Author: Han Zhou 
Date:   Mon Dec 19 11:41:58 2022 -0800

northd: Drop packets destined to router owned NAT IP for DGP.

When a packet enters LR pipeline from a distributed gateway port with
destination IP being a SNAT IP, it goes through the unSNAT stage and
it is possible that the unSNAT fails to conver

Re: [ovs-dev] [PATCH 4/4] tests: Add ovsdb execution cases for set size constraints.

[Ilya Maximets]

On 8/2/23 22:19, Dumitru Ceara wrote:
> On 7/25/23 11:32, Ilya Maximets wrote:
>> Adding an extra check to one of the ovsdb execution cases that will
>> verify that ovsdb-server is able to read back transactions previously
>> written to a database file.  And also adding new execution tests
>> that cover previously discovered issues with size checks on sets.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
>>  tests/ovsdb-execution.at | 54 ++--
>>  tests/ovsdb-server.at| 15 ++-
> 
> tests++
> 
> Looks good to me, thanks!
> 
> Acked-by: Dumitru Ceara 
> 

Thanks, Dumitru!

I fixed the typos and applied the set.  Backported down to 2.17.

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

Re: [ovs-dev] [PATCH] ovsdb-server: Fix excessive memory usage on DB open.

[Ilya Maximets]

On 8/2/23 22:41, Dumitru Ceara wrote:
> On 8/2/23 15:45, Ilya Maximets wrote:
>> During initial read of a database file all the file transactions are
>> added to the transaction history.  The history run with the history
>> size checks is only executed after the whole file is processed.
>> If, for some reason, the file contains way too many transactions,
>> this behavior may result in excessive memory consumption up to
>> hundreds of GBs.  For example, here is a log entry about memory usage
>> after reading a file with 100K+ OVN NbDB transactions:
>>
>>   |4|memory|INFO|95650400 kB peak resident set size after 96.9 seconds
>>   |5|memory|INFO|atoms:3083346 cells:1838767 monitors:0
>> raft-log:123309 txn-history:123307 txn-history-atoms:1647022868
>>
>> In this particular case ovsdb-server allocated 95 GB of RAM in order
>> to accommodate 1.6 billion ovsdb atoms in the history, while only 3
>> million atoms are in the actual database.
> 
> Wow..
> 
>>
>> Fix that by running history size checks after applying each file
>> transaction.  This way the memory usage while reading the database
>> from the example stays at about 1 GB mark.  History size checks are
>> cheap in comparison with transaction replay, so the additional calls
>> do not reduce performance.
>>
>> We could've just moved the history run into ovsdb_txn_replay_commit(),
>> but it seems more organic to call it externally, since we have init()
>> and destroy() functions called externally as well.
>>
>> Since the history run will be executed shortly after reading the
>> database and actual memory consumption peak is not always logged,
>> there seem to be no reliable way to unit test for the issue without
>> adding extra testing infrastructure into the code.
>>
>> Fixes: 695e81502794 ("ovsdb-server: Transaction history tracking.")
>> Reported-at: https://bugzilla.redhat.com/2228464
>> Signed-off-by: Ilya Maximets 
>> ---
>>  ovsdb/ovsdb-server.c | 3 ++-
>>  ovsdb/relay.c| 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>> index 8e623118b..cf09c9079 100644
>> --- a/ovsdb/ovsdb-server.c
>> +++ b/ovsdb/ovsdb-server.c
>> @@ -235,7 +235,7 @@ main_loop(struct server_config *config,
>>  
>>  SHASH_FOR_EACH_SAFE (node, all_dbs) {
>>  struct db *db = node->data;
>> -ovsdb_txn_history_run(db->db);
>> +
>>  ovsdb_storage_run(db->db->storage);
>>  read_db(config, db);
>>  /* Run triggers after storage_run and read_db to make sure new 
>> raft
>> @@ -678,6 +678,7 @@ parse_txn(struct server_config *config, struct db *db,
>>  if (!error && !uuid_is_zero(txnid)) {
>>  db->db->prereq = *txnid;
>>  }
>> +ovsdb_txn_history_run(db->db);
>>  }
>>  return error;
>>  }
>> diff --git a/ovsdb/relay.c b/ovsdb/relay.c
>> index b035cb492..27ff196b7 100644
>> --- a/ovsdb/relay.c
>> +++ b/ovsdb/relay.c
>> @@ -413,6 +413,7 @@ ovsdb_relay_run(void)
>>  }
>>  ovsdb_cs_event_destroy(event);
>>  }
>> +ovsdb_txn_history_run(ctx->db);
>>  }
>>  }
>>  
> 
> It's always cool and somewhat worrying to see such a
> commit-log-size-to-loc ratio but I think this is fine:
> 
> Acked-by: Dumitru Ceara 

Thanks!  Applied and backported down to 2.17.

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

Re: [ovs-dev] [PATCH] ovsdb-tool: Fix json leak while showing clustered log.

[Ilya Maximets]

On 8/2/23 22:43, Dumitru Ceara wrote:
> On 8/2/23 18:46, Ilya Maximets wrote:
>> The json read from file is never freed in ovsdb-tool show-log
>> for a clustered database:
>>
>>  ERROR: LeakSanitizer: detected memory leaks
>>
>>  Direct leak of 10774760 byte(s) in 269369 object(s) allocated from:
>> 0 0x50cc32 in malloc (ovsdb/ovsdb-tool+0x50cc32)
>> 1 0x6e7b6b in xmalloc__ lib/util.c:140:15
>> 2 0x6e7b6b in xmalloc lib/util.c:175:12
>> 3 0x6494f6 in json_create lib/json.c:1489:25
>> 4 0x64a8a7 in json_object_create lib/json.c:263:25
>> 5 0x6525f3 in json_parser_push_object lib/json.c:1311:25
>> 6 0x6525f3 in json_parser_input lib/json.c:1409:13
>> 7 0x64f6c4 in json_parser_feed lib/json.c:1126:17
>> 8 0x5694b5 in parse_body ovsdb/log.c:412:9
>> 9 0x5694b5 in ovsdb_log_read ovsdb/log.c:477:13
>>10 0x54d294 in do_show_log_cluster ovsdb/ovsdb-tool.c:1069:27
>>11 0x54d294 in do_show_log ovsdb/ovsdb-tool.c:1115:9
>>12 0x63b7b1 in ovs_cmdl_run_command__ lib/command-line.c:247:17
>>13 0x5488a5 in main ovsdb/ovsdb-tool.c:82:5
>>14 0xe0eb49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49)
>>15 0xe0ec0a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x27c0a)
>>16 0x471fe4 in _start (ovsdb/ovsdb-tool+0x471fe4)
>>
>> Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered 
>> databases.")
>> Reported-by: Dumitru Ceara 
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> Thanks for the quick fix!  Looks good to me:
> 
> Acked-by: Dumitru Ceara 

Thanks!  Applied and backported down to 2.17.

Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH v3] ovs-tcpdump: Bugfix-of-ovs-tcpdump

[Ilya Maximets]

On 7/28/23 03:41, Simon Jones wrote:
> From: Simon Jones 
> 
> ovs-tcpdump: clear auto-assigned ipv6 address of mirror port

I think, that supposed to be a Subject line.

> 
> ovs-tcpdump will add mipxxx NIC, and on some systems this NIC has IPv6
> address by default.  For vxlan topology, mipxxx, which has IPv6
> address, will be treated as tunnel port, and will got error actions.
> 
> Prevent this by clearing the auto-assigned IPv6 address.  This can also
> be controlled on some systems with ipv6 sysctls.
> 
> Tested on centos stream 8, and ubunto20.04

* ubuntu

> 
> Signed-off-by: Simon Jones 
> Acked-by: Aaron Conole 

Thanks!  I added an Ack from Mike from a previous version, since the
code didn't change, changed the Subject line and applied the patch.
Backported down to 2.17.

Best regards, Ilya Maximets.
___
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`

[James R T]

On Tue, Jul 11, 2023 at 5:50 PM Eelco Chaudron  wrote:
>
> This patch looks good, however, it needs a re-work due to changes to tunnel 
> checksum handling.
>
> //Eelco
>

Apologies for the late reply. I have been busy with other work. I will
fix the merge conflict in the next patch version.

Best regards,
James Raphael Tiovalen
___
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

[James R T]

On Tue, Jul 11, 2023 at 6:03 PM Eelco Chaudron  wrote:
>
> Add a new line here.
>

Sure.

>
> Assert should not be needed as xzalloc will do this.
>

Agreed, will remove this in the next patch version.

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

Re: [ovs-dev] [PATCH v12 7/8] lib, ovsdb: Add various null pointer checks

[James R T]

On Tue, Jul 11, 2023 at 6:33 PM Eelco Chaudron  wrote:
>
> Maybe just define it here (and remove it above)..
>
> char *name = node->name;
>

Sure.

>
> Line feed.
>

Sure.

>
> 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(&cond);
> struct ovsdb_monitor_table_condition *mtc =
> shash_find_data(&condition->tables, table->schema->name);
>
> if (!mtc) {
> return NULL;
> }
>
> Or add surrounding new lines:
>
> struct ovsdb_monitor_table_condition *mtc =
> shash_find_data(&condition->tables, table->schema->name);
>
> if (!mtc) {
> return NULL;
> }
>
> struct ovsdb_error *error;
> struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER(&cond);
>
>

Sure, will move it down in the next patch version. Looks a bit neater.

>
> Is this needed, as x2nrealloc will assert if no memory is available?
>

Yes this is still needed, as x2nrealloc does not always execute when
`ovsdb_row_set_add_row` is called. It is only executed if the
condition `set->n_rows >= set->allocated_rows` is satisfied.

Best regards,
James Raphael Tiovalen
___
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

[James R T]

On Tue, Jul 11, 2023 at 7:40 PM Eelco Chaudron  wrote:
>
> I think we should not assert, but do proper error handling here with a 
> dpctl_error()
>

Sure, will do that in the next patch version.

>
> Same as above.
>

Got it.

>
> I think if we do a pop from an empty set, we should just return NULL, not 
> assert.
>

Sure.

>
> Did you analyse we only need the check for parent and not vsctl_bridge or 
> port_config? Maybe it would be nicer to fix the calling functions to avoid 
> this warning, as I guess that is where the real problem exists?
>

After checking the caller functions again, I realized that they now
have an `ovs_assert`. Hence, this should no longer be necessary. Will
remove this.

>
> Is this really needed? As looking at the code add_port_to_cache() will assert 
> and never return Null?
>
> Asking as you did not add a similar assert for line 791 above; ‘br = 
> add_bridge_to_cache(vsctl_ctx, br_cfg, br_cfg->name, NULL, 0);’
>

Yup, should not be necessary given that it uses xzalloc. Will remove this.

>
> See above.

Got it.

>
> Cant we not log a warning and skip freeing the data and the node?
>

Sure, will do that instead.

Best regards,
James Raphael Tiovalen
___
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

[James R T]

On Wed, Jul 12, 2023 at 6:49 AM Ilya Maximets  wrote:
>
> 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.
>

Sure, will remove them in the next patch version then.

>
> 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.
>

Got it, will remove the check as well.

>
> 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.
>

Hmm okay, I will remove this check then.

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

[ovs-dev] [PATCH v13 0/4] treewide: Fix multiple Coverity defects

[James Raphael Tiovalen]

This cleanup patchset addresses several high and medium-impact Coverity
defects.

Unit tests have been successfully executed via `make check` and they
successfully passed.

The list of revisions so far:

v2:
Fix some apply-robot checkpatch errors and warnings.

v3:
Fix some apply-robot checkpatch errors and warnings.

v4:
- Fix some apply-robot checkpatch errors and warnings.
- Fix github-robot build failure: linux clang test asan.

v5:
Improve commit message to better describe the intent of this patch.

v6:
- Convert some explicit null pointer checks to assertions since they are
checking for impossible conditions.
- Use `nullable_memset()` and `nullable_memcpy()` instead of manually
performing null checks for `memset()` and `memcpy()`.

v7:
- Split the single commit into a patch series which consists of 8
patches to make it easier to review and help keep things organized.
- Incorporated feedback from Aaron Conole.

v8:
Incorporated feedback from Simon Horman and Ilya Maximets.

v9:
Fix warning for patch 2/8 due to using `sizeof` on a void pointer.

v10:
Resolve merge conflict for patch 8/8.

v11:
Fix order of calling `ovs_assert` before `memset` in patch 2/8.

v12:
Fix failure of some tests in patch 4/8 by checking for warning logs in
OVSDB_SERVER_SHUTDOWN.

v13:
- Incorporated feedback from Eelco Chaudron and Ilya Maximets.
- Regrouped some changes between patches 3/4 and 4/4.

James Raphael Tiovalen (4):
  lib: Add non-null assertions to some return values of `dp_packet_data`
  lib, ovs-vsctl: Add zero-initializations
  lib, ovsdb, vtep: Add various null pointer checks
  treewide: Add `ovs_assert` to check for null pointers

 lib/dp-packet.c | 24 +++-
 lib/dpctl.c | 12 
 lib/latch-unix.c|  2 +-
 lib/netdev-native-tnl.c |  6 +-
 lib/odp-execute.c   |  4 
 lib/pcap-file.c |  4 +++-
 lib/rtnetlink.c |  4 ++--
 lib/sflow_agent.c   | 12 ++--
 lib/sflow_api.h |  2 +-
 lib/shash.c |  6 +-
 lib/sset.c  |  5 +
 ovsdb/jsonrpc-server.c  |  6 +-
 ovsdb/monitor.c | 14 --
 ovsdb/ovsdb-client.c|  7 ++-
 ovsdb/ovsdb-server.c|  2 ++
 ovsdb/row.c |  5 -
 ovsdb/transaction.c |  2 ++
 utilities/ovs-vsctl.c   |  6 +++---
 vtep/vtep-ctl.c |  6 ++
 19 files changed, 103 insertions(+), 26 deletions(-)

--
2.41.0

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

[ovs-dev] [PATCH v13 1/4] lib: Add non-null assertions to some return values of `dp_packet_data`

[James Raphael Tiovalen]

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 
---
 lib/dp-packet.c | 15 ++-
 lib/netdev-native-tnl.c |  6 +-
 lib/pcap-file.c |  4 +++-
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 27114a9a9..6e3a8297a 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -187,9 +187,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(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
@@ -326,8 +329,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 715bbab2b..311ba6895 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);
@@ -419,7 +420,10 @@ netdev_gre_pop_header(struct dp_packet *packet)
 struct flow_tnl *tnl = &md->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(&tv);
 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(&prh, 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.41.0

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

[ovs-dev] [PATCH v13 2/4] lib, ovs-vsctl: Add zero-initializations

[James Raphael Tiovalen]

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.

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..743774a27 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;
+
+if (agent->allocFn) {
+alloc = (*agent->allocFn)(agent->magic, agent, bytes);
+ovs_assert(alloc);
+memset(alloc, 0, bytes);
+} else {
+alloc = SFL_ALLOC(bytes);
+}
+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 62b512302..f55c2965a 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(&br->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(&parent->ports, &port->ports_node);
 ovs_list_init(&port->ifaces);
 port->port_cfg = port_cfg;
-- 
2.41.0

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

[ovs-dev] [PATCH v13 3/4] lib, ovsdb, vtep: Add various null pointer checks

[James Raphael Tiovalen]

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 
---
 lib/dp-packet.c|  8 
 lib/dpctl.c| 12 
 lib/shash.c|  4 
 lib/sset.c |  5 +
 ovsdb/jsonrpc-server.c |  2 +-
 ovsdb/monitor.c| 11 +--
 ovsdb/ovsdb-client.c   |  7 ++-
 ovsdb/row.c|  5 -
 vtep/vtep-ctl.c|  5 +
 9 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 6e3a8297a..a9cf4bfc1 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -357,7 +357,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;
 }
 
@@ -368,7 +368,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;
 }
 
@@ -440,7 +440,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;
 }
 
@@ -451,7 +451,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/dpctl.c b/lib/dpctl.c
index 4394653ab..013919572 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -336,6 +336,12 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[],
 value = "";
 }
 
+if (!key) {
+dpctl_error(dpctl_p, 0, "key is NULL");
+error = EINVAL;
+goto next;
+}
+
 if (!strcmp(key, "type")) {
 type = value;
 } else if (!strcmp(key, "port_no")) {
@@ -454,6 +460,12 @@ dpctl_set_if(int argc, const char *argv[], struct 
dpctl_params *dpctl_p)
 value = "";
 }
 
+if (!key) {
+dpctl_error(dpctl_p, 0, "key is NULL");
+error = EINVAL;
+goto next_destroy_args;
+}
+
 if (!strcmp(key, "type")) {
 if (strcmp(value, type)) {
 dpctl_error(dpctl_p, 0,
diff --git a/lib/shash.c b/lib/shash.c
index 2bfc8eb50..b62b586fc 100644
--- a/lib/shash.c
+++ b/lib/shash.c
@@ -205,6 +205,10 @@ shash_delete(struct shash *sh, struct shash_node *node)
 char *
 shash_steal(struct shash *sh, struct shash_node *node)
 {
+if (!node) {
+return NULL;
+}
+
 char *name = node->name;
 
 hmap_remove(&sh->map, &node->node);
diff --git a/lib/sset.c b/lib/sset.c
index aa1790020..fda268129 100644
--- a/lib/sset.c
+++ b/lib/sset.c
@@ -261,6 +261,11 @@ char *
 sset_pop(struct sset *set)
 {
 const char *name = SSET_FIRST(set);
+
+if (!name) {
+return NULL;
+}
+
 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 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, &reply);
-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 01091fabe..af88b96e3 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -483,6 +483,9 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
 struct ovsdb_monitor_column *c;
 
 mt = shash_find_data(&dbmon->tables, table->schema->name);
+if (!mt) {
+return NULL;
+}
 
 /* Check for column duplication. Return duplicated column name. */
 if (mt->columns_index_map[column->index] != -1) {
@@ -808,10 +811,14 @@ ovsdb_monitor_table_condition_update(
 return NULL;
 }
 
-struct ovsdb_monitor_table_condition *mtc =
-shash_find_data(&condition->tables, table->schema->name);
 struct ovsdb_error *error;
 struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER(&cond);
+struct ovsdb_monitor_table_condition *mtc =
+shash_find_data(&condition->tables, table->schema->name);
+
+if (!mtc) {
+return

[ovs-dev] [PATCH v13 4/4] treewide: Add `ovs_assert` to check for null pointers

[James Raphael Tiovalen]

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 
---
 lib/dp-packet.c| 1 +
 lib/odp-execute.c  | 4 
 lib/rtnetlink.c| 4 ++--
 lib/shash.c| 2 +-
 ovsdb/jsonrpc-server.c | 4 
 ovsdb/monitor.c| 3 +++
 ovsdb/ovsdb-server.c   | 2 ++
 ovsdb/transaction.c| 2 ++
 utilities/ovs-vsctl.c  | 1 +
 vtep/vtep-ctl.c| 1 +
 10 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index a9cf4bfc1..4a3e149e3 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -175,6 +175,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/odp-execute.c b/lib/odp-execute.c
index 37f0f717a..eb03b57c4 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(&nh->ip_src);
 new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src);
@@ -287,6 +289,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 b62b586fc..92260cddf 100644
--- a/lib/shash.c
+++ b/lib/shash.c
@@ -269,7 +269,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/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)
 {
+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 af88b96e3..ca09780a9 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1329,6 +1329,7 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor 
*dbmon,
 struct ovsdb_monitor_table * mt;
 
 mt = shash_find_data(&dbmon->tables, table->schema->name);
+ovs_assert(mt);
 mt->select |= select;
 }
 
@@ -1713,6 +1714,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 8e623118b..1728d3a24 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -2257,6 +2257,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/transaction.c b/ovsdb/transaction.c
index 7cf4a851a..4fdc5bcea 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb

[ovs-dev] [PATCH v14 2/4] lib, ovs-vsctl: Add zero-initializations

[James Raphael Tiovalen]

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.

Signed-off-by: James Raphael Tiovalen 
Reviewed-by: Simon Horman 
---
 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..743774a27 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;
+
+if (agent->allocFn) {
+alloc = (*agent->allocFn)(agent->magic, agent, bytes);
+ovs_assert(alloc);
+memset(alloc, 0, bytes);
+} else {
+alloc = SFL_ALLOC(bytes);
+}
+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 62b512302..f55c2965a 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(&br->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(&parent->ports, &port->ports_node);
 ovs_list_init(&port->ifaces);
 port->port_cfg = port_cfg;
-- 
2.41.0

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

[ovs-dev] [PATCH v14 0/4] treewide: Fix multiple Coverity defects

[James Raphael Tiovalen]

This cleanup patchset addresses several high and medium-impact Coverity
defects.

Unit tests have been successfully executed via `make check` and they
successfully passed.

The list of revisions so far:

v2:
Fix some apply-robot checkpatch errors and warnings.

v3:
Fix some apply-robot checkpatch errors and warnings.

v4:
- Fix some apply-robot checkpatch errors and warnings.
- Fix github-robot build failure: linux clang test asan.

v5:
Improve commit message to better describe the intent of this patch.

v6:
- Convert some explicit null pointer checks to assertions since they are
checking for impossible conditions.
- Use `nullable_memset()` and `nullable_memcpy()` instead of manually
performing null checks for `memset()` and `memcpy()`.

v7:
- Split the single commit into a patch series which consists of 8
patches to make it easier to review and help keep things organized.
- Incorporated feedback from Aaron Conole.

v8:
Incorporated feedback from Simon Horman and Ilya Maximets.

v9:
Fix warning for patch 2/8 due to using `sizeof` on a void pointer.

v10:
Resolve merge conflict for patch 8/8.

v11:
Fix order of calling `ovs_assert` before `memset` in patch 2/8.

v12:
Fix failure of some tests in patch 4/8 by checking for warning logs in
OVSDB_SERVER_SHUTDOWN.

v13:
- Incorporated feedback from Eelco Chaudron and Ilya Maximets.
- Regrouped some changes between patches 3/4 and 4/4.

v14:
Fix inaccurate commit message for patches 2/4 and 3/4.

James Raphael Tiovalen (4):
  lib: Add non-null assertions to some return values of `dp_packet_data`
  lib, ovs-vsctl: Add zero-initializations
  lib, ovsdb, vtep: Add various null pointer checks
  treewide: Add `ovs_assert` to check for null pointers

 lib/dp-packet.c | 24 +++-
 lib/dpctl.c | 12 
 lib/latch-unix.c|  2 +-
 lib/netdev-native-tnl.c |  6 +-
 lib/odp-execute.c   |  4 
 lib/pcap-file.c |  4 +++-
 lib/rtnetlink.c |  4 ++--
 lib/sflow_agent.c   | 12 ++--
 lib/sflow_api.h |  2 +-
 lib/shash.c |  6 +-
 lib/sset.c  |  5 +
 ovsdb/jsonrpc-server.c  |  6 +-
 ovsdb/monitor.c | 14 --
 ovsdb/ovsdb-client.c|  7 ++-
 ovsdb/ovsdb-server.c|  2 ++
 ovsdb/row.c |  5 -
 ovsdb/transaction.c |  2 ++
 utilities/ovs-vsctl.c   |  6 +++---
 vtep/vtep-ctl.c |  6 ++
 19 files changed, 103 insertions(+), 26 deletions(-)

--
2.41.0

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

[ovs-dev] [PATCH v14 3/4] lib, ovsdb, vtep: Add various null pointer checks

[James Raphael Tiovalen]

This commit adds various null pointer checks to some files in the `lib`,
`ovsdb`, and `vtep` directories to fix several Coverity defects. These
changes are grouped together as they perform similar checks, returning
early, skipping some action, or logging a warning if a null pointer is
encountered.

Signed-off-by: James Raphael Tiovalen 
Reviewed-by: Simon Horman 
---
 lib/dp-packet.c|  8 
 lib/dpctl.c| 12 
 lib/shash.c|  4 
 lib/sset.c |  5 +
 ovsdb/jsonrpc-server.c |  2 +-
 ovsdb/monitor.c| 11 +--
 ovsdb/ovsdb-client.c   |  7 ++-
 ovsdb/row.c|  5 -
 vtep/vtep-ctl.c|  5 +
 9 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 6e3a8297a..a9cf4bfc1 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -357,7 +357,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;
 }
 
@@ -368,7 +368,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;
 }
 
@@ -440,7 +440,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;
 }
 
@@ -451,7 +451,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/dpctl.c b/lib/dpctl.c
index 4394653ab..013919572 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -336,6 +336,12 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[],
 value = "";
 }
 
+if (!key) {
+dpctl_error(dpctl_p, 0, "key is NULL");
+error = EINVAL;
+goto next;
+}
+
 if (!strcmp(key, "type")) {
 type = value;
 } else if (!strcmp(key, "port_no")) {
@@ -454,6 +460,12 @@ dpctl_set_if(int argc, const char *argv[], struct 
dpctl_params *dpctl_p)
 value = "";
 }
 
+if (!key) {
+dpctl_error(dpctl_p, 0, "key is NULL");
+error = EINVAL;
+goto next_destroy_args;
+}
+
 if (!strcmp(key, "type")) {
 if (strcmp(value, type)) {
 dpctl_error(dpctl_p, 0,
diff --git a/lib/shash.c b/lib/shash.c
index 2bfc8eb50..b62b586fc 100644
--- a/lib/shash.c
+++ b/lib/shash.c
@@ -205,6 +205,10 @@ shash_delete(struct shash *sh, struct shash_node *node)
 char *
 shash_steal(struct shash *sh, struct shash_node *node)
 {
+if (!node) {
+return NULL;
+}
+
 char *name = node->name;
 
 hmap_remove(&sh->map, &node->node);
diff --git a/lib/sset.c b/lib/sset.c
index aa1790020..fda268129 100644
--- a/lib/sset.c
+++ b/lib/sset.c
@@ -261,6 +261,11 @@ char *
 sset_pop(struct sset *set)
 {
 const char *name = SSET_FIRST(set);
+
+if (!name) {
+return NULL;
+}
+
 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 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, &reply);
-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 01091fabe..af88b96e3 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -483,6 +483,9 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
 struct ovsdb_monitor_column *c;
 
 mt = shash_find_data(&dbmon->tables, table->schema->name);
+if (!mt) {
+return NULL;
+}
 
 /* Check for column duplication. Return duplicated column name. */
 if (mt->columns_index_map[column->index] != -1) {
@@ -808,10 +811,14 @@ ovsdb_monitor_table_condition_update(
 return NULL;
 }
 
-struct ovsdb_monitor_table_condition *mtc =
-shash_find_data(&condition->tables, table->schema->name);
 struct ovsdb_error *error;
 struct ovsdb_condition cond = OVSDB_CONDITION_INITIALIZER(&cond);
+struct ovsdb_monitor_table_condition *mtc =
+shash_find_data(&condition->tables, table->schema->name);
+
+if

[ovs-dev] [PATCH v14 1/4] lib: Add non-null assertions to some return values of `dp_packet_data`

[James Raphael Tiovalen]

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 
---
 lib/dp-packet.c | 15 ++-
 lib/netdev-native-tnl.c |  6 +-
 lib/pcap-file.c |  4 +++-
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 27114a9a9..6e3a8297a 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -187,9 +187,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(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
@@ -326,8 +329,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 715bbab2b..311ba6895 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);
@@ -419,7 +420,10 @@ netdev_gre_pop_header(struct dp_packet *packet)
 struct flow_tnl *tnl = &md->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(&tv);
 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(&prh, 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.41.0

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

[ovs-dev] [PATCH v14 4/4] treewide: Add `ovs_assert` to check for null pointers

[James Raphael Tiovalen]

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 
---
 lib/dp-packet.c| 1 +
 lib/odp-execute.c  | 4 
 lib/rtnetlink.c| 4 ++--
 lib/shash.c| 2 +-
 ovsdb/jsonrpc-server.c | 4 
 ovsdb/monitor.c| 3 +++
 ovsdb/ovsdb-server.c   | 2 ++
 ovsdb/transaction.c| 2 ++
 utilities/ovs-vsctl.c  | 1 +
 vtep/vtep-ctl.c| 1 +
 10 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index a9cf4bfc1..4a3e149e3 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -175,6 +175,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/odp-execute.c b/lib/odp-execute.c
index 37f0f717a..eb03b57c4 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(&nh->ip_src);
 new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src);
@@ -287,6 +289,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 b62b586fc..92260cddf 100644
--- a/lib/shash.c
+++ b/lib/shash.c
@@ -269,7 +269,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/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)
 {
+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 af88b96e3..ca09780a9 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1329,6 +1329,7 @@ ovsdb_monitor_table_add_select(struct ovsdb_monitor 
*dbmon,
 struct ovsdb_monitor_table * mt;
 
 mt = shash_find_data(&dbmon->tables, table->schema->name);
+ovs_assert(mt);
 mt->select |= select;
 }
 
@@ -1713,6 +1714,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 8e623118b..1728d3a24 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -2257,6 +2257,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/transaction.c b/ovsdb/transaction.c
index 7cf4a851a..4fdc5bcea 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb

[ovs-dev] [PATCH ovn] northd: Fall back to 'northd' engine recompute for certain VIF scenarios.

[numans]

From: Numan Siddique 

When a logical switch has only router ports and if a new VIF port is
added, both northd engine and lflow engine handle this change
incrementally, but it misses out on adding a few logical flows
where we have checks like :
   if (od->n_router_ports != od->nbs->n_ports) {
ds_put_format(&actions, "clone {outport = %s; output; }; "
"outport = \""MC_FLOOD_L2"\"; output;",
  patch_op->json_key);

   } else {
   ds_put_format(&actions, "outport = %s; output;", patch_op->json_key);
   }

The same issue is seen when a VIF port is deleted and after which the
logical switch has only router ports.

This patch fixes this issue by falling back to full recompute of northd
engine node.  It is possible to handle these changes incrementally
in northd engine node but fall back to full recompute in lflow engine
node.  But this patch goes for a simpler fix.  This can be optimized
later if required.

Fixes: b337750e45be ("northd: Incremental processing of VIF changes in 'northd' 
node.")
CC: Han Zhou 
Signed-off-by: Numan Siddique 
---
 northd/northd.c | 10 ++
 tests/ovn-northd.at | 28 
 2 files changed, 38 insertions(+)

diff --git a/northd/northd.c b/northd/northd.c
index b9605862e..462fa83ca 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5120,6 +5120,11 @@ northd_handle_ls_changes(struct ovsdb_idl_txn 
*ovnsb_idl_txn,
 op->visited = false;
 }
 
+bool only_rports = (od->n_router_ports == hmap_count(&od->ports));
+if (only_rports) {
+goto fail;
+}
+
 /* Compare the individual ports in the old and new Logical Switches */
 for (size_t j = 0; j < changed_ls->n_ports; ++j) {
 struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j];
@@ -5201,6 +5206,11 @@ northd_handle_ls_changes(struct ovsdb_idl_txn 
*ovnsb_idl_txn,
 }
 }
 
+only_rports = (od->n_router_ports == hmap_count(&od->ports));
+if (only_rports) {
+goto fail_clean_deleted;
+}
+
 if (!ovs_list_is_empty(&ls_change->added_ports) ||
 !ovs_list_is_empty(&ls_change->updated_ports) ||
 !ovs_list_is_empty(&ls_change->deleted_ports)) {
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 3e06f14c9..912aa5431 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -9589,6 +9589,34 @@ check_recompute_counter 0 0
 
 CHECK_NO_CHANGE_AFTER_RECOMPUTE
 
+check ovn-nbctl --wait=hv ls-del ls0
+check ovn-nbctl ls-add ls0
+check ovn-nbctl --wait=sb lr-add lr0
+ovn-nbctl lrp-add lr0 lr0-ls0 00:00:00:00:ff:01 192.168.0.1/24
+ovn-nbctl lsp-add ls0 ls0-lr0
+ovn-nbctl lsp-set-type ls0-lr0 router
+ovn-nbctl lsp-set-addresses ls0-lr0 router
+check ovn-nbctl --wait=sb lsp-set-options ls0-lr0 router-port=lr0-ls0
+
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+ovn-nbctl lb-add lb0 192.168.0.10:80 10.0.0.10:8080
+check ovn-nbctl --wait=sb ls-lb-add ls0 lb0
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+# Add a lsp.  northd and lflow engine should recompute since this is
+# the first lsp added after the router ports.
+check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 
"aa:aa:aa:00:00:01 192.168.0.11"
+check_recompute_counter 1 1
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+# Delete the lsp.  northd and lflow engine should recompute.
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl lsp-del lsp0-1
+check_recompute_counter 1 1
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
-- 
2.40.1

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

Re: [ovs-dev] [PATCH v3] ovs-tcpdump: Bugfix-of-ovs-tcpdump

[Simon Jones]

OK.

At last, I think this patch is done, nothing need I do. Is that?


Simon Jones


Ilya Maximets  于2023年8月3日周四 22:46写道：

> On 7/28/23 03:41, Simon Jones wrote:
> > From: Simon Jones 
> >
> > ovs-tcpdump: clear auto-assigned ipv6 address of mirror port
>
> I think, that supposed to be a Subject line.
>
> >
> > ovs-tcpdump will add mipxxx NIC, and on some systems this NIC has IPv6
> > address by default.  For vxlan topology, mipxxx, which has IPv6
> > address, will be treated as tunnel port, and will got error actions.
> >
> > Prevent this by clearing the auto-assigned IPv6 address.  This can also
> > be controlled on some systems with ipv6 sysctls.
> >
> > Tested on centos stream 8, and ubunto20.04
>
> * ubuntu
>
> >
> > Signed-off-by: Simon Jones 
> > Acked-by: Aaron Conole 
>
> Thanks!  I added an Ack from Mike from a previous version, since the
> code didn't change, changed the Subject line and applied the patch.
> Backported down to 2.17.
>
> Best regards, Ilya Maximets.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev