Archive for July, 2010


A little off the top

Before we start with the grand string tidy up, its a good idea to remove any code from PGPlus that we don’t need.

The first item in the crosshairs is history.c. Nothing within PGPlus currently uses those functions, and when we later come to needing history like functionality we can do a better job with a somewhat different algorithm. Removal is relatively simple, first we delete the file from the src directory. Next we delete these lines from proto.h:
/* historee externs */
extern history *init_hist ( int );
extern void add_to_hist ( history *, char * );
extern int stack_hist ( history *, int, char * );
extern void clean_hist ( history * );
extern void free_hist ( history * );
extern void init_global_histories ( void );

And finally we delete this line from glue.c:
init_global_histories(); /* history logging */

Our next target is ident. Historically ident was used to provide the name of a user connected to a specific port, which was somewhat useful in the early days of telnet talkers. However these days ident servers are rare, Windows for example has no ident service and most Linux distro’s either don’t include one or have it disabled by default.  Removing ident is a little more complicated than history, but far more worthwhile from a time perspective.

Our first task is to remove it from the list of files that are compiled every time we compile the talker. To do that we need to edit src/configure/makefile.template and remove the following line:
IDENT = ident_server
Next we edit these lines to remove $(IDENT) from the list:
ifdef STRIP
all: $(TALKER) $(ANGEL) $(IDENT) $(INTERCOM) strip
else
all: $(TALKER) $(ANGEL) $(IDENT) $(INTERCOM)
endif
else
all: $(TALKER) $(ANGEL) $(IDENT) $(INTERCOM)
endif

We now completely delete this section:
$(IDENT): ident_server.c include/ident.h Makefile
@echo "Now compiling .... ident server"
@$(CC) $(CFLAGS) $(DEFS) -o $(IDENT) ident_server.c $(LIBS)

Followed by deleting these 2 lines:
@echo "Now stripping .... $(IDENT) binary"
@strip $(IDENT)

And then these 3 lines:
@echo "Now installing ... $(IDENT) binary"
@mv $(IDENT) $(BIN)/$(IDENT)
@chmod $(PERMS) $(BIN)/$(IDENT)

Finally we remove $(IDENT) from this line:
@-rm -f $(TALKER) $(ANGEL) $(IDENT) $(INTERCOM) $(OBJECTS) ../junk/*
And save that file. Next we need to edit Configure.options in the same folder, removing this line from the file:
bool 'Include ident server lookup code' IDENT
After closing that file, we need to edit Configure.help to remove these lines:
Include ident server lookup code
IDENT
Allows the admin to see what account name a user is connecting with
provided the server they are connecting from uses an ident server.

Now that the configure and makefiles have been edited, we can start removing the code. So we begin by deleting ident_server.c and ident_client.c. Next we need to track down any ident code in the talker itself. typing ./qs 'def IDENT' in our src directory will list every file we need to edit. As the first 2 edits in glue.c are non intuitive I’ll show them here, the rest are just simple deletes.
#ifndef IDENT
if (q == NULL || p == q)
#else
if (q == NULL) /* residents don't see their own ident lookup */
#endif

Becomes:
if (q == NULL || p == q)
Likewise:
#ifndef IDENT
if (can_get_addy)
return (p->inet_addr);return 0;
}
#else
#ifdef ROBOTS
if (!can_get_addy || (!strcasecmp(p->remote_user, "<") && !strcasecmp(p->remote_user, ">")) || p->residency & ROBOT_PRIV)
#else
if (!can_get_addy || (!strcasecmp(p->remote_user, "<") && !strcasecmp(p->remote_user, ">")))
#endif
{
sprintf(retstr, "%s", p->inet_addr);
while (*retstr)
retstr++;
}
else
{
sprintf(retstr, "%s@%s", p->remote_user, p->inet_addr);
while (*retstr)
retstr++;
}
*retstr++ = 0;return addy;
}
#endif

Becomes:
if (can_get_addy)
return (p->inet_addr);return 0;
}

And with that (and the edits I’m leaving to you) the old ident code is purged from the talker.

The next item to remove is the robots code. Sadly there are very few robots for PGPlus, with those that exist tending to connect to the talker via a normal login. Purging this code is a little less complicated than ident, but just as fiddly.
Just for kicks, I’ll do this one backwards. Start by deleting robot_plist.c and robot_int.c from the src directory, along with the 2 header files robot_player.h and robot_proto.h.
Next we search in our src folder for “def ROBOTS” and “def INTELLIBOTS”, as before we delete any code found. Finally we move into the configure directory, and remove the references to ROBOTS from both Configure.options and Configure.help.

With all of that done, we can now “make newos” to update our makefile, “make configure” to update our configuration, and finally “make install” to enjoy the fruits of our labours.

