[PATCH 0/2] Patches to improve some edge cases in internal client connections

[PATCH 0/2] Patches to improve some edge cases in internal client connections

[Jon TURNEY]

Here's a couple of patches provoked by a problem report on IRC the last week,
where a user had some kind of firewall problem which prevented the X server
from accepting connections from it's internal clients (which use a hard-coded
DISPLAY of 127.0.0.1:0.0 at the moment), which in -multwindow mode, results
in windows with no frame or decoration (as the window styling is done by the
internal WM thread) and no explanation.

However, I'm not terribly happy with either of them, so more polish needed.
Problems:

[01/02]
libxtrans provides a TransNoListen method to set the 'don't listen' flag for a
particular transport, but there is no interface to query the state of that flag,
so I had to resort to grovelling around in the server's list of listeners

[02/02]
Calling FatalError from an internal client thread is a no-no, so these failures
are silent at the moment (in the sense that no pop-up occurs to tell the user
that we are terminating or why).

Also, I'm just noticing I should be using pthread_kill() to ensure the signal
is delivered to the thread which is running the main dispatch loop so that will
exit the select() it may be blocked in and notice that DE_TERMINATE is set.



Jon TURNEY (2):
  Cygwin/X: Improve choice of display name used by internal clients
  Cygwin/X: Cause the X server to terminate if clipboard or WM internal
    client threads exit due to an error

 hw/xwin/Makefile.am          |    1 +
 hw/xwin/win.h                |    7 +++++
 hw/xwin/winclipboardthread.c |   22 +++++++++++++---
 hw/xwin/windisplay.c         |   55 ++++++++++++++++++++++++++++++++++++++++++
 hw/xwin/winmultiwindowwm.c   |   32 +++++++++++++++++-------
 hw/xwin/winprefs.c           |   11 ++------
 include/os.h                 |    2 +
 os/connection.c              |   17 +++++++++++++
 8 files changed, 126 insertions(+), 21 deletions(-)
 create mode 100644 hw/xwin/windisplay.c


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/

[PATCH 1/2] Cygwin/X: Improve choice of display name used by internal clients

[Jon TURNEY]

Choose display name used to connect internal clients and exported into
environment of processes started by traymenu so that it uses a transport
we know is working

This should mean the server can start correctly with -multiwindow and/or
-clipboard and any two of -nolisten inet6 -nolisten inet and -nolisten unix
(the server will correctly refuse to start if all 3 are used, as it must
be listening on at least one socket)

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 hw/xwin/Makefile.am          |    1 +
 hw/xwin/win.h                |    7 +++++
 hw/xwin/winclipboardthread.c |    5 +---
 hw/xwin/windisplay.c         |   55 ++++++++++++++++++++++++++++++++++++++++++
 hw/xwin/winmultiwindowwm.c   |   10 +------
 hw/xwin/winprefs.c           |   11 ++------
 include/os.h                 |    2 +
 os/connection.c              |   17 +++++++++++++
 8 files changed, 88 insertions(+), 20 deletions(-)
 create mode 100644 hw/xwin/windisplay.c

diff --git a/hw/xwin/Makefile.am b/hw/xwin/Makefile.am
index 2c7972a..70f8aa5 100644
--- a/hw/xwin/Makefile.am
+++ b/hw/xwin/Makefile.am
@@ -120,6 +120,7 @@ SRCS =	InitInput.c \
 	winpriv.h \
 	winresource.h \
 	winwindow.h \
+	windisplay.c \
 	XWin.rc \
 	$(top_srcdir)/Xext/dpmsstubs.c \
 	$(top_srcdir)/Xi/stubs.c \
diff --git a/hw/xwin/win.h b/hw/xwin/win.h
index 9009df2..b76ac87 100644
--- a/hw/xwin/win.h
+++ b/hw/xwin/win.h
@@ -1454,6 +1454,13 @@ Bool
 winInitCursor (ScreenPtr pScreen);
 
 /*
+ * windisplay.c
+ */
+
+void
+winGetDisplayName(char *szDisplay, unsigned int screen);
+
+/*
  * END DDX and DIX Function Prototypes
  */
 
diff --git a/hw/xwin/winclipboardthread.c b/hw/xwin/winclipboardthread.c
index 8eb825f..fefd408 100644
--- a/hw/xwin/winclipboardthread.c
+++ b/hw/xwin/winclipboardthread.c
@@ -177,10 +177,7 @@ winClipboardProc (void *pvNotUsed)
    * for all screens on the display.  That is why there is only
    * one clipboard client thread.
    */
