WIP: Fixed various warnings and errors identified by cppcheck#810
WIP: Fixed various warnings and errors identified by cppcheck#810jimklimov merged 6 commits intonetworkupstools:masterfrom
Conversation
|
I don't know this codebase at all, but I'm a proficient C programmer, and am pretty sure of most of these changes. A few have a temporary "Sean" comment that should especially be double-checked. |
|
This pull request fixes 4 alerts when merging 2e49c7c8673d84c84e21ac244de943b5eabff0a0 into e24b9a8 - view on LGTM.com fixed alerts:
|
jimklimov
left a comment
jimklimov
left a comment
There was a problem hiding this comment.
Thank you very much for this PR, caught a lot of curious nuances. I marked as "Requested changes" mostly to have someone take a second look at some commented points, and possibly change or revert something in those. But generally, very much LGTM :)
drivers/nutdrv_qx_voltronic.c
Outdated
drivers/riello_ser.c
Outdated
There was a problem hiding this comment.
I think this got inherited from some loop where the bytes pointer got incremented so we read next portions into the buffer while we can. The read(..., size - readen) above points to this too - with the current code, readen is always zero until we increment it with now.
Probably the current routine can be simplified even further to drop readen altogether, return now here and return 0 in default exit below. But I do not know the legacy context behind it... @aquette @zykh @clepple : any memories on this one?
There was a problem hiding this comment.
Indeed this looks leftover from some old refactoring. I'm inclined not to attempt any simplifications here.
drivers/solis.c
Outdated
Specifically:
arrayIndexThenCheck,drivers/nutdrv_qx_voltronic-qs-hex.c:222,style,Array index 'i' is used before limits check.
arrayIndexThenCheck,drivers/oneac.c:176,style,Array index 'i' is used before limits check.
clarifyCalculation,drivers/belkinunv.c:1063,style,Clarify calculation precedence for '&' and '?'.
clarifyCalculation,drivers/belkinunv.c:922,style,Clarify calculation precedence for '&' and '?'.
clarifyCalculation,drivers/powercom.c:677,style,Clarify calculation precedence for '&' and '?'.
clarifyCalculation,drivers/powercom.c:732,style,Clarify calculation precedence for '&' and '?'.
clarifyCondition,drivers/asem.c:248,style,Boolean result is used in bitwise operation. Clarify expression with parentheses.
duplicateConditionalAssign,drivers/solis.c:725,style,The statement 'if (DaysOnWeek!=DaysOffWeek) DaysOnWeek=DaysOffWeek' is logically equivalent to 'DaysOnWeek=DaysOffWeek'.
duplicateExpression,clients/cgilib.c:49,style,Same expression on both sides of '||'.
duplicateExpression,drivers/nutdrv_qx_voltronic.c:270,style,Same expression on both sides of '||'.
duplicateExpression,drivers/nutdrv_qx_voltronic.c:303,style,Same expression on both sides of '||'.
duplicateExpression,drivers/nutdrv_qx_voltronic.c:397,style,Same expression on both sides of '||'.
identicalInnerCondition,drivers/tripplite_usb.c:579,warning,Identical inner 'if' condition is always true.
incorrectLogicOperator,drivers/dstate.c:1170,warning,Logical conjunction always evaluates to false: c2 && !c2.
incorrectLogicOperator,drivers/rhino.c:190,warning,Logical disjunction always evaluates to true: BattVoltage > 129 || BattVoltage < 144.
incorrectStringBooleanError,drivers/mge-utalk.c:899,warning,Conversion of string literal "\r\n" to bool always evaluates to true.
invalidPrintfArgType_sint,clients/upsset.c:393,warning,%d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
invalidPrintfArgType_sint,clients/upsset.c:676,warning,%d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
invalidPrintfArgType_sint,drivers/isbmex.c:178,portability,%d in format string (no. 1) requires 'int' but the argument type is 'ssize_t {aka signed long}'.
invalidPrintfArgType_sint,drivers/tripplitesu.c:435,warning,%d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
knownConditionTrueFalse,clients/upsmon.c:919,style,Condition 'un' is always true
knownConditionTrueFalse,drivers/gamatronic.c:111,style,Condition 'ret>=0' is always true
knownConditionTrueFalse,drivers/powercom.c:696,style,Condition 'battval>bat0' is always true
knownConditionTrueFalse,drivers/solis.c:485,style,Condition 'AppPower==0' is always false
knownConditionTrueFalse,tools/nut-scanner/scan_nut.c:180,style,Condition 'numa>=3' is always true
memleak,clients/upsset.c:664,error,Memory leak: val
nullPointerRedundantCheck,drivers/snmp-ups.c:1554,warning,Either the condition 'info_template==NULL' is redundant or there is possible null pointer dereference: info_template.
pointerLessThanZero,drivers/nut-libfreeipmi.c:864,style,A pointer can not be negative so it is either pointless or an error to check if it is.
pointerLessThanZero,drivers/nut-libfreeipmi.c:884,style,A pointer can not be negative so it is either pointless or an error to check if it is.
postfixOperator,clients/nutclient.cpp:896,performance,Prefer prefix ++/-- operators for non-primitive types.
redundantInitialization,drivers/al175.c:548,style,Redundant initialization for 'reply'. The initialized value is overwritten before it is read.
uninitMemberVar,clients/nutclient.cpp:504,warning,Member variable 'TcpClient::_timeout' is not initialized in the constructor.
uninitMemberVar,clients/nutclient.cpp:513,warning,Member variable 'TcpClient::_timeout' is not initialized in the constructor.
uninitvar,server/netssl.c:546,error,Uninitialized variable: ret
unsignedLessThanZero,drivers/apcsmart-old.c:928,style,Checking if unsigned expression 'sdtype' is less than zero.
unsignedPositive,drivers/bcmxcp.c:785,style,Unsigned expression 'commandByte' can't be negative so it is unnecessary to test it.
uselessAssignmentPtrArg,drivers/riello_ser.c:105,warning,Assignment of function parameter has no effect outside the function. Did you forget dereferencing it?
|
This pull request fixes 4 alerts when merging 1b6a214 into d667a7c - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging c2329b0 into e87f1b8 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 7a02f41 into ab955fd - view on LGTM.com fixed alerts:
|
|
Friendly ping... |
Drop temporary comment with unicode for PR
Drop temporary unicode comment for PR
|
This pull request fixes 4 alerts when merging e735db6 into 82ba170 - view on LGTM.com fixed alerts:
|
|
@jimklimov I could always remove the changes that haven't been approved so that we could at least merge some of this. perhaps I shouldn't have added so much to one PR... |
|
Nay, let's leave them be - and thanks a lot! I revisited this PR today, the changes around remaining questions looked reasonable after some more review of the context, and/or the original state was too bogus to keep. So on my side nothing blocks it at the moment. This also spurred the completion of CI setup to test a lot of compilers, OSes and standards to catch more warnings coming from different perspectives, so thanks for that too :) |
|
A version of this PR's changes (plus some from "fightbugs" branch) looks decent also on the BuildBot farm covering more Linux distros and BSDs. There is work left, but at least builds pass green and I think it warns a bit less than before :) |
|
Oh, I really didn't mean/want for this to be merged as-in, hence the "WIP" in the title. I had lots of temporary comments in there for discussion purposes only. |
|
Can you revert the merge? |
|
Hm, probably could do, but maybe better now would be to overlay with next fixes and/or comment removals? I think my Per https://github.com/networkupstools/nut/pull/810/files I don't see any questioning comments left in code, and the changes that did stick out as possible troublemakers were discussed and resolved. |
3 participants
Specifically:
arrayIndexThenCheck,drivers/nutdrv_qx_voltronic-qs-hex.c:222,style,Array index 'i' is used before limits check.
arrayIndexThenCheck,drivers/oneac.c:176,style,Array index 'i' is used before limits check.
clarifyCalculation,drivers/belkinunv.c:1063,style,Clarify calculation precedence for '&' and '?'.
clarifyCalculation,drivers/belkinunv.c:922,style,Clarify calculation precedence for '&' and '?'.
clarifyCalculation,drivers/powercom.c:677,style,Clarify calculation precedence for '&' and '?'.
clarifyCalculation,drivers/powercom.c:732,style,Clarify calculation precedence for '&' and '?'.
clarifyCondition,drivers/asem.c:248,style,Boolean result is used in bitwise operation. Clarify expression with parentheses.
duplicateConditionalAssign,drivers/solis.c:725,style,The statement 'if (DaysOnWeek!=DaysOffWeek) DaysOnWeek=DaysOffWeek' is logically equivalent to 'DaysOnWeek=DaysOffWeek'.
duplicateExpression,clients/cgilib.c:49,style,Same expression on both sides of '||'.
duplicateExpression,drivers/nutdrv_qx_voltronic.c:270,style,Same expression on both sides of '||'.
duplicateExpression,drivers/nutdrv_qx_voltronic.c:303,style,Same expression on both sides of '||'.
duplicateExpression,drivers/nutdrv_qx_voltronic.c:397,style,Same expression on both sides of '||'.
identicalInnerCondition,drivers/tripplite_usb.c:579,warning,Identical inner 'if' condition is always true.
incorrectLogicOperator,drivers/dstate.c:1170,warning,Logical conjunction always evaluates to false: c2 && !c2.
incorrectLogicOperator,drivers/rhino.c:190,warning,Logical disjunction always evaluates to true: BattVoltage > 129 || BattVoltage < 144.
incorrectStringBooleanError,drivers/mge-utalk.c:899,warning,Conversion of string literal "\r\n" to bool always evaluates to true.
invalidPrintfArgType_sint,clients/upsset.c:393,warning,%d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
invalidPrintfArgType_sint,clients/upsset.c:676,warning,%d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
invalidPrintfArgType_sint,drivers/isbmex.c:178,portability,%d in format string (no. 1) requires 'int' but the argument type is 'ssize_t {aka signed long}'.
invalidPrintfArgType_sint,drivers/tripplitesu.c:435,warning,%d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
knownConditionTrueFalse,clients/upsmon.c:919,style,Condition 'un' is always true
knownConditionTrueFalse,drivers/gamatronic.c:111,style,Condition 'ret>=0' is always true
knownConditionTrueFalse,drivers/powercom.c:696,style,Condition 'battval>bat0' is always true
knownConditionTrueFalse,drivers/solis.c:485,style,Condition 'AppPower==0' is always false
knownConditionTrueFalse,tools/nut-scanner/scan_nut.c:180,style,Condition 'numa>=3' is always true
memleak,clients/upsset.c:664,error,Memory leak: val
nullPointerRedundantCheck,drivers/snmp-ups.c:1554,warning,Either the condition 'info_template==NULL' is redundant or there is possible null pointer dereference: info_template.
pointerLessThanZero,drivers/nut-libfreeipmi.c:864,style,A pointer can not be negative so it is either pointless or an error to check if it is.
pointerLessThanZero,drivers/nut-libfreeipmi.c:884,style,A pointer can not be negative so it is either pointless or an error to check if it is.
postfixOperator,clients/nutclient.cpp:896,performance,Prefer prefix ++/-- operators for non-primitive types.
redundantInitialization,drivers/al175.c:548,style,Redundant initialization for 'reply'. The initialized value is overwritten before it is read.
uninitMemberVar,clients/nutclient.cpp:504,warning,Member variable 'TcpClient::_timeout' is not initialized in the constructor.
uninitMemberVar,clients/nutclient.cpp:513,warning,Member variable 'TcpClient::_timeout' is not initialized in the constructor.
uninitvar,server/netssl.c:546,error,Uninitialized variable: ret
unsignedLessThanZero,drivers/apcsmart-old.c:928,style,Checking if unsigned expression 'sdtype' is less than zero.
unsignedPositive,drivers/bcmxcp.c:785,style,Unsigned expression 'commandByte' can't be negative so it is unnecessary to test it.
uselessAssignmentPtrArg,drivers/riello_ser.c:105,warning,Assignment of function parameter has no effect outside the function. Did you forget dereferencing it?