On Mon, Jun 11, 2018 at 07:17:13PM -0700, Han Zhou wrote: > On Mon, Jun 11, 2018 at 3:14 PM, Ben Pfaff <b...@ovn.org> wrote: > > +2. Pass the index, an iteration variable, and the key values to the > iterator. > > s/key values/index row objects
Thanks, fixed. > > +void > > +ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *cursor, > > + struct ovsdb_idl_index *index) > > A minor comment: the signature of this function looked confusing because of > the "index" parameter. Does it make sense to store the "index" pointer to > struct ovsdb_idl_cursor, so that when traversing with cursor we only need > the cursor parameter. I was undecided about this. I don't think it's actually that confusing because the only users are encapsulated within macros. But I made the change and I don't think it's a big deal either way. > > +static struct ovs_list * > > +bfd_find_ha_gateway_chassis( > > + struct ovsdb_idl_index *sbrec_port_binding_by_datapath, > > + const struct chassis_index *chassis_index, > > + const struct sbrec_datapath_binding *datapath) > > +{ > > + struct sbrec_port_binding *target = > sbrec_port_binding_index_init_row( > > + sbrec_port_binding_by_datapath); > > + sbrec_port_binding_index_set_datapath(target, datapath); > > + > > + struct ovs_list *ha_gateway_chassis = NULL; > > + const struct sbrec_port_binding *pb; > > + SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, > > + sbrec_port_binding_by_datapath) { > > + if (strcmp(pb->type, "chassisredirect")) { > > + continue; > > + } > > + > > + struct ovs_list *gateway_chassis = gateway_chassis_get_ordered( > > + pb, chassis_index); > > + if (!gateway_chassis || ovs_list_is_short(gateway_chassis)) { > > + /* We don't need BFD for non-HA chassisredirect. */ > > + gateway_chassis_destroy(gateway_chassis); > > + continue; > > + } > > + > > + ha_gateway_chassis = gateway_chassis; > > + break; > > + } > > + sbrec_port_binding_index_destroy_row(target); > > + return ha_gateway_chassis; > > +} > > This is a good refactoring and it is functionally equal to the original > one, but I wonder if there is a problem even with the original logic. It > breaks out whenever the first logical router port is found with multiple > gateways chassises, but what if there are still some other logical router > ports on the same logical router are gateway ports and on different > gateways, would those gateways be missed in the bfd sessions? (Of course, > it shouldn't belong to this patch even if it is a real problem.) I'll ask Guru. > All the comments are minor, so: > > Acked-by: Han Zhou <hzh...@ebay.com> Thanks! _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev