Ingo Schwarze wrote: > Hi, > > Stefan Hagen wrote on Sat, Sep 11, 2021 at 09:00:18AM +0200: > > > Is this better? > > No, it is no good whatsoever. > > * It introduces yet another pointless static buffer. > * It pointlessly uses snprintf(3) where printf(3) > is obviously sufficient. > * It introduces error checking for operations > that obviously cannot fail when done properly. > * It needlessly does pointer arithmetic. > > So far, so bad, but the worst part is that it apparently wasn't > tested on a 64-bit platform (a platform where sizeof(long) == 64, > for example amd64). On such a platform, there is another bug on > the very same line. The function binfmtl() has platform-dependent > behaviour and does not behave as intended on a 64-bit platform and > prints 32 spurious zero-digit characters, breaking the desired > output alignment and making the content of the output > self-contradictory on top of that. > > Rather than try to change binfmtl(), which is mostly pointless > in the first place, let's just trivially inline the desired > functionality, it's just two lines of very simple C code.
I really appreciate you (and Theo) taking the time to explain these things. I noticed the weird output, but couldn't make sense of it as the upstream version crashes (abort trap) here. Regarding my terrible C skills, I think I'll step back from trying to fix C code for a bit. I'm wasting peoples time here to educate me on topics I could as well learn elsewhere. (Except you tell me to continue submitting bad fixes and learn here...). I feel embarrassed, because I can't even explain why I used snprintf there. Your fix looks good to me. But my opinion doesn't matter at this point. Thanks, Stefan
