Merge branch 'Ionic-feature/reconnect-checks' into 3.6.x

Attributes GH PR #183: https://github.com/ArcticaProject/nx-libs/pull/183

Together with PR #143 this fixes ArcticaProject/nx-libs#132.

t merges an updated upstream into a topic branch.

4 files changed, 368 insertions, 50 deletions

diff --git a/nx-X11/programs/Xserver/hw/nxagent/Args.c b/nx-X11/programs/Xserver/hw/nxagent/Args.c
index 832595786..9e54c0073 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Args.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Args.c
@@ -1405,6 +1405,82 @@ static void nxagentParseOptions(char *name, char *value)
 
     return;
   }
+  else if (!strcmp(name, "tolerancechecks"))
+  {
+    if (strcmp(value, "strict") == 0)
+    {
+      nxagentChangeOption(ReconnectTolerance, ToleranceChecksStrict);
+    }
+    else if (strcmp(value, "safe") == 0)
+    {
+      nxagentChangeOption(ReconnectTolerance, ToleranceChecksSafe);
+    }
+    else if (strcmp(value, "risky") == 0)
+    {
+      nxagentChangeOption(ReconnectTolerance, ToleranceChecksRisky);
+    }
+    else if (strcmp(value, "none") == 0)
+    {
+      nxagentChangeOption(ReconnectTolerance, ToleranceChecksBypass);
+    }
+    else
+    {
+      /*
+       * Check for a matching integer. Or any integer, really.
+       */
+      long tolerance_parse = 0;
+
+      errno = 0;
+      tolerance_parse = strtol(value, NULL, 10);
+
+      if ((errno) && (0 == tolerance_parse))
+      {
+        fprintf(stderr, "nxagentParseOptions: Unable to convert value [%s] of option [%s]. "
+                        "Ignoring option.\n",
+                        validateString(value), validateString(name));
+
+        return;
+      }
+
+      if ((long) UINT_MAX < tolerance_parse)
+      {
+        tolerance_parse = UINT_MAX;
+
+        fprintf(stderr, "nxagentParseOptions: Warning: value [%s] of option [%s] "
+                        "out of range, clamped to [%u].\n",
+                        validateString(value), validateString(name), tolerance_parse);
+      }
+
+      if (0 > tolerance_parse)
+      {
+        tolerance_parse = 0;
+
+        fprintf(stderr, "nxagentParseOptions: Warning: value [%s] of option [%s] "
+                        "out of range, clamped to [%u].\n",
+                        validateString(value), validateString(name), tolerance_parse);
+      }
+
+      #ifdef TEST
+      switch (tolerance_parse) {
+        case ToleranceChecksStrict:
+        case ToleranceChecksSafe:
+        case ToleranceChecksRisky:
+        case ToleranceChecksBypass:
+                               break;
+        default:
+                               fprintf(stderr, "nxagentParseOptions: Warning: value [%s] of "
+                                               "option [%s] unknown, will be mapped to "
+                                               "\"Bypass\" [%u] value internally.\n",
+                                       validateString(value), validateString(name),
+                                       (unsigned int)ToleranceChecksBypass);
+      }
+      #endif
+
+      nxagentChangeOption(ReconnectTolerance, tolerance_parse);
+    }
+
+    return;
+  }
   else
   {
     #ifdef DEBUG
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Display.c b/nx-X11/programs/Xserver/hw/nxagent/Display.c
index 5943fbf77..5d258fd44 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Display.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Display.c
@@ -45,6 +45,7 @@ is" without express or implied warranty.
 #include <unistd.h>
 #include <time.h>
 #include <errno.h>
+#include <stdbool.h>
 
 #include <nx-X11/X.h>
 #include <nx-X11/Xproto.h>
@@ -178,7 +179,7 @@ static void nxagentInitDepths(void);
 static void nxagentInitPixmapFormats(void);
 
 static int nxagentCheckForDefaultDepthCompatibility(void);
-static int nxagentCheckForDepthsCompatibility(int flexibility);
+static int nxagentCheckForDepthsCompatibility(void);
 static int nxagentCheckForPixmapFormatsCompatibility(void);
 static int nxagentInitAndCheckVisuals(int flexibility);
 static int nxagentCheckForColormapsCompatibility(int flexibility);
@@ -1346,6 +1347,7 @@ FIXME: Use of nxagentParentWindow is strongly deprecated.
   nxagentInitDepths();
 
   nxagentInitPixmapFormats();
+  (void) nxagentCheckForPixmapFormatsCompatibility();
 
   /*
    * Create a pixmap for each depth matching the
@@ -1726,8 +1728,6 @@ XXX: Some X server doesn't list 1 among available depths...
     }
   }
   #endif
-
-  nxagentCheckForPixmapFormatsCompatibility();
 }
 
 void nxagentSetDefaultDrawables()
@@ -2175,10 +2175,40 @@ void nxagentDisconnectDisplay(void)
 
 static int nxagentCheckForDefaultDepthCompatibility()
 {
+  /*
+   * Depending on the (reconnect) tolerance checks value, this
+   * function checks stricter or looser:
+   *   - Strict means that the old and new default depth values
+   *     must match exactly.
+   *   - Safe or Risky means that the default depth values might differ,
+   *     but the new default depth value must be at least as
+   *     high as the former default depth value. This is
+   *     recommended, because it allows clients with a
+   *     higher default depth value to still connect, but
+   *     not lose functionality.
+   *   - Bypass or higher means that all of these checks are
+   *     essentially deactivated. This is probably a very
+   *     bad idea.
+   */
+
   int dDepth;
 
   dDepth = DefaultDepth(nxagentDisplay, DefaultScreen(nxagentDisplay));
 
+  const unsigned int tolerance = nxagentOption(ReconnectTolerance);
+
+  if (ToleranceChecksBypass <= tolerance)
+  {
+    #ifdef WARNING
+    fprintf(stderr, "nxagentCheckForDefaultDepthCompatibility: WARNING! Not proceeding with any checks, "
+                    "because tolerance [%u] higher than or equal [%u]. New default depth value "
+                    "is [%d], former default depth value is [%d].\n", tolerance,
+            ToleranceChecksBypass, dDepth, nxagentDefaultDepthRecBackup);
+    #endif
+
+    return 1;
+  }
+
   if (nxagentDefaultDepthRecBackup == dDepth)
   {
     #ifdef TEST
@@ -2188,142 +2218,326 @@ static int nxagentCheckForDefaultDepthCompatibility()
 
     return 1;
   }
+  else if ((ToleranceChecksSafe <= tolerance) && (nxagentDefaultDepthRecBackup < dDepth))
+  {
+    #ifdef WARNING
+    fprintf(stderr, "nxagentCheckForDefaultDepthCompatibility: WARNING! New default depth [%d] "
+                    "higher than the old default depth [%d] at tolerance [%u].\n", dDepth,
+            nxagentDefaultDepthRecBackup, tolerance);
+    #endif
+
+    return 1;
+  }
   else
   {
     #ifdef WARNING
     fprintf(stderr, "nxagentCheckForDefaultDepthCompatibility: WARNING! New default depth [%d] "
-                "doesn't match with old default depth [%d].\n", dDepth, nxagentDefaultDepthRecBackup);
+                "doesn't match with old default depth [%d] at tolerance [%u].\n", dDepth,
+            nxagentDefaultDepthRecBackup, tolerance);
     #endif
 
     return 0;
   }
 }
 
