[v2] os: Add -displayfd option

Submitted by Chase Douglas on April 3, 2012, 9:25 p.m.

Details

Message ID 1333488309-544-1-git-send-email-chase.douglas@canonical.com
State Accepted
Headers show

Not browsing as part of any series.

Commit Message

Chase Douglas April 3, 2012, 9:25 p.m.
This option specifies a file descriptor in the launching process.  X
will scan for an available display number and write that number back to
the launching process, at the same time as SIGUSR1 generation.  This
means display managers don't need to guess at available display numbers.
As a consequence, if X fails to start when using -displayfd, it's not
because the display was in use, so there's no point in retrying the X
launch on a higher display number.

Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Tested-by: Julien Cristau <jcristau@debian.org>
---
Changes from previous version:
* Check correct return value of TryCreateSocket() in displayfd code path

 dix/globals.c    |    1 +
 include/opaque.h |    1 +
 man/Xserver.man  |    7 +++++
 os/connection.c  |   65 +++++++++++++++++++++++++++++++++++++----------------
 os/utils.c       |    9 +++++++
 5 files changed, 63 insertions(+), 20 deletions(-)

Patch hide | download patch | download mbox

diff --git a/dix/globals.c b/dix/globals.c
index a564575..332b91f 100644
--- a/dix/globals.c
+++ b/dix/globals.c
@@ -128,6 +128,7 @@  int defaultColorVisualClass = -1;
 int monitorResolution = 0;
 
 char *display;
+int displayfd;
 char *ConnectionInfo;
 
 CARD32 TimeOutValue = DEFAULT_TIMEOUT * MILLI_PER_SECOND;
diff --git a/include/opaque.h b/include/opaque.h
index 9ca408a..b76ab6e 100644
--- a/include/opaque.h
+++ b/include/opaque.h
@@ -50,6 +50,7 @@  extern _X_EXPORT int ScreenSaverAllowExposures;
 extern _X_EXPORT int defaultScreenSaverBlanking;
 extern _X_EXPORT int defaultScreenSaverAllowExposures;
 extern _X_EXPORT char *display;
+extern _X_EXPORT int displayfd;
 
 extern _X_EXPORT int defaultBackingStore;
 extern _X_EXPORT Bool disableBackingStore;
diff --git a/man/Xserver.man b/man/Xserver.man
index 0cd9b94..8d243d6 100644
--- a/man/Xserver.man
+++ b/man/Xserver.man
@@ -127,6 +127,13 @@  Not obeyed by all servers.
 .B \-core
 causes the server to generate a core dump on fatal errors.
 .TP 8
+.B \-displayfd \fIfd\fP
+specifies a file descriptor in the launching process.  Rather than specify
+a display number, the X server will attempt to listen on successively higher
+display numbers, and upon finding a free one, will write the port number back
+on this file descriptor as a newline-terminated string.  The \-pn option is
+ignored when using \-displayfd.
+.TP 8
 .B \-deferglyphs \fIwhichfonts\fP
 specifies the types of fonts for which the server should attempt to use
 deferred glyph loading.  \fIwhichfonts\fP can be all (all fonts),
diff --git a/os/connection.c b/os/connection.c
index 1099752..028f588 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -142,6 +142,7 @@  Bool AnyClientsWriteBlocked;    /* true if some client blocked on write */
 static Bool RunFromSmartParent; /* send SIGUSR1 to parent process */
 Bool RunFromSigStopParent;      /* send SIGSTOP to our own process; Upstart (or
                                    equivalent) will send SIGCONT back. */
+static char dynamic_display[7]; /* display name */
 Bool PartialNetwork;            /* continue even if unable to bind all addrs */
 static Pid_t ParentProcess;
 
