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
