Yep it doesn't re-use it. I'll push it in a second, thanks for the thorough review.
Ethan On Mon, Apr 23, 2012 at 18:45, Ben Pfaff <[email protected]> wrote: > On Mon, Apr 23, 2012 at 01:45:24AM -0700, Ethan Jackson wrote: >> > This looks more or less good-to-go to me. Thank you! >> > >> > (How much have you tested it? What kinds of tests did you do?) >> >> I mostly did basic functional testing. Created a bunch of ports, destroyed >> them. Created bridges named for ports on other bridges. All the testing I >> did >> used valgrind to look for leaks. I also tested the conf.db which caused >> vswitchd to infinite loop to verify that it doesn't. >> >> > The ofpp_garbage list could just be a dynamic array of "uint16_t"s, >> > although the current representation is OK too. >> >> I thought about that when implementing it. I think lists are a bit easier to >> deal with, at this point I'd rather leave it cause I want to get this in. >> >> > It would work out more cleanly if we changed the "no port yet" >> > ofp_port parameter value to be OFPP_NONE, or if we changed the >> > semantics of the ofproto_port_add() output parameter to use -1. If >> > you don't make this change, though, please do notice that both the >> > function parameter and the inner block variable are named 'ofp_port' >> > currently. >> >> I find the current code a bit easier to reason about as all of the iface >> initialization is done in one place. I changed the name in the block to >> new_ofp_port. >> >> The following is an incremental: > > Thanks, it looks good. The one thing that I thought about verifying, > but didn't (because it isn't convenient at the moment), was that > iface_create() didn't use if_cfg after freeing it. > > Please push this when you are satisfied with it. > > Thanks again, > > Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
