Updated session allocation policy for sesman

Made session allocation policies more readable and maintainable.

The 'C' policy which was confusing before has been replaced with the
'Separate' keyword. This is a public interface change, but is unlikely
to affect many users.

The logging in session_get_bydata() is substantially improved, making
it far easier to spot why sessions are getting matched or not matched.

4 files changed, 177 insertions, 85 deletions

diff --git a/sesman/config.c b/sesman/config.c
index 4eabdd46..23e7ff37 100644
--- a/sesman/config.c
+++ b/sesman/config.c
@@ -37,6 +37,81 @@
 #include "chansrv/chansrv_common.h"
 #include "scp.h"
 
+static const struct bitmask_char policy_bits[] =
+{
+    { SESMAN_CFG_SESS_POLICY_U, 'U'  },
+    { SESMAN_CFG_SESS_POLICY_B, 'B'  },
+    { SESMAN_CFG_SESS_POLICY_D, 'D'  },
+    { SESMAN_CFG_SESS_POLICY_I, 'I'  },
+    BITMASK_CHAR_END_OF_LIST
+};
+
+/***************************************************************************//**
+ * Parse a session allocation policy string
+ */
+static unsigned int
+parse_policy_string(const char *value)
+{
+    unsigned int rv;
+    char unrecognised[16];
+
+    if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_DFLT_S))
+    {
+        rv = SESMAN_CFG_SESS_POLICY_DEFAULT;
+    }
+    else if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_SEP_S))
+    {
+        rv = SESMAN_CFG_SESS_POLICY_SEPARATE;
+    }
+    else
+    {
+        unrecognised[0] = '\0';
+        rv = g_charstr_to_bitmask(value, policy_bits, unrecognised,
+                                  sizeof(unrecognised));
+        if (unrecognised[0] != '\0')
+        {
+            LOG(LOG_LEVEL_WARNING, "Character(s) '%s' in the session"
+                " allocation policy are not recognised", unrecognised);
+
+            if (g_strchr(unrecognised, 'C') != NULL ||
+                    g_strchr(unrecognised, 'c') != NULL)
+            {
+                /* Change from xrdp v0.9.x */
+                LOG(LOG_LEVEL_WARNING, "Character 'C' is no longer used"
+                    " in session allocation policies - use '%s'",
+                    SESMAN_CFG_SESS_POLICY_SEP_S);
+            }
+        }
+    }
+
+    return rv;
+}
+
+/******************************************************************************/
+int
+config_output_policy_string(unsigned int value,
+                            char *buff, unsigned int bufflen)
+{
+    int rv = 0;
+    if (bufflen > 0)
+    {
+        if (value & SESMAN_CFG_SESS_POLICY_DEFAULT)
+        {
+            rv = g_snprintf(buff, bufflen, "Default");
+        }
+        else if (value & SESMAN_CFG_SESS_POLICY_SEPARATE)
+        {
+            rv = g_snprintf(buff, bufflen, "Separate");
+        }
+        else
+        {
+            rv = g_bitmask_to_charstr(value, policy_bits, buff, bufflen, NULL);
+        }
+    }
+
+    return rv;
+}
+
 /***************************************************************************//**
  *
  * @brief Reads sesman [global] configuration section
@@ -295,7 +370,8 @@ config_read_sessions(int file, struct config_sessions *se, struct list *param_n,
                      struct list *param_v)
 {
     int i;
-    char *buf;
+    const char *buf;
+    const char *value;
 
     list_clear(param_v);
     list_clear(param_n);
@@ -306,70 +382,43 @@ config_read_sessions(int file, struct config_sessions *se, struct list *param_n,
     se->max_idle_time = 0;
     se->max_disc_time = 0;
     se->kill_disconnected = 0;
-    se->policy = SESMAN_CFG_SESS_POLICY_DFLT;
+    se->policy = SESMAN_CFG_SESS_POLICY_DEFAULT;
 
     file_read_section(file, SESMAN_CFG_SESSIONS, param_n, param_v);
 
     for (i = 0; i < param_n->count; i++)
     {
-        buf = (char *)list_get_item(param_n, i);
+        buf = (const char *)list_get_item(param_n, i);
+        value = (const char *)list_get_item(param_v, i);
 
         if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_X11DISPLAYOFFSET))
         {
-            se->x11_display_offset = g_atoi((char *)list_get_item(param_v, i));
+            se->x11_display_offset = g_atoi(value);
         }
 
-        if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_MAX))
+        else if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_MAX))
         {
-            se->max_sessions = g_atoi((char *)list_get_item(param_v, i));
+            se->max_sessions = g_atoi(value);
         }
 
-        if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_KILL_DISC))
+        else if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_KILL_DISC))
         {
-            se->kill_disconnected = g_text2bool((char *)list_get_item(param_v, i));
+            se->kill_disconnected = g_text2bool(value);
         }
 
-        if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_IDLE_LIMIT))
+        else if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_IDLE_LIMIT))
         {
-            se->max_idle_time = g_atoi((char *)list_get_item(param_v, i));
+            se->max_idle_time = g_atoi(value);
         }
 
-        if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_DISC_LIMIT))
+        else if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_DISC_LIMIT))
         {
-            se->max_disc_time = g_atoi((char *)list_get_item(param_v, i));
+            se->max_disc_time = g_atoi(value);
         }
 
-        if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_POLICY_S))
+        else if (0 == g_strcasecmp(buf, SESMAN_CFG_SESS_POLICY_S))
         {
-            char *value = (char *)list_get_item(param_v, i);
-            if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_DFLT_S))
-            {
-                se->policy = SESMAN_CFG_SESS_POLICY_DFLT;
-            }
-            else if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_UBD_S))
-            {
-                se->policy = SESMAN_CFG_SESS_POLICY_UBD;
-            }
-            else if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_UBI_S))
-            {
-                se->policy = SESMAN_CFG_SESS_POLICY_UBI;
-            }
-            else if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_UBC_S))
-            {
-                se->policy = SESMAN_CFG_SESS_POLICY_UBC;
-            }
-            else if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_UBDI_S))
-            {
-                se->policy = SESMAN_CFG_SESS_POLICY_UBDI;
-            }
-            else if (0 == g_strcasecmp(value, SESMAN_CFG_SESS_POLICY_UBDC_S))
-            {
-                se->policy = SESMAN_CFG_SESS_POLICY_UBDC;
-            }
-            else /* silently ignore typos */
-            {
-                se->policy = SESMAN_CFG_SESS_POLICY_DFLT;
-            }
+            se->policy = parse_policy_string(value);
         }
     }
 
