* * * * *

                         Fixing bugs by cleaning code

I figured out an approach to “Project: Bradenburg [1].” 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 [2] and fixed all the resulting warnings. And for the
warnings I don't necessarily agree with, like -Wformat-truncation [3], I can
supress in the makefile (well, GNUmakefile if you want to be pedantic):

-----[ make ]-----
src/app/stormgr.o : override CFLAGS += -Wno-format-truncation
-----[ END OF LINE ]-----

This disables the warning

-----[ sh ]-----
src/app/stormgr.c:194:3: note: ‘snprintf’ output between 2 and 4097 bytes into a destination of size 4096
-----[ END OF LINE ]-----

from code like this:

-----[ C ]-----
snprintf(filename, sizeof(filename),"%s/", GetGlobalConfig()-­>spool_dir);
-----[ END OF LINE ]-----

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:

-----[ C ]-----
void SysLog(int priority, const char *fmt, ...)
{
 va_list marker;

 if (fSyslogOpened)
 {
   va_start(marker, fmt);
   vsyslog(priority, fmt, marker);
   va_end(marker);
 }
}
-----[ END OF LINE ]-----

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():

-----[ C ]-----
SysLog(LOG_NOTICE, BRADENBURG5024,  dbuf);
-----[ END OF LINE ]-----

And elsewere in the code in some header file:

-----[ C ]-----
#define BRADENBURG5024    "BRADENBURG5024: OUTBIND connection accepted from %s"
-----[ END OF LINE ]-----

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:

-----[ C ]-----
SysLog(LOG_EMERG, BRADENBURG0001);
-----[ END OF LINE ]-----

where BRADENBURG0001 is defined as:

-----[ C ]-----
#define BRADENBURG0001    "BRADENBURG0001: %s"
-----[ END OF LINE ]-----

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.

[1] gopher://gopher.conman.org/0Phlog:2021/12/10.1
[2] gopher://gopher.conman.org/0Phlog:2021/12/15.1
[3] https://gcc.gnu.org/onlinedocs/gcc-11.1.0/gcc/Warning-Options.html#index-Wformat-truncation

Email author at [email protected]

You are viewing proxied material from gopher.conman.org. The copyright of proxied material belongs to its original authors. Any comments or complaints in relation to proxied material should be directed to the original authors of the content concerned. Please see the disclaimer for more details.