Hi Ales
Thanks for the patch.
On Wed, Jun 25, 2025 at 3:58 PM Ales Musil via dev <ovs-dev@openvswitch.org>
wrote:
> The handler was fetching data even if SB was readonly. This handler
> can return right away if SB is read only because all the operations
> done by this handler require SB to be writable.
>
I do not think that the commit message is 100% correct: sb_ro node is only
"updated" if sb becomes writable, so
this handler is only called if sb is writable.
However, the patch makes some cleanup (as there was no need for sb_ro flag
in if_status_handle_claims),
and the immediate check in the handler is safe programming.
In other words, patch LGTM but I'd change the commit message.
Acked-by: Xavier Simonart <xsimo...@redhat.com>
Thanks
Xavier
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> controller/if-status.c | 5 ++---
> controller/if-status.h | 3 +--
> controller/ovn-controller.c | 14 ++++++++------
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/controller/if-status.c b/controller/if-status.c
> index dc7b66ee2..32b6064ba 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -428,10 +428,9 @@ if_status_handle_claims(struct if_status_mgr *mgr,
> struct local_binding_data *binding_data,
> const struct sbrec_chassis *chassis_rec,
> struct hmap *tracked_datapath,
> - const struct sbrec_port_binding_table *pb_table,
> - bool sb_readonly)
> + const struct sbrec_port_binding_table *pb_table)
> {
> - if (!binding_data || sb_readonly) {
> + if (!binding_data) {
> return false;
> }
>
> diff --git a/controller/if-status.h b/controller/if-status.h
> index eb91b62fd..d4f972355 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -58,8 +58,7 @@ bool if_status_handle_claims(struct if_status_mgr *mgr,
> struct local_binding_data *binding_data,
> const struct sbrec_chassis *chassis_rec,
> struct hmap *tracked_datapath,
> - const struct sbrec_port_binding_table
> *pb_table,
> - bool sb_readonly);
> + const struct sbrec_port_binding_table
> *pb_table);
> void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
> const struct ovsrec_interface
> *iface_rec);
> uint16_t if_status_mgr_iface_get_mtu(const struct if_status_mgr *mgr,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 1365f3e65..cd568de01 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1627,6 +1627,11 @@ en_sb_ro_cleanup(void *data OVS_UNUSED)
> static enum engine_input_handler_result
> runtime_data_sb_ro_handler(struct engine_node *node, void *data)
> {
> + /* Return right away if the SB is read only. */
> + if (!engine_get_context()->ovnsb_idl_txn) {
> + return EN_HANDLED_UNCHANGED;
> + }
> +
> const struct sbrec_chassis *chassis = NULL;
> enum engine_input_handler_result result = EN_HANDLED_UNCHANGED;
>
> @@ -1648,15 +1653,12 @@ runtime_data_sb_ro_handler(struct engine_node
> *node, void *data)
> }
> if (chassis) {
> struct ed_type_runtime_data *rt_data = data;
> - bool sb_readonly = !engine_get_context()->ovnsb_idl_txn;
> struct controller_engine_ctx *ctrl_ctx =
> engine_get_context()->client_ctx;
>
> - if (if_status_handle_claims(ctrl_ctx->if_mgr,
> - &rt_data->lbinding_data,
> - chassis,
> - &rt_data->tracked_dp_bindings,
> - pb_table, sb_readonly)) {
> + if (if_status_handle_claims(ctrl_ctx->if_mgr,
> &rt_data->lbinding_data,
> + chassis,
> &rt_data->tracked_dp_bindings,
> + pb_table)) {
> result = EN_HANDLED_UPDATED;
> rt_data->tracked = true;
> }
> --
> 2.49.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
