[ovs-dev] [PATCH v2 2/2] bridge: Refactor bridge_reconfigure().

[Ethan Jackson]

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 <blp at nicira.com> 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.