On Wed, Aug 21, 2013 at 2:26 PM, Ethan Jackson <et...@nicira.com> wrote:
> This patch is basically ready, two minor comments and we'll get it merged > today. > > First the title line should be changed to "bfd: Implement > forwarding_if_rx." Note the period. > > Yes, how could I miss that. > > > > - return bfd->state == STATE_UP > > - && bfd->rmt_diag != DIAG_PATH_DOWN > > - && bfd->rmt_diag != DIAG_CPATH_DOWN > > - && bfd->rmt_diag != DIAG_RCPATH_DOWN; > > + return (bfd->forwarding_if_rx > > + ? bfd->forwarding_if_rx_detect_time > time_msec() > > + : bfd->state == STATE_UP) > > + && bfd->rmt_diag != DIAG_PATH_DOWN > > + && bfd->rmt_diag != DIAG_CPATH_DOWN > > + && bfd->rmt_diag != DIAG_RCPATH_DOWN; > > The indentation here isn't entirely correct. > > Also the logic doesn't seem entirely correct. I think we want > something more like bfd->state == STATE_UP || (bfd->forwarding_if_rx > && bfd->forwarding_if_rx_detect_time > time_msec()) I.E. if the state > is up, we should be up no matter what forwarding if rx says. > Makes sense, I'll fix accordingly, > Btw this code reminds me, there's a bug with this logic which Paul > fixed in the CFM code a while back. We should probably port the fix > to BFD as well (in a separate patch). Could you have a look at it > after this is merged? > > Absolutely!
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
