On Mon, Jun 11, 2018 at 07:17:13PM -0700, Han Zhou wrote:
> On Mon, Jun 11, 2018 at 3:14 PM, Ben Pfaff <[email protected]> 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 <[email protected]>
Thanks!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