Breaking stuff to look tough

Yesterday we started tidying up the code by first cleaning up ADDSTACK and ENDSTACK in shortcut.c, before replacing them with macro definitions in proto.h. Sadly for us those macro’s don’t fix the hundreds of lines of:
sprintf(stack, "something");
stack = strchr(stack, 0);

For that we’re going to have to manually track down every sprintf and edit. But before we do, lets consider another case we’d also like to tidy up.
strcpy(stack, "some text");
stack = strchr(stack, 0);

Unlike the sprintf example, we can’t attribute this to sloppy coding. The return value for strcpy is a pointer to its destination, which in the above example is stack. Now we could replace those with sprintf, except it would be a silly idea. sprintf is an expensive function to call just for a string copy, if anything we want to convert sprintf to strcpy when its not doing any text formatting. A naive solution to this problem would look like:
char *my_strcpy(char *dest, char *src)
{
char *work;
strcpy(dest, src);
/* wordpress doesn't render the usual null terminator correctly, so I'll use 0 even though thats wrong to show my point */
work = strchr(dest, 0);
return work;
}

Which would make the code look pretty, but replaces 2 function calls with 3, and makes the code slightly slower. So onto a better example:
char *my_strcpy(char *dest, const char *src)
{
char *d = dest;
const char *s = src;
do
*d++ = *s;
while (*s++ != 0);

return d - 1;
}

However GNU has a library function named stpcpy which does exactly what we want. Ideally we want to use that function instead of our own when its available. To do so we first need to change our makefile. Heading to src/configure/makefiles we edit the file named linux, changing:
MF_OS_CFLAGS="-DLINUX"
To now read:
MF_OS_CFLAGS="-DLINUX -D_GNU_SOURCE"
Then back in our src directory, we need to “make newos” for those changes to take effect. If we now try to compile the talker, the compiler will immediately produce errors, so lets work through those.
In file included from admin2.c:24:0:
include/proto.h:439:19: error: conflicting types for strcasestr
/usr/include/string.h:371:14: note: previous declaration of strcasestr was here

GNU has a strcasestr function which now conflicts with the utility function in xstring.c. We can fix those compiler errors simply by wrapping the prototype and the code inside:
#ifndef _GNU_SOURCE
/* conflicting code is here */
#endif

One thing to note whilst we’re doing this, the PGPlus strcasestr function has a subtle bug:

Return Value

Upon successful completion, strcasestr() shall return a pointer to the located string or a null pointer if the string is not found. If s2 points to a string with zero length, the function shall return s1.

Whilst PGPlus’s way of returning a null pointer is saner, any code that relies upon that will break. So we’ll change PGPlus’s default function to do the right thing. Now we check every call to strcasestr to ensure its passing a valid string, and modify it if its not.

After making those changes, our next compiler error is:
In file included from aliases.c:19:0:
include/fix.h:23:17: error: conflicting types for getitimer
/usr/include/sys/time.h:127:12: note: previous declaration of getitimer was here

And just as with the strcasestr prototype, we’ll wrap the contents of fix.h in an #ifndef…#endif block.
And one last compiler warning:
ttt.c: In function ttt_print_board:
ttt.c:94:5: warning: implicit declaration of function sprintf
ttt.c:94:5: warning: incompatible implicit declaration of built-in function sprintf

That ones our fault. When we changed ADDSTACK and ENDSTACK to be macro forms of sprintf we didn’t check every file includes the correct header. So we just need to add an include for stdio.h and our code is back to compiling cleanly, and we can return to cleaning up the code.

Its always important when starting a new project to ensure the foundations are solid. With PGPlus that invariably means tidying up the code and ensuring the layout is consistent.

Consider the following 2 functions:

void ADDSTACK(char *format,...)
{
va_list argum;
va_start(argum, format);
vsprintf(stack, format, argum);
va_end(argum);
stack = strchr(stack, 0);
}
void ENDSTACK(char *format,...)
{
va_list argum;
va_start(argum, format);
vsprintf(stack, format, argum);
va_end(argum);
stack = end_string(stack);
}

Both functions copy text at the position pointed to by stack, and then move that pointer forwards. However they both do so sub-optimally. We can  immediately improve this and remove the call to strchr by taking advantage of the fact that vsprintf returns the number of characters written.

void ADDSTACK(char *format,...)
{
va_list argum;va_start(argum, format);
stack += vsprintf(stack, format, argum);
va_end(argum);
}
void ENDSTACK(char *format,...)
{
va_list argum;va_start(argum, format);
stack += vsprintf(stack, format, argum);
va_end(argum);
stack = end_string(stack);
}

