On Tue, Apr 19, 2011 at 10:49:22AM -0700, Ethan Jackson wrote:
> > It seems to me that there are several instances of code that is
> > something like
> > ? lacp_time == LACP_TIME_CUSTOM ? lacp->custom_time
> > ? : lacp_time == LACP_TIME_FAST ? LACP_FAST_TIME_TX
> > ? : LACP_SLOW_TIME_TX
> > I'd suggest using a helper function or a member in struct lacp to
> > avoid the repetition. ?Even two helpers or members, if you want both rx
> > and tx rates.
>
> I actually thought about doing this, but it turns out that we only do
> this in the code once. There are three sections of the code I think
> you are referencing. The first is a switch statement in
> lacp_process_pdu which does exactly as you describe. The second is in
> lacp_run(). It's similar to what you describe, however if the timing
> mode isn't LACP_TIME_CUSTOM then it chooses FAST or SLOW time based on
> its *partners* state, not based on the lacp_time field in the lacp
> structure. The third is in slave_set_expired() which uses always uses
> LACP_FAST_TIME unless in custom timing mode. Each case is different
> enough that I don't think they can be cleanly shoved into one
> function. I may be wrong though, I can think about it more.
How about helpers something like this:
static long long int
tx_duration(const struct lacp *lacp, bool fast)
{
return (lacp->lacp_time == LACP_TIME_CUSTOM ? lacp->custom_time
: fast ? LACP_FAST_TIME_TX
: LACP_SLOW_TIME_TX);
}
static long long int
rx_duration(const struct lacp *lacp, bool fast)
{
return rx_duration(lacp, fast) * LACP_RX_MULTIPLIER;
}
and then in lacp_process_pdu():
tx_rate = tx_duration(lacp, lacp->lacp_time == LACP_TIME_FAST);
and then in lacp_run():
duration = tx_duration(lacp, slave->partner.state & LACP_STATE_TIME);
and in slave_set_expired():
duration = rx_duration(lacp, lacp->lacp_time == LACP_TIME_FAST);
timer_set_duration(&slave->rx, duration);
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