@@ -570,6 +619,7 @@ config_dump(struct config_sesman *config)
     struct config_security *sc;
     se = &(config->sess);
     sc = &(config->sec);
+    char policy_s[64];
 
     /* Global sesman configuration */
     g_writeln("Filename:                     %s", config->sesman_ini);
@@ -583,13 +633,15 @@ config_dump(struct config_sesman *config)
               (config->auth_file_path ? config->auth_file_path : "disabled"));
 
     /* Session configuration */
+    config_output_policy_string(se->policy, policy_s, sizeof(policy_s));
+
     g_writeln("Session configuration:");
     g_writeln("    MaxSessions:              %d", se->max_sessions);
     g_writeln("    X11DisplayOffset:         %d", se->x11_display_offset);
     g_writeln("    KillDisconnected:         %d", se->kill_disconnected);
     g_writeln("    IdleTimeLimit:            %d", se->max_idle_time);
     g_writeln("    DisconnectedTimeLimit:    %d", se->max_disc_time);
-    g_writeln("    Policy:                   %d", se->policy);
+    g_writeln("    Policy:                   %s", policy_s);
 
     /* Security configuration */
     g_writeln("Security configuration:");
diff --git a/sesman/config.h b/sesman/config.h
index b73ff46c..87698c55 100644
--- a/sesman/config.h
+++ b/sesman/config.h
@@ -72,27 +72,18 @@
 
 #define SESMAN_CFG_SESS_POLICY_S "Policy"
 #define SESMAN_CFG_SESS_POLICY_DFLT_S "Default"
