It’s okay to hate the 12 year old boys who write the bulk of OpenSource code
By: Date: February 22, 2018 Categories: Experience,Information Technology,Thank You Sir May I Have Another Tags: , , , ,
logger snippet image

Even if their biological clock states they are north of 40, they never got past 12 when it comes to coding. In case you can’t see the featured image, the source file came from here. And since they will _hopefully_ sweep that up, here is the first snippet.

/* this creates a timestamp based on current time according to the
 * fine rules of RFC3164, most importantly it ensures in a portable
 * way that the month day is correctly written (with a SP instead
 * of a leading 0). The function uses a static buffer which is
 * overwritten on the next call (just like ctime() does).
 */
static char const *rfc3164_current_time(void)
{
	static char time[32];
	struct timeval tv;
	struct tm *tm;
	static char const * const monthnames[] = {
		"Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug",
		"Sep", "Oct", "Nov", "Dec"
	};

	logger_gettimeofday(&tv, NULL);
	tm = localtime(&tv.tv_sec);
	snprintf(time, sizeof(time),"%s %2d %2.2d:%2.2d:%2.2d",
		monthnames[tm->tm_mon], tm->tm_mday,
		tm->tm_hour, tm->tm_min, tm->tm_sec);
	return time;
}

The second problem is they are returning the address of a variable which is inside of the function. Those bounding {} of the function delineate the scope. The static qualifier means the function is only visible from within this source file. So, to try and defeat this, our little 12 year old stuck a static qualifier on the character array (string). ___Maybe___ in the latest and greatest C standard (which I haven’t read) the compiler is now required to BOTH save the value AND keep it externally accessible while the rest of the routine is being garbage collected away.

You see, the definition which was/is in most of the compilers out there, especially those which stopped updating at C99 or a bit sooner, that’s not a requirement. The “requirement” was that the variable only be initialized once and that it retain its value across multiple function calls. There was/is nothing about it still being externally accessible.

When you are writing OpenSource, you aren’t writing for the latest and greatest, you are writing for whatever happens to be out there.

While we are at it, choose “time” for a variable name, especially for a string, is Uber stupid. I couldn’t believe it compiled. My gut tells me that if this code really is working on PC platforms it is working due to a bad architecture and compiler with shit garbage collection.

Don’t worry, I’m not going to cover it all, this would beĀ  a 2+ million word blog post if I did that.

#define NILVALUE "-"
static void syslog_rfc3164_header(struct logger_ctl *const ctl)
{
	char pid[30], *hostname;

	*pid = '\0';
	if (ctl->pid)
		snprintf(pid, sizeof(pid), "[%d]", ctl->pid);

	if ((hostname = logger_xgethostname())) {
		char *dot = strchr(hostname, '.');
		if (dot)
			*dot = '\0';
	} else
		hostname = xstrdup(NILVALUE);

	xasprintf(&ctl->hdr, "<%d>%.15s %s %.200s%s: ",
		 ctl->pri, rfc3164_current_time(), hostname, ctl->tag, pid);

	free(hostname);
}

You see, I started cross compiling this because adding all of the advertised features to the existing logger for OpenVMS was going to take several days to a week. Cross compiling this _should_ have been quicker. Yes, the current C compiler

$ cc/ver
HP C V7.1-015 on OpenVMS Alpha V8.3

Stopped around the time of the C99 standard. I won’t go so far as to say C99 is “all” there. I’ve banged into some partially implemented stuff with the networks, but haven’t dug in to see if they are part of the standard or not.

So the above snippet nests a call to our original snippet inside of a print routine. Once this got ported to OpenVMS I could step through the first snippet and see a perfectly formatted date string. When the sprintf() got done I had a big block-o-nothin where the date should be.

Old Timers, we know these things. Global data isn’t a bad thing, it exists because we need it. All of these little tricks trying to bob and weave around the Grim Reaper of garbage collection are just time bombs waiting to go off. So, a quick cut and paste moving that static char definition of time to global scope brought about the expected compilation error.

static char time[32]; /* place to store a time return value */
............^
%CC-W-MIXLINKAGE, In this declaration, "time" is declared with both internal and external linkage. The previous declaration is at l
ine number 378 in file SYS$COMMON:[SYSLIB]DECC$RTLDEF.TLB;1 (text module TIME).
at line number 112 in file RADIAN_ROOT:[2018-VMS-SYSLOG]logger.c;148

So, changing time[32] to time_str[32] and changing the few references to it brought about the desired outcome. What “could” have been the problem?

  • Grim Reaper garbage collection is faster on old DS-10 than modern Intel PC
  • DECC puts the value in a safe place when function goes out of scope and reassigns value when function is re-entered. The original storage location, however, is garbage collected.
  • Choosing “time” as a variable name, especially for a string, was an incredibly stupid thing to do and it collided with the OS level definition in a way which whacked the string value once outside of the function.
  • Compiler developers, having been told for years by unemployable academics without a single line of QA’ed code in production at any for-profit company that global data is bad, came up with a tweaking of the “static” definition to avoid making global what obviously needed to be global. This was done without concern for existing compilers or code in production.

