Sounds good to me. Ethan
On Mon, Mar 21, 2011 at 2:08 PM, Ben Pfaff <[email protected]> wrote: > On Mon, Mar 21, 2011 at 01:53:23PM -0700, Ethan Jackson wrote: >> > +/* Returns an arbitrary interface within 'port'. */ >> > +static struct iface * >> > +port_get_an_iface(const struct port *port) >> > +{ >> > + ? ?return CONTAINER_OF(port->ifaces.next, struct iface, port_elem); >> > +} >> > + >> >> I think this function works, but I don't like it very much. If think >> it would be stylistically cleaner if it was something like the >> following. >> >> if (port->n_ifaces > 0) { >> return CONTAINER_OF(list_front(&port->ifaces), struct iface, port_elem); >> } else { >> return NULL; >> } >> >> If we are comfortable asserting that port->n_ifaces is always greater >> than 0 then we can omit the if conditional. I still prefer using >> list_front as it's slightly easier to read and verify it's obvious >> correctness. >> That said I don't think this comment should block merging, so if you >> disagree ignore. > > port->n_ifaces > 0 pretty much always has to be true. I'm happy to > port->assert-fail or segfault when it isn't. > > I applied the following incrementally. It caused const-ness warnings > so I'll send out separate patches to fix that. > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 8983d22..0a6501a 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -2580,11 +2580,13 @@ port_get_active_iface_tag(const struct port *port) > : port->no_ifaces_tag); > } > > -/* Returns an arbitrary interface within 'port'. */ > +/* Returns an arbitrary interface within 'port'. > + * > + * 'port' must have at least one interface. */ > static struct iface * > port_get_an_iface(const struct port *port) > { > - return CONTAINER_OF(port->ifaces.next, struct iface, port_elem); > + return CONTAINER_OF(list_front(&port->ifaces), struct iface, port_elem); > } > > static void > @@ -3474,7 +3476,7 @@ bond_unixctl_list(struct unixctl_conn *conn, > ds_put_format(&ds, "%s\t%s\t%s\t", br->name, port->name, > bond_mode_to_string(port->bond_mode)); > LIST_FOR_EACH (iface, port_elem, &port->ifaces) { > - if (&iface->port_elem != port->ifaces.next) { > + if (&iface->port_elem != list_front(&port->ifaces)) { > ds_put_cstr(&ds, ", "); > } > ds_put_cstr(&ds, iface->name); > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