-#define SESMAN_CFG_SESS_POLICY_UBD_S "UBD"
-#define SESMAN_CFG_SESS_POLICY_UBI_S "UBI"
-#define SESMAN_CFG_SESS_POLICY_UBC_S "UBC"
-#define SESMAN_CFG_SESS_POLICY_UBDI_S "UBDI"
-#define SESMAN_CFG_SESS_POLICY_UBDC_S "UBDC"
+#define SESMAN_CFG_SESS_POLICY_SEP_S "Separate"
 
 enum SESMAN_CFG_SESS_POLICY_BITS
 {
-    SESMAN_CFG_SESS_POLICY_D = 0x01,
-    SESMAN_CFG_SESS_POLICY_I = 0x02,
-    SESMAN_CFG_SESS_POLICY_C = 0x04
-};
-
-enum SESMAN_CFG_SESS_POLICY
-{
-    SESMAN_CFG_SESS_POLICY_DFLT = 0,
-    SESMAN_CFG_SESS_POLICY_UBD = SESMAN_CFG_SESS_POLICY_D,
-    SESMAN_CFG_SESS_POLICY_UBI = SESMAN_CFG_SESS_POLICY_I,
-    SESMAN_CFG_SESS_POLICY_UBC = SESMAN_CFG_SESS_POLICY_C,
-    SESMAN_CFG_SESS_POLICY_UBDI = SESMAN_CFG_SESS_POLICY_D | SESMAN_CFG_SESS_POLICY_I,
-    SESMAN_CFG_SESS_POLICY_UBDC = SESMAN_CFG_SESS_POLICY_D | SESMAN_CFG_SESS_POLICY_C
+    /* If these two are set, they override everything else */
+    SESMAN_CFG_SESS_POLICY_DEFAULT = (1 << 0),
+    SESMAN_CFG_SESS_POLICY_SEPARATE = (1 << 1),
+    /* Configuration bits */
+    SESMAN_CFG_SESS_POLICY_U = (1 << 2),
+    SESMAN_CFG_SESS_POLICY_B = (1 << 3),
+    SESMAN_CFG_SESS_POLICY_D = (1 << 4),
+    SESMAN_CFG_SESS_POLICY_I = (1 << 5)
 };
 
 /**
@@ -180,7 +171,7 @@ struct config_sessions
      * @var policy
      * @brief session allocation policy
      */
-    enum SESMAN_CFG_SESS_POLICY policy;
+    unsigned int policy;
 };
 
 /**
@@ -304,4 +295,16 @@ config_dump(struct config_sesman *config);
 void
 config_free(struct config_sesman *cs);
 
+/**
+ * Converts a session allocation Policy value to a strin
+ * @param value - Session allocation policy value
+ * @param buff - Buffer for result
+ * @param bufflen - Length of buffer
+ * @return Length of string that would be required without a terminator
+ *         to write the whole output (like snprintf())
+ */
+int
+config_output_policy_string(unsigned int value,
+                            char *buff, unsigned int bufflen);
+
 #endif
diff --git a/sesman/sesman.ini.in b/sesman/sesman.ini.in
index 15ef50ba..278224a1 100644
--- a/sesman/sesman.ini.in
+++ b/sesman/sesman.ini.in
@@ -69,13 +69,20 @@ DisconnectedTimeLimit=0
 IdleTimeLimit=0
 
 ;; Policy - session allocation policy
-; Type: enum [ "Default" | "UBD" | "UBI" | "UBC" | "UBDI" | "UBDC" ]
-; "Default" session per <User,BitPerPixel>
-; "UBD" session per <User,BitPerPixel,DisplaySize>
-; "UBI" session per <User,BitPerPixel,IPAddr>
-; "UBC" session per <User,BitPerPixel,Connection>
-; "UBDI" session per <User,BitPerPixel,DisplaySize,IPAddr>
-; "UBDC" session per <User,BitPerPixel,DisplaySize,Connection>
+;
+;  Type: enum [ "Default" | "Separate" | Combination from {UBDI} ]
+;  "Default"    Currently same as "UB"
+;  "Separate"   All sessions are separate. Sessions can never be rejoined,
+;               and will need to be cleaned up manually, or automatically
+;               by setting other sesman options.
+;
+;   Combination options:-
+;      U        Sessions are separated per user
+;      B        Sessions are separated by bits-per-pixel
+;      D        Sessions are separated by initial display size
+;      I        Sessions are separated by IP address
+;
+;   The options U and B are always active, and cannot be de-selected.
 Policy=Default
 
 [Logging]
