Thanks this version is much closer. Just some minor points which
should be pretty easy to clean up. Ben, so we don't have a 12 hour
review cycle on this, would you mind looking at the next version of
this patch (which should be pretty much there) and applying it if it's
ready?
>>> + /* Always resets decay_min_rx when cfg_min_rx is updated. */
>>> + bfd->decay_min_rx = 0;
>>> + bfd_poll(bfd);
I think the intention is right here, but it's a bit odd. For example,
if in_decay is true, we don't drop out of it with this logic in all
cases. How about we add a boolean to this function
"cfm_min_rx_changed" and set it to true here. If that's true, then in
the next if statement we can run through the decay_min_rx changed
logic which we know is correct.
>>> + if (bfd->state == STATE_UP && bfd->decay_min_rx > 0
>>> + && now >= bfd->decay_detect_time) {
>>> + bfd_try_decay(bfd);
>>> + }
I'm not sure the bfd_try_decay() function is giving us a lot. This is
the only caller and it's fairly short. What do you think about simply
moving it's logic into this if block? If you feel strongly about it,
it's fine as is, just a style nit.
One thing, we should probably reset the decay_detect_time when we
first enter STATE_UP. Otherwise our time in other states counts to
the detect time.
>>> - ovs_strlcpy(flag_str, ds_cstr(&ds), sizeof flag_str);
>>> + /* Do not copy the trailing whitespace. */
>>> + ovs_strlcpy(flag_str, ds_cstr(&ds), ds.length);
This probably belongs in a different patch. Also have a look at the
ds_chomp() function which I think is a bit cleaner for this sort of
thing.
>>> + measure = (bfd->decay_min_rx < 2000 ? 2000 : bfd->decay_min_rx)
>>> + / bfd->min_rx + 5;
This is awfully complicated. Any particular reason we don't simply do
something like MAX(bfd->decay_min_rx, 2000)? It certainly would be
easier to explain to people. If not, could we add a comment
explaining this formula and how we arrived at it?
Thanks,
Ethan
X-CudaMail-Whitelist-To: dev@openvswitch.org
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
