finally fixed pthreads

Makefile

@@ -6,7 +6,7 @@ PTHREADDEF = -DHAVE_PTHREADS
 PTHREADLIB = -lpthread
 
 #CC=cc
-CFLAGS=-g -Wall $(PTHREADDEF)
+CFLAGS=-g -Wall $(PTHREADDEF) $(INCLUDES)
 #CFLAGS=-O2 -Wall
 RANLIB=ranlib
 
@@ -32,7 +32,7 @@ install_OSX: OSXvnc-server
 	cp OSXvnc-server storepasswd ../OSXvnc/build/OSXvnc.app/Contents/MacOS
 
 .c.o:
-	$(CC) $(CFLAGS) $(INCLUDES) -c $<
+	$(CC) $(CFLAGS) -c $<
 
 libvncserver.a: $(OBJS)
 	$(AR) cru $@ $(OBJS)
@@ -54,7 +54,13 @@ sratest: sratest.o
 	$(CC) -o sratest sratest.o
 
 sratest.o: sraRegion.c
-	$(CC) $(CFLAGS) $(INCLUDES) -DSRA_TEST -c -o sratest.o sraRegion.c
+	$(CC) $(CFLAGS) -DSRA_TEST -c -o sratest.o sraRegion.c
+
+blooptest: blooptest.o libvncserver.a
+	$(CC) -o blooptest blooptest.o $(LIBS)
+
+blooptest.o: example.c
+	$(CC) $(CFLAGS) -DBACKGROUND_LOOP_TEST -c -o blooptest.o example.c
 
 clean:
 	rm -f $(OBJS) *~ core "#"* *.bak *.orig storepasswd.o \

README

@@ -92,6 +92,8 @@ To set the mouse coordinates (or emulate mouse clicks), call
   defaultPtrAddEvent(buttonMask,x,y,cl);
 However, this works only if your client doesn't do local cursor drawing. There
 is no way (to my knowledge) to set the pointer of a client via RFB protocol.
+IMPORTANT: do this at the end of your function, because this actually draws
+the cursor if no cursor encoding is active.
 
 What is the difference between rfbScreenInfoPtr and rfbClientPtr?
 -----------------------------------------------------------------

TODO

@@ -2,10 +2,8 @@ immediate:
 ----------
 
 fix bug in http (java) client with big endian server: byte swapping is broken
-cursor "smears" sometimes when not using cursor encoding
 really support pthreads.
-	- cursor seems to be undrawn wildly
-	- connection gone and then reconnect is a problem
+	- cursor drawing!
 
 in the works:
 -------------
@@ -16,6 +14,8 @@ optionally dont draw rich cursors as xcursors
 later:
 ------
 
