Looks good to me. Just a bit of nit picking.
Ethan
> +/* 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.
> + LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> + if (&iface->port_elem != port->ifaces.next) {
> ds_put_cstr(&ds, ", ");
> }
I would be inclined to use list_front here as well.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
