On Tue, Dec 6, 2022 at 3:33 PM Dumitru Ceara <[email protected]> wrote:
> On 12/6/22 15:20, Ales Musil wrote: > > In order to keep backward compatibility with northd we need > > to check if MAC binding table actually has the timestamp column. > > > > Reported-at: https://bugzilla.redhat.com/2151066 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > v2: Address comments from Dumitru. > > --- > > I have a few more minor comments, please see below. > > > controller/pinctrl.c | 36 ++++++++++++++++++++++++++++++++---- > > 1 file changed, 32 insertions(+), 4 deletions(-) > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index f44775c7e..15a3ff31d 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -173,6 +173,7 @@ struct pinctrl { > > pthread_t pinctrl_thread; > > /* Latch to destroy the 'pinctrl_thread' */ > > struct latch pinctrl_thread_exit; > > + bool mac_binding_has_timestamp_column; > > Nit: I'd call this 'mac_binding_can_timestamp'. > Updated in v3. > > }; > > > > static struct pinctrl pinctrl; > > @@ -544,6 +545,7 @@ pinctrl_init(void) > > bfd_monitor_init(); > > init_fdb_entries(); > > pinctrl.br_int_name = NULL; > > + pinctrl.mac_binding_has_timestamp_column = false; > > pinctrl_handler_seq = seq_create(); > > pinctrl_main_seq = seq_create(); > > > > @@ -3532,6 +3534,26 @@ pinctrl_set_br_int_name_(char *br_int_name) > > } > > } > > > > +static void > > +pinctrl_set_sb_mac_binding_has_timestamp_column(struct ovsdb_idl_txn > *txn) > > + OVS_REQUIRES(pinctrl_mutex) > > The column existence is not really a property of 'txn'. I'd pass > 'struct ovsdb_idl *idl' instead. > > Also, I think I'd change this to something like: > > static void > pinctr_update(const struct ovsdb_idl *idl, const char *br_int_name) > { > pinctrl_set_br_int_name_(br_int_name); > > ... > // Update the pinctrl.mac_binding_can_timestamp here. > ... > } > As discussed offline I have used the pinctrl_update suggestion and moved the call to ovn-controller in v3. > > > +{ > > + if (!txn) { > > + return; > > + } > > + > > + struct ovsdb_idl *idl = ovsdb_idl_txn_get_idl(txn); > > + bool current = > sbrec_server_has_mac_binding_table_col_timestamp(idl); > > + > > + if (current != pinctrl.mac_binding_has_timestamp_column) { > > + pinctrl.mac_binding_has_timestamp_column = current; > > + > > + /* Notify pinctrl_handler that mac binding timestamp column > > + * availability has changed. */ > > + notify_pinctrl_handler(); > > + } > > +} > > + > > void > > pinctrl_set_br_int_name(char *br_int_name) > > { > > @@ -3563,6 +3585,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > const struct shash *local_active_ports_ras) > > { > > ovs_mutex_lock(&pinctrl_mutex); > > + pinctrl_set_sb_mac_binding_has_timestamp_column(ovnsb_idl_txn); > > pinctrl_set_br_int_name_(br_int->name); > > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > > sbrec_port_binding_by_key, > > @@ -4245,12 +4268,17 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > b = sbrec_mac_binding_insert(ovnsb_idl_txn); > > sbrec_mac_binding_set_logical_port(b, logical_port); > > sbrec_mac_binding_set_ip(b, ip); > > - sbrec_mac_binding_set_mac(b, mac_string); > > sbrec_mac_binding_set_datapath(b, dp); > > - sbrec_mac_binding_set_timestamp(b, time_wall_msec()); > > - } else if (strcmp(b->mac, mac_string)) { > > + } > > + > > + if (strcmp(b->mac, mac_string)) { > > sbrec_mac_binding_set_mac(b, mac_string); > > - sbrec_mac_binding_set_timestamp(b, time_wall_msec()); > > + > > + /* For backward compatibility check if timestamp column is > available > > + * in SB DB. */ > > + if (pinctrl.mac_binding_has_timestamp_column) { > > + sbrec_mac_binding_set_timestamp(b, time_wall_msec()); > > + } > > } > > } > > > > Thanks, > Dumitru > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
