Merge pull request #3636 from matt335672/optimize_resize_queue

resize queue: drop unactioned client requests

common/os_calls.c

@@ -1855,7 +1855,7 @@ g_delete_wait_obj_from_socket(tintptr wait_obj)
 {
 #ifdef _WIN32
 
-    if (wait_obj == 0)
+    if (wait_obj == NULL_WAIT_OBJ)
     {
         return;
     }
@@ -1879,7 +1879,7 @@ g_set_wait_obj(tintptr obj)
     int to_write;
     char buf[4] = "sig";
 
-    if (obj == 0)
+    if (obj == NULL_WAIT_OBJ)
     {
         return 0;
     }
@@ -1928,7 +1928,7 @@ int
 g_reset_wait_obj(tintptr obj)
 {
 #ifdef _WIN32
-    if (obj == 0)
+    if (obj == NULL_WAIT_OBJ)
     {
         return 0;
     }
@@ -1939,7 +1939,7 @@ g_reset_wait_obj(tintptr obj)
     int error;
     int fd;
 
-    if (obj == 0)
+    if (obj == NULL_WAIT_OBJ)
     {
         return 0;
     }
@@ -1975,7 +1975,7 @@ int
 g_is_wait_obj_set(tintptr obj)
 {
 #ifdef _WIN32
-    if (obj == 0)
+    if (obj == NULL_WAIT_OBJ)
     {
         return 0;
     }
@@ -1985,7 +1985,7 @@ g_is_wait_obj_set(tintptr obj)
     }
     return 0;
 #else
-    if (obj == 0)
+    if (obj == NULL_WAIT_OBJ)
     {
         return 0;
     }
@@ -1999,7 +1999,7 @@ int
 g_delete_wait_obj(tintptr obj)
 {
 #ifdef _WIN32
-    if (obj == 0)
+    if (obj == NULL_WAIT_OBJ)
     {
         return 0;
     }

common/os_calls.h

@@ -198,6 +198,11 @@ g_sck_get_peer_description(int sck,
 void     g_sleep(int msecs);
 int      g_pipe(int fd[2]);
 
+// Wait objects with this value are ignored by
+// g_set_wait_obj() / g_reset_wait_obj() / g_is_wait_obj_set() /
+// g_delete_wait_obj()
+#define NULL_WAIT_OBJ (tintptr)0
+
 tintptr  g_create_wait_obj(const char *name);
 tintptr  g_create_wait_obj_from_socket(tintptr socket, int write);
 void     g_delete_wait_obj_from_socket(tintptr wait_obj);

xrdp/xrdp.h

@@ -496,6 +496,20 @@ struct display_control_monitor_layout_data
     int using_egfx;
 };
 
+enum resize_queue_source
+{
+    RQ_IGNORE_MARKER, // Ignore marker
+    RQ_FROM_SERVER, // Requested by display server / desktop
+    RQ_FROM_CLIENT  // Requested by client (dynamic monitor data)
+};
+
+// Items stored on the resize queue
+struct resize_queue_item
+{
+    enum resize_queue_source src; // Where the item came from
+    struct display_size_description description;
+};
+
 int
 xrdp_mm_drdynvc_up(struct xrdp_mm *self);
 int

xrdp/xrdp_mm.c

@@ -1435,6 +1435,69 @@ sync_dynamic_monitor_data(struct xrdp_wm *wm,
              MAXIMUM_MONITOR_SIZE);
 }
 
+/******************************************************************************/
+/**
+ * Single point to add requests to the resize queue
+ *
+ * @param self xrdp_mm object
+ * @param src Where the request is coming from
+ * @param description The requt we're adding
+ * @return 0 for success
+ */
+static int
+add_resize_request_to_queue(struct xrdp_mm *self,
+                            enum resize_queue_source src,
+                            const struct display_size_description *description)
+{
+    int rv = 0;
+    struct resize_queue_item *resize_queue_item;
+    resize_queue_item = (struct resize_queue_item *)
+                        malloc(sizeof(struct resize_queue_item));
+    if (resize_queue_item == NULL)
+    {
+        rv = 1;
+    }
+    else
+    {
+        resize_queue_item->src = src;
+        resize_queue_item->description = *description;
+
+        // See if there's already a previous item on the queue
+        if (self->resize_queue->count > 0)
+        {
+            int prev_num = self->resize_queue->count - 1;
+            struct resize_queue_item *prev;
+            prev = (struct resize_queue_item *)
+                   list_get_item(self->resize_queue, prev_num);
+            // If this is a RQ_FROM_CLIENT, and the previous item on the
+            // resize_queue is also RQ_FROM_CLIENT, and it hasn't yet been
+            // processed, we can simply ignore it
+            if (src == RQ_FROM_CLIENT && prev->src == RQ_FROM_CLIENT)
+            {
+                LOG_DEVEL(LOG_LEVEL_DEBUG,
+                          "dynamic_monitor_data: Removing unactioned resize request"
+                          " width %d, height %d.",
+                          prev->description.session_width,
+                          prev->description.session_height);
+                list_remove_item(self->resize_queue, prev_num);
+            }
+        }
+        if (!list_add_item(self->resize_queue, (tintptr)resize_queue_item))
+        {
+            free(resize_queue_item);
+            rv = 1;
+        }
+        else
+        {
+            // This call only has an effect if the wait_obj is not
+            // NULL_WAIT_OBJ
+            g_set_wait_obj(self->resize_ready);
+        }
+    }
+
+    return rv;
+}
+
 /******************************************************************************/
 static int
 dynamic_monitor_data(struct xrdp_process *id, int chan_id,
@@ -1447,7 +1510,7 @@ dynamic_monitor_data(struct xrdp_process *id, int chan_id,
     int msg_length;
     struct xrdp_wm *wm;
     int monitor_layout_size;
-    struct display_size_description *display_size_data;
+    struct display_size_description description;
 
     LOG_DEVEL(LOG_LEVEL_TRACE, "dynamic_monitor_data:");
     wm = id->wm;
@@ -1503,26 +1566,26 @@ dynamic_monitor_data(struct xrdp_process *id, int chan_id,
         return 1;
     }
 
-    display_size_data = (struct display_size_description *)
+    error = libxrdp_process_monitor_stream(s, &description, 1);
-                        g_malloc(sizeof(struct display_size_description), 1);
-    if (!display_size_data)
-    {
-        return 1;
-    }
-    error = libxrdp_process_monitor_stream(s, display_size_data, 1);
     if (error)
     {
         LOG(LOG_LEVEL_ERROR, "dynamic_monitor_data:"
             " libxrdp_process_monitor_stream"
             " failed with error %d.", error);
-        g_free(display_size_data);
         return error;
     }
-    list_add_item(wm->mm->resize_queue, (tintptr)display_size_data);
+    error = add_resize_request_to_queue(wm->mm, RQ_FROM_CLIENT, &description);
-    g_set_wait_obj(wm->mm->resize_ready);
+    if (error)
+    {
+        LOG(LOG_LEVEL_ERROR, "dynamic_monitor_data:"
+            " out of memory adding resize request to queue");
+        return error;
+    }
     LOG(LOG_LEVEL_DEBUG, "dynamic_monitor_data:"
-        " received width %d, received height %d.",
+        " received width %d, received height %d, queue %s",
-        display_size_data->session_width, display_size_data->session_height);
+        description.session_width,
+        description.session_height,
+        (wm->mm->resize_ready == NULL_WAIT_OBJ) ? "inactive" : "active");
     return 0;
 }
 
@@ -1770,6 +1833,19 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
     return 0;
 }
 
+/******************************************************************************/
+#ifdef USE_DEVEL_LOGGING
+static const char *
+resize_queue_source_to_str(enum resize_queue_source src)
+{
+    return
+        (src == RQ_IGNORE_MARKER) ? "RQ_IGNORE_MARKER" :
+        (src == RQ_FROM_SERVER) ? "RQ_FROM_SERVER" :
+        (src == RQ_FROM_CLIENT) ? "RQ_FROM_CLIENT" :
+        /* default */ "<unknown>";
+}
+#endif
+
 /******************************************************************************/
 static int
 dynamic_monitor_process_queue(struct xrdp_mm *self)
@@ -1795,30 +1871,33 @@ dynamic_monitor_process_queue(struct xrdp_mm *self)
             LOG_DEVEL(LOG_LEVEL_DEBUG, "Resize queue is empty.");
             return 0;
         }
