| Message ID | 20250331223851.373920-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-northd: Remove redundant check in update_sequence_numbers(). | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 4/1/25 12:38 AM, Dumitru Ceara via dev wrote: > There is no point to check if the 'nb' pointer is non-NULL because we > make sure the NB_Global IDL record exists in the beginning of the > function. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- docker: Error response from daemon: Head "https://quay.io/v2/itssurya/dev-images/manifests/metallb-lbservice": received unexpected HTTP status: 502 Bad Gateway. https://github.com/ovsrobot/ovn/actions/runs/14183112992/job/39733821668 Recheck-request: github-robot-_ovn-kubernetes
On Tue, Apr 1, 2025 at 12:39 AM Dumitru Ceara via dev < ovs-dev@openvswitch.org> wrote: > There is no point to check if the 'nb' pointer is non-NULL because we > make sure the NB_Global IDL record exists in the beginning of the > function. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > northd/ovn-northd.c | 63 ++++++++++++++++++++++----------------------- > 1 file changed, 31 insertions(+), 32 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 72eedbfdba..39879a1ff6 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -532,49 +532,48 @@ update_sequence_numbers(int64_t loop_start_time, > > /* Update northbound sb_cfg if appropriate. */ > int64_t sb_cfg = sb_loop->cur_cfg; > - if (nb && sb_cfg && nb->sb_cfg != sb_cfg) { > + if (sb_cfg && nb->sb_cfg != sb_cfg) { > nbrec_nb_global_set_sb_cfg(nb, sb_cfg); > nbrec_nb_global_set_sb_cfg_timestamp(nb, loop_start_time); > } > > /* Update northbound hv_cfg if appropriate. */ > - if (nb) { > - /* Find minimum nb_cfg among all chassis. */ > - const struct sbrec_chassis_private *chassis_priv; > - int64_t hv_cfg = nb->nb_cfg; > - int64_t hv_cfg_ts = 0; > - SBREC_CHASSIS_PRIVATE_FOR_EACH (chassis_priv, ovnsb_idl) { > - const struct sbrec_chassis *chassis = chassis_priv->chassis; > - if (chassis) { > - if (smap_get_bool(&chassis->other_config, > - "is-remote", false)) { > - /* Skip remote chassises. */ > - continue; > - } > - } else { > - static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "Chassis does not exist for " > - "Chassis_Private record, name: %s", > - chassis_priv->name); > - } > > - /* Detect if overflows happened within the cfg update. */ > - int64_t delta = chassis_priv->nb_cfg - hv_cfg; > - if (chassis_priv->nb_cfg < hv_cfg || delta > INT32_MAX) { > - hv_cfg = chassis_priv->nb_cfg; > - hv_cfg_ts = chassis_priv->nb_cfg_timestamp; > - } else if (chassis_priv->nb_cfg == hv_cfg && > - chassis_priv->nb_cfg_timestamp > hv_cfg_ts) { > - hv_cfg_ts = chassis_priv->nb_cfg_timestamp; > + /* Find minimum nb_cfg among all chassis. */ > + const struct sbrec_chassis_private *chassis_priv; > + int64_t hv_cfg = nb->nb_cfg; > + int64_t hv_cfg_ts = 0; > + SBREC_CHASSIS_PRIVATE_FOR_EACH (chassis_priv, ovnsb_idl) { > + const struct sbrec_chassis *chassis = chassis_priv->chassis; > + if (chassis) { > + if (smap_get_bool(&chassis->other_config, > + "is-remote", false)) { > + /* Skip remote chassises. */ > + continue; > } > + } else { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Chassis does not exist for " > + "Chassis_Private record, name: %s", > + chassis_priv->name); > } > > - /* Update hv_cfg. */ > - if (nb->hv_cfg != hv_cfg) { > - nbrec_nb_global_set_hv_cfg(nb, hv_cfg); > - nbrec_nb_global_set_hv_cfg_timestamp(nb, hv_cfg_ts); > + /* Detect if overflows happened within the cfg update. */ > + int64_t delta = chassis_priv->nb_cfg - hv_cfg; > + if (chassis_priv->nb_cfg < hv_cfg || delta > INT32_MAX) { > + hv_cfg = chassis_priv->nb_cfg; > + hv_cfg_ts = chassis_priv->nb_cfg_timestamp; > + } else if (chassis_priv->nb_cfg == hv_cfg && > + chassis_priv->nb_cfg_timestamp > hv_cfg_ts) { > + hv_cfg_ts = chassis_priv->nb_cfg_timestamp; > } > } > + > + /* Update hv_cfg. */ > + if (nb->hv_cfg != hv_cfg) { > + nbrec_nb_global_set_hv_cfg(nb, hv_cfg); > + nbrec_nb_global_set_hv_cfg_timestamp(nb, hv_cfg_ts); > + } > } > > static void > -- > 2.48.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 4/3/25 10:05 AM, Ales Musil wrote: > On Tue, Apr 1, 2025 at 12:39 AM Dumitru Ceara via dev < > ovs-dev@openvswitch.org> wrote: > >> There is no point to check if the 'nb' pointer is non-NULL because we >> make sure the NB_Global IDL record exists in the beginning of the >> function. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> > Hi Ales, Thanks for the review! Applied to main. Regards, Dumitru
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 72eedbfdba..39879a1ff6 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -532,49 +532,48 @@ update_sequence_numbers(int64_t loop_start_time, /* Update northbound sb_cfg if appropriate. */ int64_t sb_cfg = sb_loop->cur_cfg; - if (nb && sb_cfg && nb->sb_cfg != sb_cfg) { + if (sb_cfg && nb->sb_cfg != sb_cfg) { nbrec_nb_global_set_sb_cfg(nb, sb_cfg); nbrec_nb_global_set_sb_cfg_timestamp(nb, loop_start_time); } /* Update northbound hv_cfg if appropriate. */ - if (nb) { - /* Find minimum nb_cfg among all chassis. */ - const struct sbrec_chassis_private *chassis_priv; - int64_t hv_cfg = nb->nb_cfg; - int64_t hv_cfg_ts = 0; - SBREC_CHASSIS_PRIVATE_FOR_EACH (chassis_priv, ovnsb_idl) { - const struct sbrec_chassis *chassis = chassis_priv->chassis; - if (chassis) { - if (smap_get_bool(&chassis->other_config, - "is-remote", false)) { - /* Skip remote chassises. */ - continue; - } - } else { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "Chassis does not exist for " - "Chassis_Private record, name: %s", - chassis_priv->name); - } - /* Detect if overflows happened within the cfg update. */ - int64_t delta = chassis_priv->nb_cfg - hv_cfg; - if (chassis_priv->nb_cfg < hv_cfg || delta > INT32_MAX) { - hv_cfg = chassis_priv->nb_cfg; - hv_cfg_ts = chassis_priv->nb_cfg_timestamp; - } else if (chassis_priv->nb_cfg == hv_cfg && - chassis_priv->nb_cfg_timestamp > hv_cfg_ts) { - hv_cfg_ts = chassis_priv->nb_cfg_timestamp; + /* Find minimum nb_cfg among all chassis. */ + const struct sbrec_chassis_private *chassis_priv; + int64_t hv_cfg = nb->nb_cfg; + int64_t hv_cfg_ts = 0; + SBREC_CHASSIS_PRIVATE_FOR_EACH (chassis_priv, ovnsb_idl) { + const struct sbrec_chassis *chassis = chassis_priv->chassis; + if (chassis) { + if (smap_get_bool(&chassis->other_config, + "is-remote", false)) { + /* Skip remote chassises. */ + continue; } + } else { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "Chassis does not exist for " + "Chassis_Private record, name: %s", + chassis_priv->name); } - /* Update hv_cfg. */ - if (nb->hv_cfg != hv_cfg) { - nbrec_nb_global_set_hv_cfg(nb, hv_cfg); - nbrec_nb_global_set_hv_cfg_timestamp(nb, hv_cfg_ts); + /* Detect if overflows happened within the cfg update. */ + int64_t delta = chassis_priv->nb_cfg - hv_cfg; + if (chassis_priv->nb_cfg < hv_cfg || delta > INT32_MAX) { + hv_cfg = chassis_priv->nb_cfg; + hv_cfg_ts = chassis_priv->nb_cfg_timestamp; + } else if (chassis_priv->nb_cfg == hv_cfg && + chassis_priv->nb_cfg_timestamp > hv_cfg_ts) { + hv_cfg_ts = chassis_priv->nb_cfg_timestamp; } } + + /* Update hv_cfg. */ + if (nb->hv_cfg != hv_cfg) { + nbrec_nb_global_set_hv_cfg(nb, hv_cfg); + nbrec_nb_global_set_hv_cfg_timestamp(nb, hv_cfg_ts); + } } static void
There is no point to check if the 'nb' pointer is non-NULL because we make sure the NB_Global IDL record exists in the beginning of the function. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.c | 63 ++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 32 deletions(-)