+cursor "smears" sometimes when not using cursor encoding
+	(seems to be gone now; haven't debugged properly, though)
 udp
 autoconf? at least Sun Solaris compilation
 rfbCloseClient, rfbConnect, ConnectToTcpAddr
@@ -34,4 +34,10 @@ done:
 .test drawing of cursors when not using xcursor or rich cursor encoding
 fix bug with odd width (depends on client depth: width has to be multiple of server.bytesPerPixel/client.bytesPerPixel). only raw!! -> bug of vncviewer!
 .use sraRegion from Wez instead of miregion, because it is much smaller
-
+.	- connection gone and then reconnect is a problem
+	  the reason: there are in fact three threads accessing
+	  the clientPtr: input, output and the application thread.
+	  if you kill the viewer or do rfbCloseClient, all of those
+	  three have to be warned that this is happening.
+	-> rfbClientConnectionGone can only be called by the outer loop
+	(with background loop, it is input, else it is processEvents).

example.c

@@ -21,6 +21,7 @@
  *  USA.
  */
 
+#include <unistd.h>
 #ifdef __IRIX__
 #include <netdb.h>
 #endif
@@ -99,10 +100,10 @@ void drawline(unsigned char* buffer,int rowstride,int bpp,int x1,int y1,int x2,i
 void doptr(int buttonMask,int x,int y,rfbClientPtr cl)
 {
    ClientData* cd=cl->clientData;
+
    if(cl->screen->cursorIsDrawn)
      rfbUndrawCursor(cl);
-   cl->screen->cursorX=x;
-   cl->screen->cursorY=y;
+
    if(x>=0 && y>=0 && x<maxx && y<maxy) {
       if(buttonMask) {
 	 int i,j,x1,x2,y1,y2;
@@ -112,6 +113,7 @@ void doptr(int buttonMask,int x,int y,rfbClientPtr cl)
 		     x,y,cd->oldx,cd->oldy);
 	    rfbMarkRectAsModified(cl->screen,x,y,cd->oldx,cd->oldy);
 	 } else { /* draw a point (diameter depends on button) */
+	    int w=cl->screen->paddedWidthInBytes;
 	    x1=x-buttonMask; if(x1<0) x1=0;
 	    x2=x+buttonMask; if(x2>maxx) x2=maxx;
 	    y1=y-buttonMask; if(y1<0) y1=0;
@@ -119,7 +121,7 @@ void doptr(int buttonMask,int x,int y,rfbClientPtr cl)
 
 	    for(i=x1*bpp;i<x2*bpp;i++)
 	      for(j=y1;j<y2;j++)
-		cl->screen->frameBuffer[j*cl->screen->paddedWidthInBytes+i]=0xff;
+		cl->screen->frameBuffer[j*w+i]=0xff;
 	    rfbMarkRectAsModified(cl->screen,x1,y1,x2-1,y2-1);
 	 }
 
@@ -131,6 +133,7 @@ void doptr(int buttonMask,int x,int y,rfbClientPtr cl)
 
       cd->oldx=x; cd->oldy=y; cd->oldButton=buttonMask;
    }
+   defaultPtrAddEvent(buttonMask,x,y,cl);
 }
 
 /* aux function to draw a character to x, y */
@@ -308,14 +311,16 @@ int main(int argc,char** argv)
   /* initialize the server */
   rfbInitServer(rfbScreen);
 
+#ifndef BACKGROUND_LOOP_TEST
   /* this is the blocking event loop, i.e. it never returns */
   /* 40000 are the microseconds, i.e. 0.04 seconds */
   rfbRunEventLoop(rfbScreen,40000,FALSE);
+#endif
 
   /* this is the non-blocking event loop; a background thread is started */
   rfbRunEventLoop(rfbScreen,40000,TRUE);
   /* now we could do some cool things like rendering */
-  while(1) /* render() */;
+  while(1) sleep(5); /* render(); */
    
   return(0);
 }

main.c

