For future proofing I set if_cfg to NULL after free()ing it btw. Ethan
> 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