You can probably come up with more.

The point is, the 12 year old boys always want to play with the new toys while the production coders will keep it simple. Production coders who go to independent QA departments know better than to push the envelop. Code they write today could be ported to a Z-80 chip with a 1980s compiler. You think I’m kidding? There are production systems today still running OS/2 Warp.

OS/2 Warp job posting

Oh, and for the record. I knew it would take 3 days to a week to add the bulk of the features to the existing ported code. This __should__ have taken one day to port. I’m now on day 3 tracking down stuff like this. Too close to finished to quit.

Thanks!

 

 

2 thoughts on “It’s okay to hate the 12 year old boys who write the bulk of OpenSource code

  1. Not sure were this code comes from but I see obvious errors. First off, you are not supposed to hard-code month names. We now live in an international/multicultural world (because of the internet) so customers will want to see month names based upon their locale or personal preferences (in Canada, the legally recognized languages are both English and French so if you want to get a contact job in any layer of government then you had better be supporting two languages (minimum); with French in Canada or Spanish in the USA you will need an extended character set; some programmers try to use ISO-8858-1 or CP-1252 but these efforts almost always fail; everyone needs to move to Unicode which is best encoded as UTF-8. This will put your platform into a mode where you will be able to buy/sell from Alibaba). The short variable size problem reminded me of something I saw on OpenVMS which had been in production for more than a decade. On startup, a program called sys$bintime and stored the result in a 32-bit long (which overwrote 32-bits of the neighboring variable). At the time I was doing code renovation while moving from VAX/VMS to Alpha so I wanted “everything” properly declared (something you very rarely saw on VAX/VMS or Solaris). I mentioned the fubur to the programmer and his response was something like “hey, it works”. Lots of people do not believe me when I talk about proper declarations so let me direct you to one of my introductory c-programming pages where I mention the statement “#define __NEW_STARLET 1” which first appeared in OpenVMS-7.0 () which makes me wonder why it did not appear earlier.
    http://neilrieck.net/demo_vms_html/openvms_demo_index.html#vms_c
    So another things you are not supposed to do is write your own date-time routines. This is how we ended up with the Y2K crisis. It is better to look up the OS “system calls” then call them as required. Sure it will be extra work but now your wiz-bang solution will be portable (able to live on another platform)

    1. I thought I included the link. The code comes from here.
      https://github.com/karelzak/util-linux/blob/7595bed1898109b34eb072093902071fcb75fea9/misc-utils/logger.c
      It’s the logger utility shipped with most every Linux distro. Used for testing syslog, rsyslog, etc. Lots of production shell scripts use it to write information to the system log.

      Wow! __NEW_STARLET 1

      I used to know the story behind that. Oh, yeah, Itanic. The port/common code base began with version 7. You will remember that both INTEL and HP were caught read handed stealing Alpha technology for their disastrous 64-bit failure which was already a decade late to market. GQ Bob sold out the universe to buy himself more suits. Rather than doing the ethical thing of suing and taking over both HP and INTEL he made them change the return register and a bunch of other things, still allowing INTEL to be second source for Alpha. Given the sheer and utter incompetence of G.Q. Bob, DEC continued it’s downward spiral. The ultimate screwing of DEC and the OpenVMS faithful came when HP bought DEC/Compaq and forced OpenVMS to be the first operating system ported to the G.Q. Bob bastard because no other OS wanted to use such an unwanted hate child. The common code base lowest of the low level code in STARLET had to straddle one set of register rules for the Alpha and one set for the Itanic. Hence the need for __NEW_STARLET. Prior to adding support for this unHoly demon spawn, all of the chips VMS (and Linux for that matter) had ever been on used the same group of low number registers in the same way. By convention, I believe it was always R0 which held the return value…assembler days well behind me. It was a different register on the Itanic.

      https://en.wikipedia.org/wiki/OpenVMS

      =====
      Unlike the port from VAX to Alpha, in which a snapshot of the VAX code base circa V5.4-2[17] was used as the basis for the Alpha release and the 64-bit source code pool then diverged, the OpenVMS Alpha and I64 (Itanium) versions of OpenVMS are built and maintained using a common source code library and common tools. The core software source code control system used for OpenVMS is the VMS Development Environment (VDE).[23]

      Two pre-production releases, OpenVMS I64 V8.0 and V8.1, were available on June 30, 2003 and on December 18, 2003. These releases were intended for HP organizations and third-party vendors involved with porting software packages to OpenVMS I64.

      The following are recent OpenVMS I64 releases:

      OpenVMS I64 V8.2, the first production-quality Itanium release, was shipped January 13, 2005. A V8.2 release is also available for Alpha platforms.
      =====

Comments are not opened.