-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #69: Coverity defects #70
Conversation
…nt in PayloadConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting one minor grammar issue and asking a question regarding a possible larger refactoring in the future
I traced the remoteAddress parameter all the way to the RelpConversion object which forwards the message. The parameter wasn't actually used at all. The address is only gotten from the RelpConfig object at the start of execution and placed in RelpConnectionFactory, which ultimately gives it to the RelpConversion instead of the message itself. That means that taking the remoteAddress from the message or the channel wasn't necessary at all, so I removed the whole thing. |
Fixed Coverity defects 460538 and 452917.
The first (460538) was a simple useless variable assignment in PayloadConfig.
The second (452917) was a possible NPE in HttpServerHandler.
ChannelHandlerContext.remoteAddress()
might return null if the channel is not connected. I noticed that there is a check for this in the classisActive()
so I used that and logged the error and returned. This might not be the best way to deal with the situation, maybe an error should be thrown instead? Furthermore, Coverity might not see that the NPE is dealt with this fix so it needs to be solved manually from Coverity after merging.In #69, fixing was specifically asked for 461131. This was a possible NullPointerException in
RelpConversion
'ssendMessage()
, but the function call is already surrounded by a try catch as per #7, so in my opinion a null check for this is a bit overkill and clutters the code more. There is also no threat of a null happening there yet with the current Supplier (RelpConnectionFactory
), but this might of course change in the future.Went over the rest of the defects in Coverity as well, and I don't think they are necessary to "fix", I think they are false positives mostly.