-static int nxagentCheckForDepthsCompatibility(int flexibility)
+static int nxagentCheckForDepthsCompatibility()
 {
-  int i, j;
-  int matched;
-  int compatible;
+  /*
+   * Depending on the (reconnect) tolerance checks value, this
+   * function checks stricter or looser:
+   *   - Strict means that the number of old and new depths must
+   *     match exactly and every old depth value must be
+   *     available in the new depth array.
+   *   - Safe means that the number of depths might diverge,
+   *     but all former depth must also be included in the
+   *     new depth array. This is recommended, because
+   *     it allows clients with more depths to still
+   *     connect, but not lose functionality.
+   *   - Risky means that the new depths array is allowed to be
+   *     smaller than the old depths array, but at least
+   *     one depth value must be included in both.
+   *     This is potentially unsafe.
+   *   - Bypass or higher means that all of these checks are
+   *     essentially deactivated. This is a very bad idea.
+   */
 
-  if (nxagentNumDepths != nxagentNumDepthsRecBackup)
+  const unsigned int tolerance = nxagentOption(ReconnectTolerance);
+
+  if (ToleranceChecksBypass <= tolerance)
   {
     #ifdef WARNING
-    fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! Number of new available depths [%d] "
-                "doesn't match with old depths [%d].\n", nxagentNumDepths,
-                    nxagentNumDepthsRecBackup);
+    fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! Not proceeding with any checks, "
+                    "because tolerance [%u] higher than or equal [%u]. Number of newly available depths "
+                    "is [%d], number of old depths is [%d].\n", tolerance, ToleranceChecksBypass,
+            nxagentNumDepths, nxagentNumDepthsRecBackup);
+    #endif
+
+    return 1;
+  }
+
+  if ((ToleranceChecksStrict == tolerance) && (nxagentNumDepths != nxagentNumDepthsRecBackup))
+  {
+    #ifdef WARNING
+    fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! No tolerance allowed and "
+                    "number of new available depths [%d] doesn't match with number of old "
+                    "depths [%d].\n", nxagentNumDepths,
+            nxagentNumDepthsRecBackup);
     #endif
 
     return 0;
   }
 
