[ovs-dev] [PATCH v2 1/5] ovsdb-idl: Redesign use of indexes.

Ben Pfaff blp at ovn.org
Tue Jun 12 15:21:34 UTC 2018


On Mon, Jun 11, 2018 at 07:17:13PM -0700, Han Zhou wrote:
> On Mon, Jun 11, 2018 at 3:14 PM, Ben Pfaff <blp at 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 <hzhou8 at ebay.com>

Thanks!


More information about the dev mailing list