[ovs-dev] [PATCH] Added handling of previously ignored cfm faults.
Ben Pfaff
blp at nicira.com
Thu Apr 5 22:27:38 UTC 2012
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 <mmahajan at 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()?
More information about the dev
mailing list