-  compatible = 1;
+  if ((ToleranceChecksSafe == tolerance) && (nxagentNumDepths < nxagentNumDepthsRecBackup))
+  {
+    #ifdef WARNING
+    fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! Tolerance [%u] not "
+                    "high enough and number of new available depths [%d] "
+                    "lower than number of old depths [%d].\n", tolerance,
+            nxagentNumDepths, nxagentNumDepthsRecBackup);
+    #endif
+
+    return 0;
+  }
+
+  /*
+   * By now the tolerance is either:
+   *   - Strict and both depth numbers match
+   *   - Safe and:
+   *     o the number of old and new depths matches exactly, or
+   *     o the number of old depths is lower than the number
+   *       of new depths
+   *   - Risky
+   */
+
+  bool compatible = true;
+  bool one_match = false;
+  bool matched = false;
+  int total_matches = 0;
 
-  for (i = 0; i < nxagentNumDepths; i++)
+  /*
+   * FIXME: within this loop, we try to match all "new" depths
+   *        against the "old" depths. Depending upon the flexibility
+   *        value, either all "new" depths must have a corresponding
+   *        counterpart in the "old" array, or at least one value
+   *        must be included in both.
+   *        Is this safe enough though?
+   *        Shouldn't we better try to match entries in the "old"
+   *        depths array against the "new" depths array, such that
+   *        we know that all "old" values are covered by "new"
+   *        values? Or is it more important that "new" values are
+   *        covered by "old" ones, with potentially more "old"
+   *        values lingering around that cannot be displayed by the
+   *        connected client?
+   *
+   * This section probably needs a revisit at some point in time.
+   */
+  for (int i = 0; i < nxagentNumDepths; ++i)
   {
-    matched = 0;
+    matched = false;
 
-    for (j = 0; j < nxagentNumDepthsRecBackup; j++)
+    for (int j = 0; j < nxagentNumDepthsRecBackup; ++j)
     {
       if (nxagentDepths[i] == nxagentDepthsRecBackup[j])
       {
-        matched = 1;
+        matched = true;
+        one_match = true;
+        ++total_matches;
 
         break;
       }
     }
 
-    if (matched == 0)
+    if ((ToleranceChecksRisky > tolerance) && (!matched))
     {
       #ifdef WARNING
-      fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! Failed to match available depth [%d].\n",
-                  nxagentDepths[i]);
+      fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! Tolerance [%u] too low and "
+                      "failed to match available depth [%d].\n", tolerance, nxagentDepths[i]);
       #endif
 
-      compatible = 0;
+      compatible = false;
 
       break;
     }
   }
 
+  /*
+   * At Risky tolerance, only one match is necessary to be "compatible".
+   */
+  if (ToleranceChecksRisky == tolerance)
+  {
+    compatible = one_match;
+  }
+
+  int ret = (!(!compatible));
 
-  if (compatible == 1)
+  if (compatible)
   {
     #ifdef TEST
     fprintf(stderr, "nxagentCheckForDepthsCompatibility: Internal depths match with "
-                "remote depths.\n");
+                "remote depths at tolerance [%u].\n", tolerance);
     #endif
+
+    if (total_matches != nxagentNumDepths)
+    {
+      #ifdef WARNING
+      fprintf(stderr, "nxagentCheckForDepthsCompatibility: only some [%d] of the new depths [%d] "
+                  "match with old depths [%d] at tolerance [%u].\n", total_matches, nxagentNumDepths,
+              nxagentNumDepthsRecBackup, tolerance);
+      #endif
+    }
   }
   else
   {
     #ifdef WARNING
-    fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! New available depths don't match with "
-                "old depths.\n");
+    fprintf(stderr, "nxagentCheckForDepthsCompatibility: WARNING! New available depths [%d] don't match "
+                    "with old depths [%d] at tolerance [%u]. Only [%d] depth values matched.\n",
+            nxagentNumDepths, nxagentNumDepthsRecBackup, tolerance, total_matches);
     #endif
   }
 