-  snprintf (szDisplay,
-	    512,
-	    "127.0.0.1:%s.0",
-	    display);
+  winGetDisplayName(szDisplay, 0);
 
   /* Print the display connection string */
   ErrorF ("winClipboardProc - DISPLAY=%s\n", szDisplay);
diff --git a/hw/xwin/windisplay.c b/hw/xwin/windisplay.c
new file mode 100644
index 0000000..3561aa0
--- /dev/null
+++ b/hw/xwin/windisplay.c
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) Jon TURNEY 2009
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#include <opaque.h>  // for display
+#include "win.h"
+
+/*
+  Generate a display name string referring to the display of this server,
+  using a transport we know is enabled
+*/
+
+void
+winGetDisplayName(char *szDisplay, unsigned int screen)
+{
+  if (TransIsListening("local"))
+    {
+      snprintf(szDisplay, 512, ":%s.%d", display, screen);
+    }
+  else if (TransIsListening("inet"))
+    {
+      snprintf(szDisplay, 512, "127.0.0.1:%s.%d", display, screen);
+    }
+  else if (TransIsListening("inet6"))
+    {
+      snprintf(szDisplay, 512, "::1:%s.%d", display, screen);
+    }
+  else
+    {
+      // this can't happen!
+      snprintf(szDisplay, 512, "localhost:%s.%d", display, screen);
+    }
+
+  ErrorF ("winGetDisplay: DISPLAY=%s\n", szDisplay);
+}
diff --git a/hw/xwin/winmultiwindowwm.c b/hw/xwin/winmultiwindowwm.c
index 18d9aed..0de607d 100644
--- a/hw/xwin/winmultiwindowwm.c
+++ b/hw/xwin/winmultiwindowwm.c
@@ -137,7 +137,6 @@ typedef struct _XMsgProcArgRec {
  * References to external symbols
  */
 
-extern char *display;
 extern void ErrorF (const char* /*f*/, ...);
 
 
@@ -938,8 +937,7 @@ winMultiWindowXMsgProc (void *pArg)
   XSetIOErrorHandler (winMultiWindowXMsgProcIOErrorHandler);
 
   /* Setup the display connection string x */
-  snprintf (pszDisplay,
-	    512, "127.0.0.1:%s.%d", display, (int)pProcArg->dwScreen);
+  winGetDisplayName(pszDisplay, (int)pProcArg->dwScreen);
 
   /* Print the display connection string */
   ErrorF ("winMultiWindowXMsgProc - DISPLAY=%s\n", pszDisplay);
@@ -1266,11 +1264,7 @@ winInitMultiWindowWM (WMInfoPtr pWMInfo, WMProcArgPtr pProcArg)
   XSetIOErrorHandler (winMultiWindowWMIOErrorHandler);
 
   /* Setup the display connection string x */
-  snprintf (pszDisplay,
-	    512,
-	    "127.0.0.1:%s.%d",
-	    display,
-	    (int) pProcArg->dwScreen);
+  winGetDisplayName(pszDisplay, (int)pProcArg->dwScreen);
 
   /* Print the display connection string */
   ErrorF ("winInitMultiWindowWM - DISPLAY=%s\n", pszDisplay);
diff --git a/hw/xwin/winprefs.c b/hw/xwin/winprefs.c
index d5bceb9..d854d2e 100644
--- a/hw/xwin/winprefs.c
+++ b/hw/xwin/winprefs.c
@@ -69,10 +69,6 @@ extern HICON		g_hSmallIconX;
 /* Currently in use command ID, incremented each new menu item created */
 static int g_cmdid = STARTMENUID;
 
-
-/* Defined in DIX */
-extern char *display;
-
 /* Local function to handle comma-ified icon names */
 static HICON
 LoadImageComma (char *fname, int sx, int sy, int flags);
@@ -780,16 +776,15 @@ LoadPreferences (void)
 
   /* Setup a DISPLAY environment variable, need to allocate on heap */
   /* because putenv doesn't copy the argument... */
-  snprintf (szDisplay, 512, "DISPLAY=127.0.0.1:%s.0", display);
-  szEnvDisplay = (char *)(malloc (strlen(szDisplay)+1));
+  winGetDisplayName(szDisplay, 0);
+  szEnvDisplay = (char *)(malloc(strlen(szDisplay)+strlen("DISPLAY=")+1));
   if (szEnvDisplay)
     {
-      strcpy (szEnvDisplay, szDisplay);
+      snprintf(szEnvDisplay, 512, "DISPLAY=%s", szDisplay);
       putenv (szEnvDisplay);
     }
 
   /* Replace any "%display%" in menu commands with display string */
-  snprintf (szDisplay, 512, "127.0.0.1:%s.0", display);
   for (i=0; i<pref.menuItems; i++)
     {
       for (j=0; j<pref.menu[i].menuItems; j++)
diff --git a/include/os.h b/include/os.h
index 2f6b0c0..174ffd3 100644
--- a/include/os.h
+++ b/include/os.h
@@ -110,6 +110,8 @@ extern _X_EXPORT int WriteToClient(ClientPtr /*who*/, int /*count*/, const void*
 
 extern _X_EXPORT void ResetOsBuffers(void);
 
+extern _X_EXPORT int TransIsListening(char *protocol);
+
 extern _X_EXPORT void InitConnectionLimits(void);
 
 extern _X_EXPORT void NotifyParentProcess(void);
diff --git a/os/connection.c b/os/connection.c
index 3ff93bb..5ea55ae 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -270,6 +270,23 @@ lookup_trans_conn (int fd)
     return (NULL);
 }
 
+int
+TransIsListening(char *protocol)
+{
+  /* look for this transport in the list of listeners */
+  int i;
+  for (i = 0; i < ListenTransCount; i++)
+    {
+      if (!strcmp(protocol, ListenTransConns[i]->transptr->TransName))
+        {
+          return 1;
+        }
+    }
+
+  return 0;
+}
+
+
 /* Set MaxClients and lastfdesc, and allocate ConnectionTranslation */
 
 void
-- 
1.6.4.2


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/

[PATCH 2/2] Cygwin/X: Cause the X server to terminate if clipboard or WM internal client threads exit due to an error

[Jon TURNEY]

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 hw/xwin/winclipboardthread.c |   17 +++++++++++++++++
 hw/xwin/winmultiwindowwm.c   |   22 +++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/hw/xwin/winclipboardthread.c b/hw/xwin/winclipboardthread.c
index fefd408..962d9d6 100644
--- a/hw/xwin/winclipboardthread.c
+++ b/hw/xwin/winclipboardthread.c
@@ -84,6 +84,8 @@ winClipboardErrorHandler (Display *pDisplay, XErrorEvent *pErr);
 static int
 winClipboardIOErrorHandler (Display *pDisplay);
 
+static void
+winClipboardThreadExit(void *arg);
 
 /*
  * Main thread function
@@ -110,6 +112,8 @@ winClipboardProc (void *pvNotUsed)
   char			szDisplay[512];
   int			iSelectError;
 
+  pthread_cleanup_push(&winClipboardThreadExit, NULL);
+
   ErrorF ("winClipboardProc - Hello\n");
 
   /* Do we have Unicode support? */
@@ -434,6 +438,8 @@ winClipboardProc (void *pvNotUsed)
   g_pClipboardDisplay = NULL;
   g_hwndClipboard = NULL;
 
+  pthread_cleanup_pop(0);
+
   return NULL;
 }
 
@@ -475,3 +481,14 @@ winClipboardIOErrorHandler (Display *pDisplay)
   
   return 0;
 }
