Fightwarn: huawei-ups2000#1216
Conversation
…t, to match ser_get_buf()
… int to size_t, to match ser_send_buf()
…isters(), ups2000_device_identification(): initialize variables that clang-9 is not sure about Seems like a clang analyzer bug frankly: code starts with a retrying loop that would initialize these variables. But still, adding determinism into code never hurts ;)
…k and cast crc16 calculations
…2_t calculations of ups2000_ident
…tions of reg_id parts
… ssize_t vs size_t increments
…ngs for non-fixed format string
…t all_alarms_len from snprintf() retval
…_write_register() and handler_func()
jimklimov
added
modbus
refactor/fightwarn
|
This pull request introduces 2 alerts when merging e9f02e2 into 218c6aa - view on LGTM.com new alerts:
|
|
Note: no worry about the LGTM warnings for range-checks, it only builds for "one true bitness" so sees no real-life issues those if's try to handle, like elsewhere. |
jimklimov
added
the
CI
|
@whc2001 @biergaizi : I would be grateful if you can build and test these changes on hardware, that there is no regression (any bits lost to type-casting, etc.) UPDATE: Should be part of NUT master branch now |
|
Gonna try it later! |
|
Just compiled and installed the latest version, tried |
|
What is the compiler version and their options for reproducing these warnings? I always use Some are indeed quite serious bugs. For example, assigning a negative default value to an unsigned integer. If I've been warned by the compiler, I would never submit my code to the project until it's fixed. Last but not least, while I agree that some implicit type conversions are also serious bugs, but I disagree on how they should be fixed. The current fixes use a lot of forced type casts everywhere, making the code difficult to read. I think clarity can be improved by changing some of the variable types at their definitions, so we can avoid these forced casts altogether. I'll open a pull request for alternative fixes if I am able to reproducing these warnings. |
|
For warnings - you can check the configure script options (or .ac code), it
defines several levels and so far NUT master passes the easiest set and
gets close to passing the second level (gotta score the usb/shut code
remaining, in progress).
For fixes, PRs with educated guesses to fix would be greatly appreciated!
But in many cases (I'm on it for a year and a few thousand commits by now)
it does boil down to different system header declarations between OSes and
third party libs evolution, so some range-checking and casts is the best we
can do, especially in code for devices we can't test directly. There was a
huge chunk of changes to match int sizes for some known wire protocols,
signedness fixes, size_t or sometimes ssize_t instead of int to match real
system function return types and buflens, etc.
And for a few cases, where code intentionally builds formatting strings as
variables, turning a "pragma"tic eye to that seems the best (at least
quick) approach. It should be no worse than original state that just did
not complain at lower warning levels.
When the codebase does not emit thousands of warnings, it becomes feasible
to fix things by looking at what compilers do catch, and especially at what
changes with iterations... so some madness had to be accepted (hopefully
for a short while).
So, if you have better fixes to propose (and especially able to test) -
feel very welcome to PR them :)
…On Thu, Dec 9, 2021, 14:36 biergaizi ***@***.***> wrote:
What is the compiler version and their options for reproducing these
warnings? I always use the CFLAGS=-Wall -Wextra and the latest GCC and
Clang versions to compile my code, but I didn't receive any of these
warnings. The situation is quite confusing. Does the build system suppress
the warnings by default? Or is it because old compilers generate more false
positives than new complies?
Some are indeed quite serious bugs. For example, assigning a negative
default value to an unsigned integer. If I've been warned by the compiler,
I would never submit my code to the project.
Last but not least, I agree that some implicit type conversions are
serious bugs, but I disagree on how they should've be fixed. The current
fixes use a lot of forced type casts everywhere, making the code difficult
to read. I think clarify can be improved by changing some of the variable
types at their definitions, so we can avoid these forced casts altogether.
Also the suppression of string formatting security warnings is the most
ugly workaround so far. I wonder if there's a better way to do that.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#1216 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMPTFERDZHXSV5F63UXJJ3UQCWHFANCNFSM5JPQDB2Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
3 participants
CC @biergaizi (so split from #1215 and earlier PRs with other similar fixes): Fixing CI warnings for the new huawei-ups2000 driver from PR #1198, #1066 and #954 for issue #1017
I don't agree that all of those make sense (e.g. initialization of variables does seem like a false-positive alert), but many of those do point to mismatches.
Anyhow, these warnings would be a blocker after dialing up the default level of warnings in CI, which I'm getting close to for the goals of "Fightwarn effort" #823, so gotta get fixed or quiesced otherwise.