@@ -350,6 +351,10 @@  void
 NotifyParentProcess(void)
 {
 #if !defined(WIN32)
+    if (dynamic_display[0]) {
+        write(displayfd, dynamic_display, strlen(dynamic_display));
+        close(displayfd);
+    }
     if (RunFromSmartParent) {
         if (ParentProcess > 1) {
             kill(ParentProcess, SIGUSR1);
@@ -360,6 +365,18 @@  NotifyParentProcess(void)
 #endif
 }
 
+static Bool
+TryCreateSocket(int num, int *partial)
+{
+    char port[20];
+
+    sprintf(port, "%d", num);
+
+    return (_XSERVTransMakeAllCOTSServerListeners(port, partial,
+                                                  &ListenTransCount,
+                                                  &ListenTransConns) >= 0);
+}
+
 /*****************
  * CreateWellKnownSockets
  *    At initialization, create the sockets to listen on for new clients.
@@ -370,7 +387,6 @@  CreateWellKnownSockets(void)
 {
     int i;
     int partial;
-    char port[20];
 
     FD_ZERO(&AllSockets);
     FD_ZERO(&AllClients);
@@ -386,29 +402,38 @@  CreateWellKnownSockets(void)
 
     FD_ZERO(&WellKnownConnections);
 
-    snprintf(port, sizeof(port), "%d", atoi(display));
-
-    if ((_XSERVTransMakeAllCOTSServerListeners(port, &partial,
-                                               &ListenTransCount,
-                                               &ListenTransConns) >= 0) &&
-        (ListenTransCount >= 1)) {
-        if (!PartialNetwork && partial) {
-            FatalError("Failed to establish all listening sockets");
+    if (display) {
+        if (TryCreateSocket(atoi(display), &partial) &&
+            ListenTransCount >= 1)
+            if (!PartialNetwork && partial)
+                FatalError ("Failed to establish all listening sockets");
+    }
+    else { /* -displayfd */
+        Bool found = 0;
+        for (i = 0; i < 65535 - X_TCP_PORT; i++) {
+            if (TryCreateSocket(i, &partial) && !partial) {
+                found = 1;
+                break;
+            }
+            else
+                CloseWellKnownConnections();
         }
-        else {
-            ListenTransFds = malloc(ListenTransCount * sizeof(int));
+        if (!found)
+            FatalError("Failed to find a socket to listen on");
+        sprintf(dynamic_display, "%d\n", i);
+        display = dynamic_display;
+    }
 
-            for (i = 0; i < ListenTransCount; i++) {
-                int fd = _XSERVTransGetConnectionNumber(ListenTransConns[i]);
+    ListenTransFds = malloc(ListenTransCount * sizeof (int));
 
-                ListenTransFds[i] = fd;
-                FD_SET(fd, &WellKnownConnections);
+    for (i = 0; i < ListenTransCount; i++) {
+        int fd = _XSERVTransGetConnectionNumber(ListenTransConns[i]);
 
-                if (!_XSERVTransIsLocal(ListenTransConns[i])) {
-                    DefineSelf(fd);
-                }
-            }
-        }
+        ListenTransFds[i] = fd;
+        FD_SET(fd, &WellKnownConnections);
+
+        if (!_XSERVTransIsLocal(ListenTransConns[i]))
+            DefineSelf (fd);
     }
 
     if (!XFD_ANYSET(&WellKnownConnections))
diff --git a/os/utils.c b/os/utils.c
index 30592d2..3a1ef93 100644
--- a/os/utils.c
+++ b/os/utils.c
@@ -659,6 +659,15 @@  ProcessCommandLine(int argc, char *argv[])
             else
                 UseMsg();
         }
+        else if (strcmp(argv[i], "-displayfd") == 0) {
+            if (++i < argc) {
+                displayfd = atoi(argv[i]);
+                display = NULL;
+                nolock = TRUE;
+            }
+            else
+                UseMsg();
+        }
 #ifdef DPMSExtension
         else if (strcmp(argv[i], "dpms") == 0)
             /* ignored for compatibility */ ;

Comments

On 04/ 3/12 02:25 PM, Chase Douglas wrote:
> +static Bool
> +TryCreateSocket(int num, int *partial)
> +{
> +    char port[20];
> +
> +    sprintf(port, "%d", num);
> +
> +    return (_XSERVTransMakeAllCOTSServerListeners(port, partial,
> +                                                  &ListenTransCount,
> +                                                  &ListenTransConns) >= 0);
> +}
> +

I'd prefer if this stayed with snprintf like the old code did:

> -    snprintf(port, sizeof(port), "%d", atoi(display));

Not because there's any chance of it overflowing, but because it's one less
place we have to check to be sure sprintf can't overflow.

> +    if (display) {
[...]
> +    }
> +    else { /* -displayfd */

I had to go digging through the code before I could be convinced this was
a safe assumption to make.   Perhaps a comment somewhere like dix/globals.c
should state that display is initialized to "0" by main, and then set by
ProcessCommandLine to either a command line specified display, or in the
case of -displayfd to NULL, would be helpful for future understanding.

Definitely in favor of adding this support though, thanks for finishing it off.
On 03/04/2012 22:25, Chase Douglas wrote:
> This option specifies a file descriptor in the launching process.  X
> will scan for an available display number and write that number back to
> the launching process, at the same time as SIGUSR1 generation.  This
> means display managers don't need to guess at available display numbers.
> As a consequence, if X fails to start when using -displayfd, it's not
> because the display was in use, so there's no point in retrying the X
> launch on a higher display number.
> 
> Signed-off-by: Adam Jackson <ajax-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Chase Douglas <chase.douglas-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Reviewed-by: Julien Cristau <jcristau-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> Tested-by: Julien Cristau <jcristau-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> ---

Very much in favour of adding this.

> +    else { /* -displayfd */
> +        Bool found = 0;
> +        for (i = 0; i < 65535 - X_TCP_PORT; i++) {
> +            if (TryCreateSocket(i, &partial) && !partial) {
> +                found = 1;
> +                break;
> +            }
> +            else
> +                CloseWellKnownConnections();
>          }
> -        else {
> -            ListenTransFds = malloc(ListenTransCount * sizeof(int));
> +        if (!found)
> +            FatalError("Failed to find a socket to listen on");
> +        sprintf(dynamic_display, "%d\n", i);
> +        display = dynamic_display;

Would you mind changing this so it doesn't inconsistently add a trailing '\n'
to display if -displayfd was used. (This is difficulty for XWin as we
subsequently use the value of display to point our helper clients at the right
display)