Latest Entries »

From here to there

I’ve spoken to a few coders/owners of talkers privately this week, and its clear that the majority of you want “inline cname”. Whats been interesting about those conversations is the questions that you want covered. For most of you its “will it be easy to install and when can we download it?”, although one or two of you are interested in the “how is it done?”, “why is Spot’s version slower than Lokie’s?”, “Why doesn’t Silvers version work?”.

The first question is really easy to answer, it’ll be part of the big PGPlus patch/PGPlus Download I’ll put on the web once this is all done. If theres enough demand I’ll also split it out into a separate download.

Now for the fun technical stuff. Theres basically 3 ways to do inline coloured names. The first is really easy, and is the way most coders do it their first time. You start by installing Silvers cname code and placing nameof(p) in a few hundred places. Then you find the swear filter (its in parse.c) and a quick copy and paste drop that into some part of the code path and jobs done. Historically Trekkers Rest did it that way after a certain coder got drunk and was convinced having colours everywhere was a good idea (I’ve since sobered up!).

Now I hated inline cname so we let a year pass before I realise we can never get rid of that damn “”feature”” and coder pride means its time to do it properly. You can remove all of the nameof(p) calls, the hotwired swear filter, and replace it all with a simple check in process_output. For those of you that read the pstack_mid article you can see how a simple piece of state information allows you to ensure the correct colour after a replacement. You can take this even further by completely removing the default swear filter and moving that into process_output, and of course if you read the pstack_mid article theres a really great clue in how to stop people beating it with the old sh^Nit trick. For free you get to fix the swear filter, have a simple way of allowing users to switch on or off the swear and name filtering. Historically Trekkers Rest ended up doing it that way and TrueFriends when they acquired the code started by doing it that way.

The third way requires a little sideways thinking. lets forget the second way of doing things for a moment and look again at the first. Now I mentioned you could hotwire the old swear filter to do this job, I didn’t mention its a real pain if you leave it in the same place as the swear filter to allow users that hate the feature the ability to turn it off. But if you move it far enough down you can remove all the work that Silvers code requires and get an easy on/off switch. The logical place for that is in the tellplayer() function. Forgetting things like the emergency log and tell history… that would give you a tellplayer function that looks something like this in pcode:
void tellplayer(p, str)
{
char *tstr;
tstr = process_dynatext(p, str);
tstr = process_cname(p, tstr);
tstr = process_output(p, tstr);
write_socket(p, tstr); /* yes I know process_output returns a file object, but lets keep things simple! */
}

I suppose we could waste a paragraph talking about some obvious optimisations. Instead of a players name, dynatext should use a players cname. Cname could precompute the client colour codes and save some work in process_output, and so forth….

Finished thinking about cool optimisations and how to save work?

Okay back to the present… no matter how good your optimisations are, method 2 will take 2/3rds the time of method 3. What we have is 3 linear search and replace routines each with their own specialised domain knowledge. Dynatext looks for sequences beginning with ‘<‘, cname/swear looks for words, process_output looks for ‘^’ and ‘\n’. From a holistic point of view they are all the same code!

The third method is about exploiting the fact that what we’re about to send to the user is really a sequence of tokens that require processing, and that instead of having 3 specialised pieces of code to handle that processing we can replace it with a single generalised piece of code. Not only is that far quicker and more memory efficient, but should we decide to add additional tokens (such as smileys, more colour codes, inline images, sound, non telnet support) we can deal with them in the same place.

And Truefriends uses an output system based on those ideas, oh and I renamed ‘cname’ to ‘filtered output code’ as cname no longer seemed appropriate.

Whilst most of you want me to now post a download link, my aims are somewhat different. I want people to understand the code, or at least the rationale behind some of the decisions that shaped it. And so this post is about saying “hey this is why my code performs better than someone else’s, this is the secret sauce I used to solve this problem in what I hope is the best way” and hopefully some of you will go off and experiment. The rest, well I’ve got a few cool projects between here and there that will not only improve your talker, but hopefully give you some further insights ready for when we tackle the feature most users instantly notice.

Stuck in the middle with you

After a few days of tidying up code its time to get back to the job of improving things. Today we’ll tackle a nice simple job, pstack_mid.

To begin with we’ll create a variable argument version of that function, mainly because it annoys me there isn’t one:

void PSTACK_MID(const char *format, ...)
{
char *temp;
va_list argum;va_start(argum, format);
vsprintf(stack, format, argum);
va_end(argum);temp = strdup(stack);
pstack_mid(temp);
if (temp)
FREE(temp);
}

So a pretty standard function, with the only talking point being that I don’t check strdup returned a sane value until its time to tidy up.

The current pstack_mid is a little dumb when it comes to handling strings with colour codes, so we’ll fix that, and give it a slight performance boost as well.


