On second thought, I have a tidier proposal. I will send a follow up.
On 6 November 2013 10:03, Joe Stringer <[email protected]> wrote: > Right---there's two separate issues; One is that flaps are only > counted when logging is on, and the other is that faults are only > logged when the connection flaps (rather than on any difference in > error). Currently, old_cfm_fault is a bool while cfm->fault is not; > this makes it harder to follow. > > I propose keeping the patch as is, then following up with another for > the second issue:- > > --- a/lib/cfm.c > +++ b/lib/cfm.c > @@ -395,7 +395,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex) > if (timer_expired(&cfm->fault_timer)) { > long long int interval = cfm_fault_interval(cfm); > struct remote_mp *rmp, *rmp_next; > - bool old_cfm_fault = cfm->fault; > + enum cfm_fault_reason old_cfm_fault = cfm->fault; > bool demand_override; > bool rmp_set_opup = false; > bool rmp_set_opdown = false; > @@ -481,7 +481,9 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex) > } > > if (old_cfm_fault != cfm->fault) { > - cfm->flap_count++; > + if (old_cfm_fault == 0 || cfm->fault == 0) { > + cfm->flap_count++; > + } > > if (!VLOG_DROP_INFO(&rl)) { > struct ds ds = DS_EMPTY_INITIALIZER; > > On 6 November 2013 09:41, Alex Wang <[email protected]> wrote: >> Thanks very much for spotting this. >> >> Just one thing, >> >>> Signed-off-by: Joe Stringer <[email protected]> >>> --- >>> --- >>> lib/cfm.c | 27 +++++++++++++-------------- >>> 1 file changed, 13 insertions(+), 14 deletions(-) >>> >>> diff --git a/lib/cfm.c b/lib/cfm.c >>> index d256a5f..01c9a8c 100644 >>> --- a/lib/cfm.c >>> +++ b/lib/cfm.c >>> @@ -480,20 +480,19 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex) >>> cfm->fault |= CFM_FAULT_RECV; >>> } >>> >>> - if (old_cfm_fault != cfm->fault && !VLOG_DROP_INFO(&rl)) { >>> - struct ds ds = DS_EMPTY_INITIALIZER; >>> - >>> - ds_put_cstr(&ds, "from ["); >>> - ds_put_cfm_fault(&ds, old_cfm_fault); >>> - ds_put_cstr(&ds, "] to ["); >>> - ds_put_cfm_fault(&ds, cfm->fault); >>> - ds_put_char(&ds, ']'); >>> - VLOG_INFO("%s: CFM faults changed %s.", cfm->name, >>> ds_cstr(&ds)); >>> - ds_destroy(&ds); >>> - >>> - /* If there is a flap, increments the counter. */ >>> - if (old_cfm_fault == false || cfm->fault == false) { >>> - cfm->flap_count++; >>> + if (old_cfm_fault != cfm->fault) { >>> + cfm->flap_count++; >>> + >>> + if (!VLOG_DROP_INFO(&rl)) { >>> + struct ds ds = DS_EMPTY_INITIALIZER; >>> + >>> + ds_put_cstr(&ds, "from ["); >>> + ds_put_cfm_fault(&ds, old_cfm_fault); >>> + ds_put_cstr(&ds, "] to ["); >>> + ds_put_cfm_fault(&ds, cfm->fault); >>> + ds_put_char(&ds, ']'); >>> + VLOG_INFO("%s: CFM faults changed %s.", cfm->name, >>> ds_cstr(&ds)); >>> + ds_destroy(&ds); >>> } >>> } >> >> >> >> We still want to log the "fault <-> fault" change (though it is not counted >> as a flap). So, could you move the "if (!VLOG_DROP_INFO(&rl)) " outside? _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
