On Thu, Apr 05, 2012 at 03:12:21PM -0700, Mehak Mahajan wrote: > The CFM packets that are out of sequence or contain invalid cfm_interval were > previously not ignored. The behavior is changed with this patch to not > process those CFM frames. > > Signed-off-by: Mehak Mahajan <mmaha...@nicira.com>
I guess Ethan ought to look at this too, but... Please define cfm_fault as "enum cfm_fault_reason" for documentation purposes: > - bool fault = false; > + int cfm_fault = 0; The other stuff I noticed is preexisting so it doesn't really affect your changes but it would be good to consider updating. I guess we should change the type of the "fault" and "recv_fault" members of struct cfm to enum cfm_fault_reason. Here I wonder whether "invalid" should be written "unexpected". The former implies that the message is malformed, the latter only that the value isn't what we expect: > VLOG_WARN_RL(&rl, "%s: received a CCM with an invalid interval" > " (%"PRIu8") from RMP %"PRIu64, cfm->name, > ccm_interval, ccm_mpid); > - fault = true; Ditto here: > VLOG_WARN_RL(&rl, "%s: received a CCM with an invalid extended" > " interval (%"PRIu16"ms) from RMP %"PRIu64, > cfm->name, > ccm_interval_ms_x, ccm_mpid); > - fault = true; > } > > rmp = lookup_remote_mp(cfm, ccm_mpid); It looks like rdi gets updated on every CCM packet receive, so that if we receive a CCM without RDI after one with it between fault checks, cfm_run() won't report the RDI. I wonder whether we should actually only reset rdi to false in cfm_run() and only set it to true, never to false, in cfm_process_heartbeat()? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