extern int valid_char_col(char);void pstack_mid(char *str)
{
int width, half, len;
char *line_pos, *str_pos, cur_col[1], last_col[1];/* if str is empty or non existant, just print a line */
if (!str || !*str)
{
stack = stpcpy(stack, LINE "\n");
return;
}

Thats why we didn’t handle strdup in PSTACK_MID, we’ll handle it in the real function and save duplicating some code.


/* we want the length of str without counting the colour codes */
str_pos = str;
len = 0;
*last_col = 'N';
while (*str_pos)
{
/* check if we have a colour code */
if (*str_pos == '^')
{
if (*(str_pos + 1) == 'N' || *(str_pos + 1) == 'n' || valid_char_col(*(str_pos + 1)))
{
*last_col = *(str_pos + 1);
str_pos += 2;
continue;
}
/* ^^ renders as ^ so check for that case */
else if (*(str_pos + 1) == '^')
str_pos++;
}
/* if we're here count the character */
len++;
str_pos++;
}

The new pstack_mid is really just 3 more copy and pastes of that loop.


/* try and get the current line width */
if (current_player)
width = current_player->term_width;
else
width = 76;
/* ensure width is always long enough to hold str. We'll let process_output
handle getting the line wrap right */
width = width * ((len + 2) / width) + 1);

The original code has a slight flaw. If the length of the string being centered is the same as the line width, it’ll crash the talker. the original code was a little silly when str was larger than width, so we solve both those problems by figuring out the number of lines we need to hold the str and sizing width accordingly.


/* copy the prefix */
half = (width - (len + 2)) / 2;
*cur_col = 'N';
line_pos = LINE;
while (half)
{
/* check to see if we're on a colour code */
if (*line_pos == '^')
{
if (*(line_pos + 1) == 'N' || *(line_pos + 1) == 'n' || valid_char_col(*(line_pos + 1)))
{
*cur_col = *(line_pos + 1);
line_pos += 2;
*stack++ = '^';
*stack++ = *cur_col;
if (!*line_pos)
line_pos = LINE;
}
/* ^^ renders as ^ so check for that case */
else if (*(line_pos + 1) == '^')
line_pos++;
}
*stack++ = *line_pos++;
half--;
if (!*line_pos)
line_pos = LINE;
}

Very similar to our string counting loop. Only real important thing to note is that we don’t use an external function to keep track of where we are in the LINE constant.


/* now to copy the str... */
*stack++ = ' ';
stack = stpcpy(stack, str);

We could have used the same loop to copy str into place, but this way is potentially faster.


/* we need to sync our positions so a quick loop coming up */
half = len + 2;
while (half)
{
/* check to see if we're on a colour code */
if (*line_pos == '^')
{
if (*(line_pos + 1) == 'N' || *(line_pos + 1) == 'n' || valid_char_col(*(line_pos + 1)))
{
*cur_col = *(line_pos + 1);
line_pos += 2;
if (!*line_pos)
line_pos = LINE;
}
/* ^^ renders as ^ so check for that case */
else if (*(line_pos + 1) == '^')
line_pos++;
}
half--;
line_pos++;
if (!*line_pos)
line_pos = LINE;
}

Although we still need that loop to ensure we’re in the right place for when we recopy the line, and have the right colour code for the next part.


/* if your paying attention as opposed to blindly copying and pasting
you'll note I've kept track of the colour code in effect. So now
we can restore the line colour if needed. */
if (*cur_col != *last_col)
{
stack = stpcpy(stack, "^N^");
*stack++ = *cur_col;
}
*stack++ = ' ';

TrueFriends uses a similar system to ensure its cname doesn’t mess up the screen. Well similar in the same way that oranges and lemons are both citrus fruits.


/* now we copy the postfix */
half = (width - (len + 2)) / 2;
/* if width - (len + 2) is odd, then the line is short by 1 character
so lets fix that */
if (width - (len + 2) % 2)
half++;

I’m sorry there just isn’t an intuitive way to test if a number is odd.


while (half)
{
/* check to see if we're on a colour code */
if (*line_pos == '^')
{
if (*(line_pos + 1) == 'N' || *(line_pos + 1) == 'n' || valid_char_col(*(line_pos + 1)))
{
*cur_col = *(line_pos + 1);
line_pos += 2;
*stack++ = '^';
*stack++ = *cur_col;
if (!*line_pos)
line_pos = LINE;
}
/* ^^ renders as ^ so check for that case */
else if (*(line_pos + 1) == '^')
line_pos++;
}
*stack++ = *line_pos++;
half--;
if (!*line_pos)
line_pos = LINE;
}

The final cut and paste of that loop to finish off the line.


/* Now we tidy everything up */
if (*cur_col != 'N')
{
*stack++ = '^';
*stack++ = 'N';
}
*stack++ = '\n';
*stack = '';
}

If colour is on the line, then we end the colour.

And thats a colour aware pstack_mid. Its probably a good idea to quickly scan the code and see where its sane to use the new variable argument version in place of the single argument version.

Sorry, for WordPress being dumb and stripping out the code formatting, I’ll try and find a way to make it saner for the  more complicated code thats coming.

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.