waitforx logging improvements

sesman/session.c

@@ -350,9 +350,6 @@ session_start_chansrv(int uid, int display)
     chansrv_pid = g_fork();
     if (chansrv_pid == 0)
     {
-        LOG(LOG_LEVEL_INFO,
-            "Starting the xrdp channel server for display %d", display);
-
         chansrv_params = list_create();
         chansrv_params->auto_free = 1;
 
@@ -364,6 +361,9 @@ session_start_chansrv(int uid, int display)
                      g_cfg->env_names,
                      g_cfg->env_values);
 
+        LOG(LOG_LEVEL_INFO,
+            "Starting the xrdp channel server for display %d", display);
+
         /* executing chansrv */
         g_execvp_list(exe_path, chansrv_params);
 
@@ -585,12 +585,16 @@ session_start(struct auth_info *auth_info,
         }
         else if (window_manager_pid == 0)
         {
+            enum xwait_status xws;
+
             env_set_user(s->uid,
                          0,
                          display,
                          g_cfg->env_names,
                          g_cfg->env_values);
-            if (wait_for_xserver(display))
+            xws = wait_for_xserver(display);
+
+            if (xws == XW_STATUS_OK)
             {
                 auth_set_env(auth_info);
                 if (s->directory != 0)
@@ -661,8 +665,18 @@ session_start(struct auth_info *auth_info,
             }
             else
             {
-                LOG(LOG_LEVEL_ERROR,
+                switch (xws)
-                    "There is no X server active on display %d", display);
+                {
+                    case XW_STATUS_TIMED_OUT:
+                        LOG(LOG_LEVEL_ERROR, "Timed out waiting for X server");
+                        break;
+                    case XW_STATUS_FAILED_TO_START:
+                        LOG(LOG_LEVEL_ERROR, "X server failed to start");
+                        break;
+                    default:
+                        LOG(LOG_LEVEL_ERROR,
+                            "An error occurred waiting for the X server");
+                }
             }
 
             LOG(LOG_LEVEL_ERROR, "A fatal error has occurred attempting to start "

sesman/xwait.c

@@ -9,39 +9,82 @@
 
 #include <stdio.h>
 #include <string.h>
+#include <sys/wait.h>
 
 /******************************************************************************/
-int
+enum xwait_status
 wait_for_xserver(int display)
 {
     FILE *dp = NULL;
-    int ret = 0;
+    enum xwait_status rv = XW_STATUS_MISC_ERROR;
+    int ret;
+
     char buffer[100];
-    char exe_cmd[262];
+    const char exe[] = XRDP_LIBEXEC_PATH "/waitforx";
+    char cmd[sizeof(exe) + 64];
 
-    LOG(LOG_LEVEL_DEBUG, "Waiting for X server to start on display %d", display);
+    if (!g_file_exist(exe))
+    {
+        LOG(LOG_LEVEL_ERROR, "Unable to find %s", exe);
+        return rv;
+    }
 
-    g_snprintf(exe_cmd, sizeof(exe_cmd), "%s/waitforx", XRDP_LIBEXEC_PATH);
+    g_snprintf(cmd, sizeof(cmd), "%s -d :%d", exe, display);
-    dp = popen(exe_cmd, "r");
+    LOG(LOG_LEVEL_DEBUG, "Waiting for X server to start on display :%d",
+        display);
+
+    dp = popen(cmd, "r");
     if (dp == NULL)
     {
         LOG(LOG_LEVEL_ERROR, "Unable to launch waitforx");
-        return 1;
+        return rv;
     }
 
     while (fgets(buffer, 100, dp))
     {
+        const char *msg = buffer;
+        enum logLevels level = LOG_LEVEL_ERROR;
+
         g_strtrim(buffer, 2);
-        LOG(LOG_LEVEL_DEBUG, "%s", buffer);
+
+        // Has the message got a class at the start?
+        if (strlen(buffer) > 3 && buffer[0] == '<' && buffer[2] == '>')
+        {
+            switch (buffer[1])
+            {
+                case 'D':
+                    level = LOG_LEVEL_DEBUG;
+                    break;
+                case 'I':
+                    level = LOG_LEVEL_INFO;
+                    break;
+                case 'W':
+                    level = LOG_LEVEL_WARNING;
+                    break;
+                default:
+                    level = LOG_LEVEL_ERROR;
+                    break;
+            }
+            msg = buffer + 3;
+        }
+
+        if (strlen(msg) > 0)
+        {
+            LOG(level, "waitforx: %s", msg);
+        }
     }
 
     ret = pclose(dp);
-    if (ret != 0)
+    if (WIFEXITED(ret))
     {
-        LOG(LOG_LEVEL_ERROR, "An error occurred while running waitforx");
+        rv = (enum xwait_status)WEXITSTATUS(ret);
-        return 0;
+    }
+    else if (WIFSIGNALED(ret))
+    {
+        int sig = WTERMSIG(ret);
+        LOG(LOG_LEVEL_ERROR, "waitforx failed with unexpected signal %d",
+            sig);
     }
 
-
+    return rv;
-    return 1;
 }

sesman/xwait.h

@@ -1,12 +1,21 @@
 #ifndef XWAIT_H
 #define XWAIT_H
+
+enum xwait_status
+{
+    XW_STATUS_OK = 0,
+    XW_STATUS_MISC_ERROR,
+    XW_STATUS_TIMED_OUT,
+    XW_STATUS_FAILED_TO_START
+};
+
 /**
  *
  * @brief waits for X to start
  * @param display number
- * @return 0 on error, 1 if X has outputs
+ * @return status
  *
  */
-int
+enum xwait_status
 wait_for_xserver(int display);
 #endif

waitforx/Makefile.am

@@ -3,7 +3,9 @@ pkglibexec_PROGRAMS = \
 
 AM_LDFLAGS = -lX11 -lXrandr
 
-AM_CPPFLAGS = -I$(top_srcdir)/common
+AM_CPPFLAGS = \
+  -I$(top_srcdir)/sesman \
+  -I$(top_srcdir)/common
 
 AM_CFLAGS = $(X_CFLAGS)
 

waitforx/waitforx.c

@@ -8,94 +8,152 @@
 #include "config_ac.h"
 #include "os_calls.h"
 #include "string_calls.h"
+#include "xwait.h" // For return status codes
 
 #define ATTEMPTS 10
 #define ALARM_WAIT 30
 
-void
+/*****************************************************************************/
+static void
 alarm_handler(int signal_num)
 {
-    /* Avoid printf() in signal handler (see signal-safety(7)) */
+    /* Avoid printf() in signal handler (see signal-safety(7))
-    const char msg[] = "Timed out waiting for RandR outputs\n";
+     *
+     * Prefix the message with a newline in case another message
+     * has been partly output */
+    const char msg[] = "\n<E>Timed out waiting for RandR outputs\n";
     g_file_write(1, msg, g_strlen(msg));
-    exit(1);
+    exit(XW_STATUS_TIMED_OUT);
 }
 
-int
+/*****************************************************************************/
-main(int argc, char **argv)
+static Display *
+open_display(const char *display)
 {
-    char *display = NULL;
-    int error_base = 0;
-    int event_base = 0;
-    int n = 0;
-    int outputs = 0;
-    int wait = ATTEMPTS;
-
     Display *dpy = NULL;
-    XRRScreenResources *res = NULL;
+    unsigned int wait = ATTEMPTS;
+    unsigned int n;
 
-    display = getenv("DISPLAY");
+    for (n = 1; n <= ATTEMPTS; ++n)
-
-    g_set_alarm(alarm_handler, ALARM_WAIT);
-
-    if (!display)
-    {
-        printf("DISPLAY is null");
-        exit(1);
-    }
-
-    for (n = 1; n <= wait; ++n)
     {
+        printf("<D>Opening display %s. Attempt %u of %u\n", display, n, wait);
         dpy = XOpenDisplay(display);
-        printf("Opening display %s. Attempt %d of %d\n", display, n, wait);
         if (dpy != NULL)
         {
-            printf("Opened display %s\n", display);
+            printf("<D>Opened display %s\n", display);
             break;
         }
         g_sleep(1000);
     }
 
-    if (!dpy)
+    return dpy;
-    {
+}
-        printf("Unable to open display %s\n", display);
+
-        exit(1);
+/*****************************************************************************/
-    }
+/**
+ * Wait for the RandR extension (if in use) to be available
+ *
+ * @param dpy Display
+ * @return 0 if/when outputs are available, 1 otherwise
+ */
+static int
+wait_for_r_and_r(Display *dpy)
+{
+    int error_base = 0;
+    int event_base = 0;
+    unsigned int outputs = 0;
+    unsigned int wait = ATTEMPTS;
+    unsigned int n;
+
+    XRRScreenResources *res = NULL;
 
     if (!XRRQueryExtension(dpy, &event_base, &error_base))
     {
-        printf("RandR not supported on display %s", display);
+        printf("<I>RandR not supported on display %s\n",
+               DisplayString(dpy));
+        return 0;
     }
-    else
+
+    for (n = 1; n <= wait; ++n)
     {
-        for (n = 1; n <= wait; ++n)
+        printf("<D>Waiting for outputs. Attempt %u of %u\n", n, wait);
+        res = XRRGetScreenResources(dpy, DefaultRootWindow(dpy));
+        if (res != NULL)
         {
-            res = XRRGetScreenResources(dpy, DefaultRootWindow(dpy));
+            if (res->noutput > 0)
-            printf("Waiting for outputs. Attempt %d of %d\n", n, wait);
-            if (res != NULL)
             {
-                if (res->noutput > 0)
+                outputs = res->noutput;
-                {
-                    outputs = res->noutput;
-                    XRRFreeScreenResources(res);
-                    printf("Found %d output[s]\n", outputs);
-                    break;
-                }
-                XRRFreeScreenResources(res);
             }
-            g_sleep(1000);
+            XRRFreeScreenResources(res);
         }
 
         if (outputs > 0)
         {
-            printf("display %s ready with %d outputs\n", display, res->noutput);
+            printf("<D>Display %s ready with %u RandR outputs\n",
+                   DisplayString(dpy), outputs);
+            return 0;
         }
-        else
+        g_sleep(1000);
+    }
+
+    printf("<E>Unable to find any RandR outputs\n");
+    return 1;
+}
+
+/*****************************************************************************/
+static void
+usage(const char *argv0, int status)
+{
+    printf("Usage: %s -d display\n", argv0);
+    exit(status);
+}
+
+/*****************************************************************************/
+int
+main(int argc, char **argv)
+{
+    const char *display_name = NULL;
+    int opt;
+    int status = XW_STATUS_MISC_ERROR;
+    Display *dpy = NULL;
+
+    /* Disable stdout buffering so any messages are passed immediately
+     * to sesman */
+    setvbuf(stdout, NULL, _IONBF, 0);
+
+    while ((opt = getopt(argc, argv, "d:")) != -1)
+    {
+        switch (opt)
         {
-            printf("Unable to find any outputs\n");
+            case 'd':
-            exit(1);
+                display_name = optarg;
+                break;
+            default: /* '?' */
+                usage(argv[0], status);
         }
     }
 
-    exit(0);
+    if (!display_name)
+    {
+        usage(argv[0], status);
+    }
+
+    g_set_alarm(alarm_handler, ALARM_WAIT);
+
+    dpy = open_display(display_name);
+    if (!dpy)
+    {
+        printf("<E>Unable to open display %s\n", display_name);
+        status = XW_STATUS_FAILED_TO_START;
+    }
+    else
+    {
+        if (wait_for_r_and_r(dpy) == 0)
+        {
+            status = XW_STATUS_OK;
+        }
+        XCloseDisplay(dpy);
+    }
+
+    return status;
 }