@@ -72,7 +72,7 @@ void rfbMarkRegionAsModified(rfbScreenInfoPtr rfbScreen,sraRegionPtr modRegion)
 {
    rfbClientIteratorPtr iterator;
    rfbClientPtr cl;
-   
+
    iterator=rfbGetClientIterator(rfbScreen);
    while((cl=rfbClientIteratorNext(iterator))) {
      pthread_mutex_lock(&cl->updateMutex);
@@ -88,6 +88,7 @@ void rfbMarkRectAsModified(rfbScreenInfoPtr rfbScreen,int x1,int y1,int x2,int y
 {
    sraRegionPtr region;
    int i;
+
    if(x1>x2) { i=x1; x1=x2; x2=i; }
    x2++;
    if(x1<0) { x1=0; if(x2==x1) x2++; }
@@ -103,7 +104,7 @@ void rfbMarkRectAsModified(rfbScreenInfoPtr rfbScreen,int x1,int y1,int x2,int y
    sraRgnDestroy(region);
 }
 
-int rfbDeferUpdateTime = 400; /* ms */
+int rfbDeferUpdateTime = 40; /* ms */
 
 #ifdef HAVE_PTHREADS
 static void *
@@ -415,22 +416,25 @@ void rfbInitServer(rfbScreenInfoPtr rfbScreen)
 void
 rfbProcessEvents(rfbScreenInfoPtr rfbScreen,long usec)
 {
-    rfbCheckFds(rfbScreen,usec);
-    httpCheckFds(rfbScreen);
+  rfbClientPtr cl,cl_next;
+
+  rfbCheckFds(rfbScreen,usec);
+  httpCheckFds(rfbScreen);
 #ifdef CORBA
-    corbaCheckFds(rfbScreen);
+  corbaCheckFds(rfbScreen);
 #endif
-    {
-       rfbClientPtr cl,cl_next;
-       cl=rfbScreen->rfbClientHead;
-       while(cl) {
-	 cl_next=cl->next;
-	 if(cl->sock>=0 && FB_UPDATE_PENDING(cl)) {
-	    rfbSendFramebufferUpdate(cl,cl->modifiedRegion);
-	 }
-	 cl=cl_next;
-       }
-    }
+
+  /* this needn't be thread safe:
+     you use rfbRunEventLoop(..,TRUE) for pthreads. */
+  cl=rfbScreen->rfbClientHead;
+  while(cl) {
+    cl_next=cl->next;
+    if(cl->sock>=0 && FB_UPDATE_PENDING(cl))
+      rfbSendFramebufferUpdate(cl,cl->modifiedRegion);
+    if(cl->sock==-1)
+      rfbClientConnectionGone(cl);
+    cl=cl_next;
+  }
 }
 
 void rfbRunEventLoop(rfbScreenInfoPtr rfbScreen, long usec, Bool runInBackground)
@@ -441,11 +445,12 @@ void rfbRunEventLoop(rfbScreenInfoPtr rfbScreen, long usec, Bool runInBackground
 #ifdef HAVE_PTHREADS
        pthread_t listener_thread;
 
-       //pthread_mutex_init(&logMutex, NULL);
+       pthread_mutex_init(&logMutex, NULL);
        pthread_create(&listener_thread, NULL, listenerRun, rfbScreen);
     return;
 #else
     fprintf(stderr,"Can't run in background, because I don't have PThreads!\n");
+    exit(-1);
 #endif
   }
 

rfb.h

@@ -282,9 +282,6 @@ typedef struct rfbClientRec {
    
     int sock;
     char *host;
-#ifdef HAVE_PTHREADS
-    pthread_mutex_t outputMutex;
-#endif
                                 /* Possible client states: */
     enum {
         RFB_PROTOCOL_VERSION,   /* establishing protocol version */
@@ -333,6 +330,8 @@ typedef struct rfbClientRec {
 
 
 #ifdef HAVE_PTHREADS
+    pthread_mutex_t dontKillMutex; /* if you need a reliable clientPtr */
+    pthread_mutex_t outputMutex;
     pthread_mutex_t updateMutex;
     pthread_cond_t updateCond;
 #endif

rfbserver.c

@@ -262,8 +262,10 @@ rfbClientConnectionGone(cl)
     int i;
 
 #ifdef HAVE_PTHREADS
-    pthread_mutex_lock(&cl->updateMutex);
-    pthread_mutex_lock(&cl->outputMutex);
+    /*
+      pthread_mutex_lock(&cl->updateMutex);
+      pthread_mutex_lock(&cl->outputMutex);
+    */
     pthread_mutex_lock(&rfbClientListMutex);
 #endif
 
@@ -815,15 +817,15 @@ rfbProcessClientNormalMessage(cl)
  */
 
 Bool
-rfbSendFramebufferUpdate(cl, updateRegion)
+rfbSendFramebufferUpdate(cl, givenUpdateRegion)
      rfbClientPtr cl;
-     sraRegionPtr updateRegion;
+     sraRegionPtr givenUpdateRegion;
 {
     sraRectangleIterator* i;
     sraRect rect;
     int nUpdateRegionRects;
     rfbFramebufferUpdateMsg *fu = (rfbFramebufferUpdateMsg *)cl->updateBuf;
-    sraRegionPtr updateCopyRegion;
+    sraRegionPtr updateRegion,updateCopyRegion;
     int dx, dy;
     Bool sendCursorShape = FALSE;
     Bool cursorWasDrawn = FALSE;
@@ -862,8 +864,8 @@ rfbSendFramebufferUpdate(cl, updateRegion)
      * no update is needed.
      */
 
-    updateRegion = sraRgnCreateRgn(cl->copyRegion);
-    sraRgnOr(updateRegion,cl->modifiedRegion);
+    updateRegion = sraRgnCreateRgn(givenUpdateRegion);
+    sraRgnOr(updateRegion,cl->copyRegion);
     if(!sraRgnAnd(updateRegion,cl->requestedRegion) && !sendCursorShape) {
       sraRgnDestroy(updateRegion);
       return TRUE;
@@ -1209,14 +1211,6 @@ Bool
 rfbSendUpdateBuf(cl)
     rfbClientPtr cl;
 {
-    /*
-    int i;
-    for (i = 0; i < cl->ublen; i++) {
-        fprintf(stderr,"%02x ",((unsigned char *)cl->updateBuf)[i]);
-    }
-    fprintf(stderr,"\n");
-    */
-
     if (WriteExact(cl, cl->updateBuf, cl->ublen) < 0) {
         rfbLogPerror("rfbSendUpdateBuf: write");
         rfbCloseClient(cl);

sockets.c

@@ -141,7 +141,8 @@ rfbCheckFds(rfbScreenInfoPtr rfbScreen,long usec)
     char buf[6];
     const int one = 1;
     int sock;
-    rfbClientPtr cl;
+    rfbClientIteratorPtr i;
+    rfbClientPtr cl,cl_next;
 
     if (!rfbScreen->inetdInitDone && rfbScreen->inetdSock != -1) {
 	rfbNewClientConnection(rfbScreen,rfbScreen->inetdSock); 
@@ -232,10 +233,33 @@ rfbCheckFds(rfbScreenInfoPtr rfbScreen,long usec)
 	    return;
     }
 
-    for (cl = rfbScreen->rfbClientHead; cl; cl=cl->next) {
-	if (FD_ISSET(cl->sock, &fds) && FD_ISSET(cl->sock, &(rfbScreen->allFds))) {
-	    rfbProcessClientMessage(cl);
-	}
+    /* I know that this is horrible. But we have the following problem:
+       inside this loop, the IO functions access the clients via the
+       iterator.
+       So we have to lock rfbClientListMutex to fetch a reliable
+       rfbClientHead. Remember, a client can just go away in a multithreaded
+       environment. So we have to lock the next client before working with
+       the current.
+    */
+    i = rfbGetClientIterator(rfbScreen);
+    cl = rfbClientIteratorNext(i);
+    if(cl) {
+#ifdef HAVE_PTHREADS
+      //pthread_mutex_lock(&cl->updateMutex);
+#endif
+    }
+    rfbReleaseClientIterator(i);
+
+    while(cl) {
+      cl_next = cl->next;
+#ifdef HAVE_PTHREADS
+      //pthread_mutex_unlock(&cl->updateMutex);
+      //if(cl_next)
+	//pthread_mutex_lock(&cl_next->updateMutex);
+#endif
+      if (FD_ISSET(cl->sock, &fds) && FD_ISSET(cl->sock, &(rfbScreen->allFds)))
+	rfbProcessClientMessage(cl);
+      cl=cl_next;
     }
 }
 
@@ -257,8 +281,8 @@ rfbCloseClient(cl)
     close(cl->sock);
     cl->sock = -1;
     pthread_cond_signal(&cl->updateCond);
-    pthread_mutex_lock(&cl->updateMutex);
-    rfbClientConnectionGone(cl);
+    //pthread_mutex_lock(&cl->updateMutex);
+    //rfbClientConnectionGone(cl);
     pthread_mutex_unlock(&cl->updateMutex);
 }