+
+/*
+ * winClipboardThreadExit - Thread exit handler
+ */
+
+static void
+winClipboardThreadExit(void *arg)
+{
+  /* clipboard thread has exited, stop server as well */
+  kill(getpid(), SIGTERM);
+}
diff --git a/hw/xwin/winmultiwindowwm.c b/hw/xwin/winmultiwindowwm.c
index 0de607d..87cb3dc 100644
--- a/hw/xwin/winmultiwindowwm.c
+++ b/hw/xwin/winmultiwindowwm.c
@@ -180,6 +180,9 @@ winMultiWindowXMsgProcErrorHandler (Display *pDisplay, XErrorEvent *pErr);
 static int
 winMultiWindowXMsgProcIOErrorHandler (Display *pDisplay);
 
+static void
+winMultiWindowThreadExit(void *arg);
+
 static int
 winRedirectErrorHandler (Display *pDisplay, XErrorEvent *pErr);
 
@@ -637,6 +640,8 @@ winMultiWindowWMProc (void *pArg)
 {
   WMProcArgPtr		pProcArg = (WMProcArgPtr)pArg;
   WMInfoPtr		pWMInfo = pProcArg->pWMInfo;
+
+  pthread_cleanup_push(&winMultiWindowThreadExit, NULL);
   
   /* Initialize the Window Manager */
   winInitMultiWindowWM (pWMInfo, pProcArg);
@@ -849,6 +854,9 @@ winMultiWindowWMProc (void *pArg)
 #if CYGMULTIWINDOW_DEBUG
   ErrorF("-winMultiWindowWMProc ()\n");
 #endif
+
+  pthread_cleanup_pop(0);
+
   return NULL;
 }
 