-        LOG_DEVEL(LOG_LEVEL_DEBUG, "dynamic_monitor_process_queue: Queue is"
+        const struct resize_queue_item *queue_head =
-                  " not empty. Filling out description.");
+            (struct resize_queue_item *)list_get_item(self->resize_queue, 0);
-        const struct display_size_description *queue_head =
-            (struct display_size_description *)
-            list_get_item(self->resize_queue, 0);
 
-        const int invalid_dimensions = queue_head->session_width <= 0
+        LOG_DEVEL(LOG_LEVEL_INFO, "dynamic_monitor_process_queue: source of"
-                                       || queue_head->session_height <= 0;
+                  " resize queue head is %s",
+                  resize_queue_source_to_str(queue_head->src));
+        const struct display_size_description *queued_size =
+                &queue_head->description;
+
+        const int invalid_dimensions = queued_size->session_width <= 0
+                                       || queued_size->session_height <= 0;
 
         if (invalid_dimensions)
         {
             LOG(LOG_LEVEL_DEBUG,
                 "dynamic_monitor_process_queue: Not allowing"
                 " resize due to invalid dimensions (w: %d x h: %d)",
-                queue_head->session_width,
+                queued_size->session_width,
-                queue_head->session_height);
+                queued_size->session_height);
         }
 
         const struct display_size_description *current_size
                 = &wm->client_info->display_sizes;
 
