Tuesday, January 04, 2022
Fixing bugs by cleaning code
I figured out an approach to “Project: Bradenburg.” I'm dumping the C++ code as it was never used in production and as someone who is more confortable with C than C++, I think that's the best choice right now. To that end, I've made a checklist of the items that need addressing right now—and they're all related to cleaning up the code.
I've normalized the whitespace in all the source files.
Tab characters were introduced by the previous developer,
who used a different tab stop setting than I'm used to,
so the code formatting is just way off.
I removed the #pragma declarations and fixed all the resulting warnings.
And for the warnings I don't necessarily agree with,
like -Wformat-truncation,
I can supress in the makefile
(well, GNUmakefile if you want to be pedantic):
src/app/stormgr.o : override CFLAGS += -Wno-format-truncation
This disables the warning
src/app/stormgr.c:194:3: note: ‘snprintf’ output between 2 and 4097 bytes into a destination of size 4096
from code like this:
snprintf(filename, sizeof(filename),"%s/", GetGlobalConfig()->spool_dir);
It's nice I get the warning, but in this case, if we get a filename of 4097 characters long we have other issues to worry about.
And that's pretty much the only warning I have to supress. The other warnings were all issues that needed to be addressed, and in one case, an actual bug was fixed.
Right now I'm cleaning up what is,
to me,
a pointless abstraction dealing with syslog().
The previous developer wrote a wrapper around syslog() called SysLog().
It would be one thing if there was some reason to wrap syslog(),
like we would be running on Windows
(we won't)
or needed to redirect output to a file
(we don't).
But no,
all the wrapper does is:
void SysLog(int priority, const char *fmt, ...)
{
va_list marker;
if (fSyslogOpened)
{
va_start(marker, fmt);
vsyslog(priority, fmt, marker);
va_end(marker);
}
}
And the funny thing—syslog() already handles the case when openlog() hasn't been called.
So there's no reason for this wrapper whatsoever in this code base.
What makes this even more special is how the developer called SysLog():
SysLog(LOG_NOTICE, BRADENBURG5024, dbuf);
And elsewere in the code in some header file:
#define BRADENBURG5024 "BRADENBURG5024: OUTBIND connection accepted from %s"
I … um … I … erm … yeah. I can make sense of this, in the “we might want to translate the log messages to another language” argument. But we don't sell this product—it's for internal use. And there are other ways to go about doing this rather than separate the format string from its use. Nothing like finding nine instances of this:
SysLog(LOG_EMERG, BRADENBURG0001);
where BRADENBURG0001 is defined as:
#define BRADENBURG0001 "BRADENBURG0001: %s"
For those unwise in the ways of C programming, this is calling a function with effectively a missing parameter and the compiler can't warn about it because the format string (which informs the called function about what parameters to expect) exists in a different file.
So just by cleaning up the code and removing pointless abstractions I'm fixing bugs.