-  return compatible;
+  return (ret);
 }
 
 static int nxagentCheckForPixmapFormatsCompatibility()
 {
-  int i, j;
-  int matched;
-  int compatible;
+  /*
+   * Depending on the (reconnect) tolerance checks value, this
+   * function checks stricter or looser:
+   *   - Strict means that the number of internal and external
+   *     pixmap formats must match exactly and every
+   *     internal pixmap format must be available in the
+   *     external pixmap format array.
+   *   - Safe means that the number of pixmap formats might
+   *     diverge, but all internal pixmap formats must
+   *     also be included in the external pixmap formats
+   *     array. This is recommended, because it allows
+   *     clients with more pixmap formats to still connect,
+   *     but not lose functionality.
+   *   - Risky means that the internal pixmap formats array is
+   *     allowed to be smaller than the external pixmap
+   *     formats array, but at least one pixmap format must
+   *     be included in both. This is potentially unsafe.
+   *   - Bypass or higher means that all of these checks are
+   *     essentially deactivated. This is a very bad idea.
+   */
 
-  compatible = 1;
+  const unsigned int tolerance = nxagentOption(ReconnectTolerance);
+
+  if (ToleranceChecksBypass <= tolerance)
+  {
+    #ifdef WARNING
+    fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: WARNING! Not proceeding with any checks, "
+                    "because tolerance [%u] higher than or equal [%u]. Number of internally available "
+                    "pixmap formats is [%d], number of externally available pixmap formats is [%d].\n",
+            tolerance, ToleranceChecksBypass, nxagentNumPixmapFormats, nxagentRemoteNumPixmapFormats);
+    #endif
 
-  if (nxagentNumPixmapFormats != nxagentRemoteNumPixmapFormats)
+    return 1;
+  }
+
+  if ((ToleranceChecksStrict == tolerance) && (nxagentNumPixmapFormats != nxagentRemoteNumPixmapFormats))
   {
     #ifdef DEBUG
-    fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: WARNING! Number of internal pixmap formats [%d] "
-                "doesn't match with remote formats [%d].\n", nxagentNumPixmapFormats,
-                    nxagentRemoteNumPixmapFormats);
+    fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: WARNING! No tolerance allowed and number "
+                    "of internal pixmap formats [%d] doesn't match with number of remote formats [%d].\n",
+            nxagentNumPixmapFormats, nxagentRemoteNumPixmapFormats);
     #endif
+
+    return 0;
   }
 
-  for (i = 0; i < nxagentNumPixmapFormats; i++)
+  if ((ToleranceChecksSafe == tolerance) && (nxagentNumPixmapFormats > nxagentRemoteNumPixmapFormats))
   {
-    matched = 0;
+    #ifdef DEBUG
+    fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: WARNING! Tolerance [%u] too low "
+                    "and number of internal pixmap formats [%d] higher than number of external formats [%d].\n",
+            tolerance, nxagentNumPixmapFormats, nxagentRemoteNumPixmapFormats);
+    #endif
+
+    return 0;
+  }
+
+  /*
+   * By now the tolerance is either:
+   *   - Strict
+   *   - Safe and:
+   *     o the number of internal and external pixmap formats
+   *       matches exactly, or
+   *     o the number of external pixmap formats is higher than
+   *       the number of internal pixmap formats,
+   *   - Risky
+   */
+
+  bool compatible = true;
+  bool one_match = false;
+  bool matched = false;
+  int total_matches = 0;
+
+  for (int i = 0; i < nxagentNumPixmapFormats; ++i)
+  {
+    matched = false;
 
-    for (j = 0; j < nxagentRemoteNumPixmapFormats; j++)
+    for (int j = 0; j < nxagentRemoteNumPixmapFormats; ++j)
     {
       if (nxagentPixmapFormats[i].depth == nxagentRemotePixmapFormats[j].depth &&
           nxagentPixmapFormats[i].bits_per_pixel == nxagentRemotePixmapFormats[j].bits_per_pixel &&
           nxagentPixmapFormats[i].scanline_pad == nxagentRemotePixmapFormats[j].scanline_pad)
       {
-        matched = 1;
+        matched = true;
+        one_match = true;
+        ++total_matches;
 
         break;
       }
     }
 
-    if (matched == 0)
+    if ((ToleranceChecksRisky > tolerance) && (!matched))
     {
       #ifdef WARNING
-      fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: WARNING! Failed to match internal "
-                  "pixmap format depth [%d] bpp [%d] pad [%d].\n", nxagentPixmapFormats[i].depth,
-                      nxagentPixmapFormats[i].bits_per_pixel, nxagentPixmapFormats[i].scanline_pad);
+      fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: WARNING! Tolerance [%u] too low "
+                      "and failed to match internal pixmap format (depth [%d] bpp [%d] pad [%d]).\n",
+              tolerance, nxagentPixmapFormats[i].depth, nxagentPixmapFormats[i].bits_per_pixel,
+              nxagentPixmapFormats[i].scanline_pad);
       #endif
 