-        const int already_this_size = queue_head->session_width
+        const int already_this_size = queued_size->session_width
                                       == current_size->session_width
-                                      && queue_head->session_height
+                                      && queued_size->session_height
                                       == current_size->session_height;
 
         if (already_this_size)
@@ -1826,8 +1905,8 @@ dynamic_monitor_process_queue(struct xrdp_mm *self)
             LOG(LOG_LEVEL_DEBUG,
                 "dynamic_monitor_process_queue: Not resizing."
                 " Already this size. (w: %d x h: %d)",
-                queue_head->session_width,
+                queued_size->session_width,
-                queue_head->session_height);
+                queued_size->session_height);
         }
 
         if (!invalid_dimensions && !already_this_size)
@@ -1836,8 +1915,7 @@ dynamic_monitor_process_queue(struct xrdp_mm *self)
                 sizeof(struct display_control_monitor_layout_data);
             self->resize_data = (struct display_control_monitor_layout_data *)
                                 g_malloc(LAYOUT_DATA_SIZE, 1);
-            g_memcpy(&(self->resize_data->description), queue_head,
+            self->resize_data->description = *queued_size;
-                     sizeof(struct display_size_description));
             const unsigned int time = g_get_elapsed_ms();
             self->resize_data->start_time = time;
             self->resize_data->last_state_update_timestamp = time;
@@ -1896,8 +1974,6 @@ dynamic_monitor_initialize(struct xrdp_mm *self)
     struct xrdp_drdynvc_procs d_procs;
     int flags;
     int error;
-    char buf[1024];
-    int pid;
 
     LOG_DEVEL(LOG_LEVEL_TRACE, "dynamic_monitor_initialize:");
 
@@ -1921,11 +1997,8 @@ dynamic_monitor_initialize(struct xrdp_mm *self)
     // Initialize xrdp_mm specific variables.
     self->resize_queue = list_create();
     self->resize_queue->auto_free = 1;
-    pid = g_getpid();
-    /* setup wait objects for signaling */
-    g_snprintf(buf, sizeof(buf), "xrdp_%8.8x_resize_ready", pid);
-    self->resize_ready = g_create_wait_obj(buf);
     self->resize_data = NULL;
+    self->resize_ready = NULL_WAIT_OBJ;
 
     return error;
 }
@@ -1934,7 +2007,12 @@ dynamic_monitor_initialize(struct xrdp_mm *self)
 int
 xrdp_mm_drdynvc_up(struct xrdp_mm *self)
 {
-    struct display_control_monitor_layout_data *ignore_marker;
+    struct display_size_description null_desc =
+    {
+        .monitorCount = 0,
+        .session_width = 0,
+        .session_height = 0
+    };
     const char *enable_dynamic_resize;
     int error = 0;
 
@@ -1963,10 +2041,12 @@ xrdp_mm_drdynvc_up(struct xrdp_mm *self)
             " Client likely does not support it.");
         return error;
     }
-    ignore_marker = (struct display_control_monitor_layout_data *)
+    error = add_resize_request_to_queue(self, RQ_IGNORE_MARKER, &null_desc);
-                    g_malloc(sizeof(struct display_control_monitor_layout_data),
+    if (error != 0)
-                             1);
+    {
-    list_add_item(self->resize_queue, (tintptr)ignore_marker);
+        LOG(LOG_LEVEL_ERROR, "%s: Out of memory", __func__);
+        error = 1;
+    }
     return error;
 }
 