@@ -871,6 +879,8 @@ winMultiWindowXMsgProc (void *pArg)
   int			iReturn;
   XIconSize		*xis;
 
+  pthread_cleanup_push(&winMultiWindowThreadExit, NULL);
+
   ErrorF ("winMultiWindowXMsgProc - Hello\n");
 
   /* Check that argument pointer is not invalid */
@@ -1108,7 +1118,7 @@ winMultiWindowXMsgProc (void *pArg)
     }
 
   XCloseDisplay (pProcArg->pDisplay);
-  pthread_exit (NULL);
+  pthread_cleanup_pop(0);
   return NULL;
 }
 
@@ -1428,6 +1438,16 @@ winMultiWindowXMsgProcIOErrorHandler (Display *pDisplay)
   return 0;
 }
 
+/*
+ * winMultiWindowThreadExit - Thread exit handler
+ */
+
+static void
+winMultiWindowThreadExit(void *arg)
+{
+  /* multiwindow client thread has exited, stop server as well */
+  kill(getpid(), SIGTERM);
+}
 
 /*
  * Catch RedirectError to detect other window manager running
-- 
1.6.4.2


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/

Re: [PATCH 1/2] Cygwin/X: Improve choice of display name used by internal clients

[Yaakov (Cygwin/X)]

On 01/10/2009 07:12, Jon TURNEY wrote:
> +  if (TransIsListening("local"))
> +    {
> +      snprintf(szDisplay, 512, ":%s.%d", display, screen);
> +    }
> +  else if (TransIsListening("inet"))
> +    {
> +      snprintf(szDisplay, 512, "127.0.0.1:%s.%d", display, screen);
> +    }
> +  else if (TransIsListening("inet6"))
> +    {
> +      snprintf(szDisplay, 512, "::1:%s.%d", display, screen);
> +    }

Perhaps we should be giving priority to inet6 over inet if it's available?

> +  else
> +    {
> +      // this can't happen!
> +      snprintf(szDisplay, 512, "localhost:%s.%d", display, screen);
> +    }

If this can't happen, an ErrorF would be appropriate.


Yaakov
Cygwin/X

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/

Re: [PATCH 1/2] Cygwin/X: Improve choice of display name used by internal clients

[Jon TURNEY]

On 07/10/2009 03:16, Yaakov (Cygwin/X) wrote:
> On 01/10/2009 07:12, Jon TURNEY wrote:
>> + if (TransIsListening("local"))
>> + {
>> + snprintf(szDisplay, 512, ":%s.%d", display, screen);
>> + }
>> + else if (TransIsListening("inet"))
>> + {
>> + snprintf(szDisplay, 512, "127.0.0.1:%s.%d", display, screen);
>> + }
>> + else if (TransIsListening("inet6"))
>> + {
>> + snprintf(szDisplay, 512, "::1:%s.%d", display, screen);
>> + }
>
> Perhaps we should be giving priority to inet6 over inet if it's available?

I don't know. I didn't think very much about the order.

I guess we should prefer inet first, as that's what we are doing currently, so 
at least in theory we know that works.   I don't know if there's any 
performance reason to prefer a unix socket (I think they are implemented using 
a IP socket anyhow?).

What reasons are there for preferring inet6?

>> + else
>> + {
>> + // this can't happen!
>> + snprintf(szDisplay, 512, "localhost:%s.%d", display, screen);
>> + }
>
> If this can't happen, an ErrorF would be appropriate.

Good catch, thanks.

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/