gPTP - add some NULL pointer checking #471
Conversation
…E_EVENT (could be NULLed out by sync timeout
| @@ -598,7 +598,8 @@ void IEEE1588Port::processEvent(Event e) | |||
|
|
|||
| case STATE_CHANGE_EVENT: | |||
| if (!automotive_profile) { // BMCA is not active with Automotive Profile | |||
| if ( clock->getPriority1() != 255 ) { | |||
| if ( clock->getPriority1() != 255 && qualified_announce != NULL ) { | |||
There was a problem hiding this comment.
I think checking qualified_announce here is redundant; it's set right before STATE_CHANGE_EVENT is queued for this port and it's also checked below when calculateERBest() is called on each port in your addition below. I can't see that it's accessed between here and the check below.
There was a problem hiding this comment.
Agree with @pinealservo there's not really any way I could see that qualified_announce would be NULL. If you want to check for NULL, I think it would be better to check the value of EBest, but still not necessary.
There was a problem hiding this comment.
This line was added due to null pointer faults (gPTP crashes) we observed in-house. I see that EBest checks were added here on the same day, so perhaps additional unnecessary checks were added. I'll update with this commit removed.
|
I think the logic covers all the possibilities that could introduce badness in a multi-port implementation. I thought there was a missing one and made a comment about it, but I was mistaken and deleted it; if you get an email and can't find the reference, that's why. If you'd like to leave the possibly-redundant check, I don't think it hurts anything and I may have missed the reason for it, so I won't object. |
…TE_CHANGE_EVENT (could be NULLed out by sync timeout" This reverts commit 23b79a5.
|
I think I have now addressed all comments made by @pinealservo and @christopher-s-hall |
|
Looks good to me! |
This pull request adds some NULL pointer checking related to announce processing.