@@ -3208,6 +3288,41 @@ xrdp_mm_connect(struct xrdp_mm *self)
     xrdp_mm_connect_sm(self);
 }
 
+/*****************************************************************************/
+/**
+ * Start resize queue processing
+ *
+ * The xrdp login screen does not currently support client-side resizes. We
+ * currently address this by not processing the resize queue until we are
+ * able to do so.
+ *
+ * We implement this by not creating the resize_ready wait object until
+ * we are able to process the queue. Calls made to an empty wait object
+ * are simply ignored.
+ *
+ * @param self MM module
+ */
+static void
+start_processing_resize_queue(struct xrdp_mm *self)
+{
+    if (self->resize_ready == NULL_WAIT_OBJ)
+    {
+        char buf[32];
+        int outstanding =
+            (self->resize_queue != NULL) ? self->resize_queue->count : 0;
+        int pid = g_getpid();
+        g_snprintf(buf, sizeof(buf), "xrdp_%8.8x_resize_ready", pid);
+        self->resize_ready = g_create_wait_obj(buf);
+        LOG(LOG_LEVEL_INFO,
+            "xrdp can now process resize requests (%d outstanding)",
+            outstanding);
+        if (outstanding > 0)
+        {
+            g_set_wait_obj(self->resize_ready);
+        }
+    }
+}
+
 /*****************************************************************************/
 static void
 xrdp_mm_connect_sm(struct xrdp_mm *self)
@@ -3384,6 +3499,12 @@ xrdp_mm_connect_sm(struct xrdp_mm *self)
                                 "Connecting to display server");
                 /* This is synchronous - no reply message expected */
                 status = xrdp_mm_display_server_connect(self);
+                if (status == 0)
+                {
+                    // This is as good a place as any to start processing
+                    // the resize_queue
+                    start_processing_resize_queue(self);
+                }
             }
             break;
 
@@ -3481,7 +3602,7 @@ xrdp_mm_get_wait_objs(struct xrdp_mm *self,
         read_objs[(*rcount)++] = self->encoder->xrdp_encoder_event_processed;
     }
 
-    if (self->resize_queue != 0)
+    if (self->resize_queue != 0 && self->resize_ready != NULL_WAIT_OBJ)
     {
         read_objs[(*rcount)++] = self->resize_ready;
     }
@@ -4676,7 +4797,7 @@ client_monitor_resize(struct xrdp_mod *mod, int width, int height,
 {
     int error = 0;
     struct xrdp_wm *wm;
-    struct display_size_description *display_size_data;
+    struct display_size_description description;
 
     LOG_DEVEL(LOG_LEVEL_TRACE, "client_monitor_resize:");
     wm = (struct xrdp_wm *)(mod->wm);
@@ -4699,25 +4820,23 @@ client_monitor_resize(struct xrdp_mod *mod, int width, int height,
         return 1;
     }
 
-    display_size_data = g_new0(struct display_size_description, 1);
-    if (display_size_data == NULL)
-    {
-        LOG(LOG_LEVEL_ERROR, "client_monitor_resize: Out of memory");
-        return 1;
-    }
     error = libxrdp_init_display_size_description(num_monitors,
             monitors,
-            display_size_data);
+            &description);
     if (error)
     {
         LOG(LOG_LEVEL_ERROR, "client_monitor_resize:"
             " libxrdp_init_display_size_description"
             " failed with error %d.", error);
-        free(display_size_data);
         return error;
     }
-    list_add_item(wm->mm->resize_queue, (tintptr)display_size_data);
+    error = add_resize_request_to_queue(wm->mm, RQ_FROM_SERVER, &description);
-    g_set_wait_obj(wm->mm->resize_ready);
+    if (error)
+    {
+        LOG(LOG_LEVEL_ERROR, "client_monitor_resize:"
+            " out of memory adding queue item");
+        return error;
+    }
 
     return 0;
 }

xrdp/xrdp_types.h

@@ -459,6 +459,8 @@ struct xrdp_mm
     /* Resize on-the-fly control */
     struct display_control_monitor_layout_data *resize_data;
     struct list *resize_queue;
+    /* wait obj for resize_queue
+     * Only allocated when the queue can be processed */
     tbus resize_ready;
     /* Last sync event if a module isn't loaded */
     int last_sync_saved;