diff --git a/sesman/session.c b/sesman/session.c
index b59be93d..f95a5e23 100644
--- a/sesman/session.c
+++ b/sesman/session.c
@@ -88,17 +88,31 @@ dumpItemsToString(struct list *self, char *outstr, int len)
 struct session_item *
 session_get_bydata(const struct session_parameters *sp)
 {
+    char policy_str[64];
     struct session_chain *tmp;
-    enum SESMAN_CFG_SESS_POLICY policy = g_cfg->sess.policy;
+    int policy = g_cfg->sess.policy;
 
-    LOG(LOG_LEVEL_DEBUG,
-        "session_get_bydata: search policy %d U %s W %d H %d bpp %d T %d IP %s",
-        policy, sp->username, sp->width, sp->height, sp->bpp,
-        sp->type, sp->ip_addr);
+    if ((policy & SESMAN_CFG_SESS_POLICY_DEFAULT) != 0)
+    {
+        /* In the past (i.e. xrdp before v0.9.14), the default
+         * session policy varied by sp->type. If this is needed again
+         * in the future, here is the place to add it */
+        policy = SESMAN_CFG_SESS_POLICY_U | SESMAN_CFG_SESS_POLICY_B;
+    }
+
+    config_output_policy_string(policy, policy_str, sizeof(policy_str));
 
-    if ((policy & SESMAN_CFG_SESS_POLICY_C) != 0)
+    LOG(LOG_LEVEL_DEBUG,
+        "%s: search policy=%s type=%s U=%s B=%d D=(%dx%d) I=%s",
+        __func__,
+        policy_str, SCP_SESSION_TYPE_TO_STR(sp->type),
+        sp->username, sp->bpp, sp->width, sp->height,
+        sp->ip_addr);
+
+    /* 'Separate' policy never matches */
+    if (policy & SESMAN_CFG_SESS_POLICY_SEPARATE)
     {
-        /* Never matches */
+        LOG(LOG_LEVEL_DEBUG, "%s: No matches possible", __func__);
         return NULL;
     }
 
@@ -107,19 +121,33 @@ session_get_bydata(const struct session_parameters *sp)
         struct session_item *item = tmp->item;
 
         LOG(LOG_LEVEL_DEBUG,
-            "session_get_bydata: try %p U %s W %d H %d bpp %d T %d IP %s",
+            "%s: try %p type=%s U=%s B=%d D=(%dx%d) I=%s",
+            __func__,
             item,
+            SCP_SESSION_TYPE_TO_STR(item->type),
             item->name,
+            item->bpp,
             item->width, item->height,
-            item->bpp, item->type,
             item->start_ip_addr);
 
-        if (g_strncmp(sp->username, item->name, 255) != 0 ||
-                item->bpp != sp->bpp ||
-                item->type != sp->type)
+        if (item->type != sp->type)
+        {
+            LOG(LOG_LEVEL_DEBUG, "%s: Type doesn't match", __func__);
+            continue;
+        }
+
+        if ((policy & SESMAN_CFG_SESS_POLICY_U) &&
+                g_strncmp(sp->username, item->name, sizeof(item->name) - 1) != 0)
         {
             LOG(LOG_LEVEL_DEBUG,
-                "session_get_bydata: Basic parameters don't match");
+                "%s: Username doesn't match for 'U' policy", __func__);
+            continue;
+        }
+
+        if ((policy & SESMAN_CFG_SESS_POLICY_B) && item->bpp != sp->bpp)
+        {
+            LOG(LOG_LEVEL_DEBUG,
+                "%s: bpp doesn't match for 'B' policy", __func__);
             continue;
         }
 
@@ -127,7 +155,7 @@ session_get_bydata(const struct session_parameters *sp)
                 (item->width != sp->width || item->height != sp->height))
         {
             LOG(LOG_LEVEL_DEBUG,
-                "session_get_bydata: Dimensions don't match for 'D' policy");
+                "%s: Dimensions don't match for 'D' policy", __func__);
             continue;
         }
 
@@ -135,14 +163,16 @@ session_get_bydata(const struct session_parameters *sp)
                 g_strcmp(item->start_ip_addr, sp->ip_addr) != 0)
         {
             LOG(LOG_LEVEL_DEBUG,
-                "session_get_bydata: IPs don't match for 'I' policy");
+                "%s: IPs don't match for 'I' policy", __func__);
             continue;
         }
 
-        LOG(LOG_LEVEL_DEBUG, "session_get_bydata: Got match");
+        LOG(LOG_LEVEL_DEBUG,
+            "%s: Got match, display=%d", __func__, item->display);
         return item;
     }
 
+    LOG(LOG_LEVEL_DEBUG, "%s: No matches found", __func__);
     return 0;
 }