On Thu, Jun 26, 2025 at 12:24 PM Xavier Simonart <xsimo...@redhat.com>
wrote:
> Hi Ales
>
> Thanks for the patch.
>
Hi Xavier,
thank you for the review.
>
> 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.
>
Ah you are right I misread the handler, I will remove the check too in v2,
there is really no point in
having that. Even though it's a good practice it shouldn't happen and if it
does we should crash
and fix the issue IMO.
>
> 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
>>
>>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