-      compatible = 0;
+      compatible = false;
     }
   }
 
-  #ifdef TEST
+  int ret = !(!(compatible));
 
-  if (compatible == 1)
+  if (compatible)
   {
+    #ifdef TEST
     fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: Internal pixmap formats match with "
-                "remote pixmap formats.\n");
-  }
+                    "remote pixmap formats at tolerance [%u].\n", tolerance);
+    #endif
 
-  #endif
+    if (total_matches != nxagentNumPixmapFormats)
+    {
+      #ifdef WARNING
+      fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: Only some [%d] of the internal "
+                      "pixmap formats [%d] match with external pixmap formats [%d] at tolerance [%u].\n",
+              total_matches, nxagentNumPixmapFormats, nxagentRemoteNumPixmapFormats, tolerance);
+      #endif
+    }
+  }
+  else
+  {
+    #ifdef WARNING
+    fprintf(stderr, "nxagentCheckForPixmapFormatsCompatibility: WARNING! Internally available "
+                    "pixmap formats [%d] don't match with external pixmap formats [%d] "
+                    "at tolerance [%u]. Only [%d] depth values matched.\n",
+            nxagentNumPixmapFormats, nxagentRemoteNumPixmapFormats, tolerance, total_matches);
+    #endif
+  }
 
-  return compatible;
+  return (ret);
 }
 
 static int nxagentInitAndCheckVisuals(int flexibility)
 {
+  /* FIXME: does this also need work? */
   XVisualInfo viTemplate;
   XVisualInfo *viList;
   XVisualInfo *newVisuals;
@@ -2433,6 +2647,7 @@ FIXME: Should the visual be ignored in this case?
 
 static int nxagentCheckForColormapsCompatibility(int flexibility)
 {
+  /* FIXME: does this also need work? */
   if (nxagentNumDefaultColormaps == nxagentNumDefaultColormapsRecBackup)
   {
     #ifdef TEST
@@ -2592,7 +2807,7 @@ Bool nxagentReconnectDisplay(void *p0)
 
   reconnectDisplayState = GOT_DEPTH_LIST;
 
-  if (nxagentCheckForDepthsCompatibility(flexibility) == 0)
+  if (nxagentCheckForDepthsCompatibility() == 0)
   {
     nxagentSetReconnectError(FAILED_RESUME_DEPTHS_ALERT,
                                  "Couldn't restore all the required depths.");
@@ -2626,6 +2841,14 @@ Bool nxagentReconnectDisplay(void *p0)
 
   nxagentInitPixmapFormats();
 
+  if (nxagentCheckForPixmapFormatsCompatibility() == 0)
+  {
+    nxagentSetReconnectError(FAILED_RESUME_PIXMAPS_ALERT,
+                                 "Couldn't restore all the required pixmap formats.");
+
+    return False;
+  }
+
   reconnectDisplayState = GOT_PIXMAP_FORMAT_LIST;
 
   /*
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Options.c b/nx-X11/programs/Xserver/hw/nxagent/Options.c
index 0a894f9a4..978f3ab95 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Options.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Options.c
@@ -166,6 +166,8 @@ void nxagentInitOptions()
   nxagentOptions.Xinerama = 1;
 
   nxagentOptions.SleepTime = DEFAULT_SLEEP_TIME;
+
+  nxagentOptions.ReconnectTolerance = DEFAULT_TOLERANCE;
 }
 
 /*
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Options.h b/nx-X11/programs/Xserver/hw/nxagent/Options.h
index df6f8b65b..20616a90b 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Options.h
+++ b/nx-X11/programs/Xserver/hw/nxagent/Options.h
@@ -67,6 +67,17 @@ typedef enum _ClientOsType
 
 } ClientOsType;
 
+typedef enum _ToleranceChecksMode
+{
+  ToleranceChecksStrict = 0,
+  ToleranceChecksSafe = 1,
+  ToleranceChecksRisky = 2,
+  ToleranceChecksBypass = 3
+} ToleranceChecksMode;
+
+
+#define DEFAULT_TOLERANCE ToleranceChecksStrict
+
 /*
  * Set of options affecting agent operations.
  */
@@ -414,6 +425,12 @@ typedef struct _AgentOptions
 
   unsigned int SleepTime;
 
+  /*
+   * Tolerance - tightens or loosens reconnect checks.
+   */
+
+  ToleranceChecksMode ReconnectTolerance;
+
 } AgentOptionsRec;
 
 typedef AgentOptionsRec *AgentOptionsPtr;