> Problem is, I'm not an X programmer, so I don't know if my fixes make
> any sense.

Well, someone already pointed out that littering /usr/X386 throughout
the Makefile is probably a bad idea.  I address myself to the code
changes.

This is not a complete vetting of Xchess.  It's restricted to just
issues raised by the patches posted.

> diff -c ../Xchess/program.c ./program.c
> *** ../Xchess/program.c	Fri Dec 17 13:56:15 1993
> --- ./program.c	Fri Oct 21 11:14:08 1994
> ***************
> *** 146,152 ****
>   	/* Do a poll... */
>   
>   	if (!(i = select(32, &rfd, &wfd, &xfd, &notime)) &&
> ! 			!from->_cnt) {		/* Bad stuff... */
>   		if (debug)
>   			fprintf(stderr, "poll: nothing\n");
>   		return (NULL);
> --- 146,152 ----
>   	/* Do a poll... */
>   
>   	if (!(i = select(32, &rfd, &wfd, &xfd, &notime)) &&
> ! 			!from->_r) {		/* Bad stuff... */
>   		if (debug)
>   			fprintf(stderr, "poll: nothing\n");
>   		return (NULL);

This is hair-raisingly awful code (both before and after the fix).
First of all is the practice of trying to mix syscall I/O (select) and
stdio I/O (fgets).  This is never a good idea, and the sort of things
that resulted here are why.  Fortunately in this case, stdio is hardly
needed; the only thing done on that FILE * seems to be fgets().

Some of the problems:

- select() no longer takes ints; it takes fd_sets.  This will break
  horribly the first time it encouters a system where fd_sets are laid
  out significantly differently from ints in-core (say, a system where
  an fd_set is just a very large big-endian integer type).

- There is no assurance that fileno(from) is small enough that
  1<<fileno(from) won't overflow.

- There is no assurance that select() won't modify the struct timeval
  passed as its fifth argument; it must be re-zeroed each time around.

- There is no assurance that anything like _cnt even exists, though as
  this person found, NetBSD stdio has a similar field with a different
  name.  from should be replaced by some sort of local buffering built
  on top of read() calls.  (Actually, stdio should export some sort of
  amount_of_data_buffered() interface, but until then....)

> diff -c ../Xchess/scrollText.c ./scrollText.c
> *** ../Xchess/scrollText.c	Fri Dec 17 13:56:21 1993
> --- ./scrollText.c	Fri Dec 17 13:56:21 1993
> ***************
> *** 629,635 ****
>   		   textInfo->bgGC, 
>   		   0, 0, BARSIZE, top-1);
>       XFillRectangle(display, textInfo->scrollBar,
> ! 		   DEFAULT_GC, top, BARSIZE - (2*BARBORDER) - 2,
>   		   bottom - top);
>       XFillRectangle(display, textInfo->scrollBar, DEFAULT_GC,
>   		   0, bottom+1, BARSIZE,
> --- 629,635 ----
>   		   textInfo->bgGC, 
>   		   0, 0, BARSIZE, top-1);
>       XFillRectangle(display, textInfo->scrollBar,
> ! 		   DEFAULT_GC, 0, top, BARSIZE - (2*BARBORDER) - 2,
>   		   bottom - top);
>       XFillRectangle(display, textInfo->scrollBar, DEFAULT_GC,
>   		   0, bottom+1, BARSIZE,

The original code is definitely wrong; it's passing too few arguments
to XFillRectangle().  This fix appears correct.

> ***************
> *** 1058,1064 ****
>       XCopyArea(display, textInfo->arrowMap, textInfo->mainWindow,
>   	       textInfo->CursorGC,
>   	       0, 0, arrow_width, arrow_height,
> ! 	       x, y + h - arrow_height, 1);
>   
>       /* Then draw the stem */
>       XDrawLine(display, textInfo->mainWindow, textInfo->CursorGC,
> --- 1058,1064 ----
>       XCopyArea(display, textInfo->arrowMap, textInfo->mainWindow,
>   	       textInfo->CursorGC,
>   	       0, 0, arrow_width, arrow_height,
> ! 	       x, y + h - arrow_height);
>   
>       /* Then draw the stem */
>       XDrawLine(display, textInfo->mainWindow, textInfo->CursorGC,

Looks right.  (Quite likely a snarf-&-barf error in the original code
from an XClearArea() call somewhere.)

> ***************
> *** 1573,1579 ****
>       /* Clear out bottom space region */
>       XClearArea(display, textInfo->mainWindow,
>   	       0, textInfo->h - textInfo->bottomSpace,
> ! 	       textInfo->w, textInfo->bottomSpace);
>       
>       UpdateExposures(display, textInfo);
>       UpdateScroll(display, textInfo);
> --- 1573,1579 ----
>       /* Clear out bottom space region */
>       XClearArea(display, textInfo->mainWindow,
>   	       0, textInfo->h - textInfo->bottomSpace,
> ! 	       textInfo->w, textInfo->bottomSpace, 1);
>       
>       UpdateExposures(display, textInfo);
>       UpdateScroll(display, textInfo);

The fix you tried to make is correct, but you should pass True rather
than 1 as the last argument.  That argument is a Bool, not an int, and
there's (a) no guarantee that Xlib's Bool type is compatible with an
int and (b) even if it is, no guarantee that 1 is the appropriate value
for True.

> diff -c ../Xchess/std.c ./std.c
> *** ../Xchess/std.c	Fri Dec 17 13:56:22 1993
> --- ./std.c	Fri Dec 17 13:56:22 1993
> ***************
> *** 345,351 ****
>           char *s;
>   {
>   	fputs("Internal Error: ", stderr);
> ! 	_doprnt(s, &args, stderr);
>   	putc('\n', stderr);
>   
>   	kill(getpid(), SIGIOT);
> --- 345,351 ----
>           char *s;
>   {
>   	fputs("Internal Error: ", stderr);
> ! /*	_doprnt(s, &args, stderr); */
>   	putc('\n', stderr);
>   
>   	kill(getpid(), SIGIOT);

fatal() should be completely rewritten.  It should read something like

#include <stdarg.h> /* up with other #includes, of course */
void fatal(const char *s, ...)
{
 va_list ap;

 va_start(ap,s);
 fprintf(stderr,"Internal Error: ");
 vfprintf(stderr,s,ap);
 fprintf(stderr,"\n");
 va_end(ap);
 kill(getpid(),SIGIOT); /* or perhaps abort(), though I prefer this */
}

> diff -c ../Xchess/std.h ./std.h
> *** ../Xchess/std.h	Fri Dec 17 13:56:23 1993
> --- ./std.h	Fri Dec 17 13:56:23 1993
> ***************
> *** 71,77 ****
>   
>   extern char *getenv();
>   extern int errno;
> - extern char *sys_errlist[];
>   
>   /* Should use BSIZE instead of BUFSIZ... */
>   
> --- 71,76 ----

Better yet, eliminate errno as well, include <errno.h>, and use
strerror() instead of any reference to sys_errlist/sys_nerr.

> diff -c ../Xchess/window.c ./window.c
> *** ../Xchess/window.c	Fri Dec 17 13:56:24 1993
> --- ./window.c	Fri Oct 21 15:28:27 1994
> ***************
> *** 846,852 ****
>   				   xchess_width, xchess_height);
>   	cur = XCreatePixmapCursor(win->display, bm, bmask,
>   			    &win->cursorcolor,
> ! 			    &WhitePixel(win->display, 0),
>   			    xchess_x_hot, xchess_y_hot);
>   	XFreePixmap(win->display, bm);
>   	XFreePixmap(win->display, bmask);
> --- 846,852 ----
>   				   xchess_width, xchess_height);
>   	cur = XCreatePixmapCursor(win->display, bm, bmask,
>   			    &win->cursorcolor,
> ! 			    &win->textback,
>   			    xchess_x_hot, xchess_y_hot);
>   	XFreePixmap(win->display, bm);
>   	XFreePixmap(win->display, bmask);

This unquestionably needs fixing; there is no assurance at all that
WhitePixel() returns anything that can have its address taken.
textback is perhaps not ideal, but it likely is the best thing readily
available. :-/

					der Mouse

			    mouse@collatz.mcrcim.mcgill.edu