Looking at ENDSTACK we can see that it positions the stack pointer 1 place past the string terminator, so we can replace the call to end_string with a simple statement, like so:

void ENDSTACK(char *format,...)
{
va_list argum;
va_start(argum, format);
stack += vsprintf(stack, format, argum);
va_end(argum);
*stack++ = '';
}

And what we’ve ended up with is a variable argument call wrapping another variable argument call. Obviously thats a little silly, so instead of modifying the functions themselves, lets replace them with some preprocessor magic.

#define ADDSTACK(a, ...) stack += sprintf(stack, a, ## __VA_ARGS__)
#define ENDSTACK(a, ...) stack += sprintf(stack, a, ## __VA_ARGS__); *stack++ = ''

And now the compiler is doing a search and replace for us. However if we try to compile with that change, we’re going to immediately hit a compiler error:

channel.c: In function zc:
channel.c:637:3: error: else without a previous if
make: *** [../junk/channel.o] Error 1

Looking at the code in question reveals immediately what went wrong:

if (newchan)
ENDSTACK("<(%s)> ++ %s creates and joins this channel ++\n", p->zchannel, p->name);
else
ENDSTACK("<(%s)> ++ %s joins this channel ++\n", p->zchannel, p->name);

Our ENDSTACK macro creates 2 statements, and that breaks any blocks that arn’t wrapped in a pair of braces. Luckily theres an easy way to fix this which doesn’t involve editing lots of code. ENDSTACK moves the stack pointer 1 position past the end of the string its just written, so if we change the macro like so:

#define ENDSTACK(a, ...) stack += sprintf(stack, a, ## __VA_ARGS__) + 1

And with that our code compiles correctly without us needing to do any more work.

Getting it installed

So you’ve downloaded PG+ from the website, unpacked it, typed make followed by make install and watched as your screen filled with compile errors.

So todays task is to get PG+ to compile without any warnings.

Now compiling .... admin2.c 0 adm lines
In file included from admin2.c:24:0:
include/proto.h:235:13: warning: conflicting types for built-in function log

Back when PG+ was first written, math functions were contained in their own header file, todays compilers however are aware of them, hence the warning message. Its not really a problem as we’re not linking with the math library so the PG+ log function is the one that will be used. We’ll solve this warning by renaming PG+’s function logF (which makes sense as theres a companion PG+ function called LOGF).

On a 64bit operating the next error we encounter looks like this.

Now compiling .... aliases.c alia lines
aliases.c: In function tmp_comp_alias:
aliases.c:77:24: warning: cast from pointer to integer of different size
aliases.c:77:38: warning: cast from pointer to integer of different size

Looking at the line immediately reveals the problem:

store_int(oldstack, ((int) stack - (int) oldstack));

An 8 byte pointer is being assigned to a 4 byte integer. We can easily fix that warning by removing the forced casts and allowing the compiler to do the necessary type conversions. The rest of those errors all have the same underlying cause, if not the same solution.

Now compiling .... glue.c 3 glu lines
glue.c: In function main:
glue.c:1778:7: warning: format %d expects type int, but argument 3 has type long int

And the offending line looks like:
sprintf(stack_start, "Lost stack reclaimed %d bytes\n", stack - stack_start);
We can fix this one by either changing the format specifier to handle a long int, or casting the the pointer math to an int, both are valid fixes in this instance.

intercom_glue.c: In function start_intercom:
intercom_glue.c:138:7: warning: missing sentinel in function call

Which is usually caused by a variable number of arguments not being terminated by a NULL value, heres the line in question.
execlp ("bin/intercom", intercom_name, 0);
And heres the fix.
execlp ("bin/intercom", intercom_name, (char *)NULL);

And the last 2 errors from that file.
intercom_glue.c:152:3: warning: implicit declaration of function time
intercom_glue.c:152:3: warning: nested extern declaration of time

Time functions are spread across time.h and sys/time.h, so its just a matter of ensuring both are included.

items.c:969:32: warning: trigraph ??! ignored, use -trigraphs to enable

And for that one we just need to edit the text to ensure the preprocessor isn’t confused.

And after all that work, we’re finally ready to boot our talker.

The obligatory introduction

Its been almost 5 years since I last used or coded for a telnet talker. During that time some have disappeared from the internet, whilst others have stagnated.

This blog isn’t about that decline, or some magic bullet that will suddenly make talkers relevant again. Instead its a chronicle of how to make a new talker, one that hopefully appeals to the web 2.0 crowd.

For those of you running your own talkers, hopefully you’ll find this an interesting place for ideas, hits, tips, and of course code. And just to make things interesting I’ll start with a default copy of PG+ and promise not to quickly nab code from any of my previous talkers.