[ovs-dev] [PATCH] Added handling of previously ignored cfm faults.

[Ben Pfaff]

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()?