Bug fix config-group data structure

As config group is reallocated when full, do not store the pointer to the
parent group. Instead use the id of the group which is invariant across
reallocs. Similarly in connection array store the id of the group
instead of a pointer.

Also

- Do not call ActivateConfigGroups() -- when connections are active:
  in this case we want preserve config data structures during rescan.

Signed-off-by: Selva Nair <selva.nair@gmail.com>

fixup

openvpn_config.c

@@ -141,15 +141,17 @@ AddConfigFileToList(int config, const TCHAR *filename, const TCHAR *config_dir)
 
 /*
  * Create a new group with the given name as a child of the
- * specified parent group and return pointer to the new group.
+ * specified parent group id and returns the id of the new group.
  * If FLAG_ADD_CONFIG_GROUPS is not enabled, returns the
  * parent itself.
  */
-static config_group_t *
+static int
-NewConfigGroup(const wchar_t *name, config_group_t *parent, int flags)
+NewConfigGroup(const wchar_t *name, int parent, int flags)
 {
     if (!(flags & FLAG_ADD_CONFIG_GROUPS))
+    {
         return parent;
+    }
 
     if (!o.groups || o.num_groups == o.max_groups)
     {
@@ -171,7 +173,7 @@ NewConfigGroup(const wchar_t *name, config_group_t *parent, int flags)
     cg->parent = parent;
     cg->active = false; /* activated later if not empty */
 
-    return cg;
+    return cg->id;
 }
 
 /*
@@ -187,19 +189,24 @@ ActivateConfigGroups(void)
     /* the root group is always active */
     o.groups[0].active = true;
 
+    /* children is a counter re-used for activation, menu indexing etc. -- reset before use */
+    for (int i = 0; i < o.num_groups; i++)
+        o.groups[i].children = 0;
+
     /* count children of each group -- this includes groups
      * and configs which have it as parent
      */
     for (int i = 0; i < o.num_configs; i++)
     {
-        o.conn[i].group->children++;
+        CONFIG_GROUP(&o.conn[i])->children++;
     }
 
     for (int i = 1; i < o.num_groups; i++)
     {
         config_group_t *this = &o.groups[i];
-        if (this->parent) /* should be true as i = 0 is omitted */
+        config_group_t *parent = PARENT_GROUP(this);
-            this->parent->children++;
+        if (parent) /* should be true as i = 0 is omitted */
+            parent->children++;
 
         /* unless activated below the group stays inactive */
         this->active = false;
@@ -215,10 +222,10 @@ ActivateConfigGroups(void)
      */
     for (int i = 0; i < o.num_configs; i++)
     {
-        config_group_t *cg = o.conn[i].group;
+        config_group_t *cg = CONFIG_GROUP(&o.conn[i]);
 
         /* if not root and has only this config as child -- squash it */
-        if (cg->parent && cg->children == 1
+        if (PARENT_GROUP(cg) && cg->children == 1
             && !wcscmp(cg->name, o.conn[i].config_name))
         {
             cg->children--;
@@ -229,12 +236,12 @@ ActivateConfigGroups(void)
     /* activate all groups that connect a config to the root */
     for (int i = 0; i < o.num_configs; i++)
     {
-        config_group_t *cg = o.conn[i].group;
+        config_group_t *cg = CONFIG_GROUP(&o.conn[i]);
 
         while (cg)
         {
             cg->active = true;
-            cg = cg->parent;
+            cg = PARENT_GROUP(cg);
         }
     }
 }
@@ -245,12 +252,12 @@ ActivateConfigGroups(void)
  *        flags      -- enable warnings, use directory based
  *                      grouping of configs etc.
  * Currently configs in a directory are grouped together and group is
- * a pointer to the current parent group in the global group array |o.groups|
+ * the id of the current group in the global group array |o.groups|
  * This may be recursively called until depth becomes 1 and each time
  * the group is changed to that of the directory being recursed into.
  */
 static void
-BuildFileList0(const TCHAR *config_dir, int recurse_depth, config_group_t *group, int flags)
+BuildFileList0(const TCHAR *config_dir, int recurse_depth, int group, int flags)
 {
     WIN32_FIND_DATA find_obj;
     HANDLE find_handle;
@@ -316,7 +323,7 @@ BuildFileList0(const TCHAR *config_dir, int recurse_depth, config_group_t *group
             {
                 /* recurse into subdirectory */
                 _sntprintf_0(subdir_name, _T("%s\\%s"), config_dir, find_obj.cFileName);
-                config_group_t *sub_group = NewConfigGroup(find_obj.cFileName, group, flags);
+                int sub_group = NewConfigGroup(find_obj.cFileName, group, flags);
 
                 BuildFileList0(subdir_name, recurse_depth - 1, sub_group, flags);
             }
@@ -332,7 +339,7 @@ BuildFileList()
     static bool issue_warnings = true;
     int recurse_depth = 4; /* read config_dir and 3 levels of sub-directories */
     int flags = 0;
-    config_group_t *root = NULL;
+    int root = 0;
 
     if (o.silent_connection)
         issue_warnings = false;
@@ -342,15 +349,15 @@ BuildFileList()
      * to make a new list. Else we keep all current configs and
      * rescan to add any new one's found
      */
-    if (CountConnState(disconnected) == o.num_configs)
+    if (!o.num_groups || CountConnState(disconnected) == o.num_configs)
     {
         o.num_configs = 0;
         o.num_groups = 0;
         flags |= FLAG_ADD_CONFIG_GROUPS;
-        root = NewConfigGroup(L"User Profiles", NULL, flags);
+        root = NewConfigGroup(L"ROOT", -1, flags); /* -1 indicates no parent */
     }
     else
-        root = &o.groups[0];
+        root = 0;
 
     if (issue_warnings)
     {
@@ -375,7 +382,11 @@ BuildFileList()
         o.num_configs = MAX_CONFIGS; /* menus don't work with more -- ignore the rest */
     }
 
-    ActivateConfigGroups();
+    /* if adding groups, activate non-empty ones */
+    if (flags &FLAG_ADD_CONFIG_GROUPS)
+    {
+        ActivateConfigGroups();
+    }
 
     issue_warnings = false;
 }

options.h

@@ -103,15 +103,19 @@ typedef struct {
  * which is enough for our purposes.
  */
 typedef struct config_group {
-    int id;                      /* A unique id for the group */
+    int id;                      /* A unique id for the group >= 0*/
     wchar_t name[40];            /* Name of the group -- possibly truncated */
-    struct config_group *parent; /* Pointer to parent group */
+    int parent;                  /* Id of parent group. -1 implies no parent */
     BOOL active;                 /* Displayed in the menu if true -- used to prune empty groups */
     int children;                /* Number of children groups and configs */
     int pos;                     /* Index within the parent group -- used for rendering */
     HMENU menu;                  /* Handle to menu entry for this group */
 } config_group_t;
 
+/* short hand for pointer to the group a config belongs to */
+#define CONFIG_GROUP(c) (&o.groups[(c)->group])
+#define PARENT_GROUP(cg) ((cg)->parent < 0 ? NULL : &o.groups[(cg)->parent])
+
 /* Connections parameters */
 struct connection {
     TCHAR config_file[MAX_PATH];    /* Name of the config file */
@@ -126,7 +130,7 @@ struct connection {
     int failed_auth_attempts;       /* # of failed user-auth attempts */
     time_t connected_since;         /* Time when the connection was established */
     proxy_t proxy_type;             /* Set during querying proxy credentials */
-    config_group_t *group;          /* Pointer to the group this config belongs to */
+    int group;                      /* ID of the group this config belongs to */
     int pos;                        /* Index of the config within its group */
 
     struct {

tray.c

@@ -48,26 +48,24 @@ HMENU hMenuService;
 NOTIFYICONDATA ni;
 extern options_t o;
 
-#define USE_NESTED_CONFIG_MENU ((o.config_menu_view == CONFIG_VIEW_AUTO && o.num_configs > 50)   \
+#define USE_NESTED_CONFIG_MENU ((o.config_menu_view == CONFIG_VIEW_AUTO && o.num_configs > 25)   \
                                  || (o.config_menu_view == CONFIG_VIEW_NESTED))
 
 /* Create popup menus */
 void
 CreatePopupMenus()
 {
-    int i;
-
     /* We use groups[0].menu as the root menu, so,
      * even if num_configs = 0, we want num_groups > 0.
      * This is guaranteed as the root node is always defined.
      */
     assert(o.num_groups > 0);
 
-    for (i = 0; i < o.num_configs; i++)
+    for (int i = 0; i < o.num_configs; i++)
     {
         hMenuConn[i] = CreatePopupMenu();
     }
-    for (i = 0; i < o.num_groups; i++)
+    for (int i = 0; i < o.num_groups; i++)
     {
         if (!o.groups[i].active)
             continue;
@@ -113,15 +111,14 @@ CreatePopupMenus()
         SetMenuStatusById(0,  o.conn[0].state);
     }
     else {
-        int i;
         /* construct the submenu tree first */
         if (USE_NESTED_CONFIG_MENU)
         {
             /* i = 0 is the root menu and has no parent */
-            for (i = 1; i < o.num_groups; i++)
+            for (int i = 1; i < o.num_groups; i++)
             {
                 config_group_t *this = &o.groups[i];
-                config_group_t *parent = this->parent;
+                config_group_t *parent = PARENT_GROUP(this);
 
                 if (!this->active || !parent)
                     continue;
@@ -134,16 +131,16 @@ CreatePopupMenus()
         }
 
         /* add config file (connection) entries */
-        for (i = 0; i < o.num_configs; i++)
+        for (int i = 0; i < o.num_configs; i++)
         {
             connection_t *c = &o.conn[i];
             config_group_t *parent = &o.groups[0]; /* by default config is added to the root */
 
             if (USE_NESTED_CONFIG_MENU)
             {
-                if (c->group) /* should be always true, but in case... */
+                parent = CONFIG_GROUP(c);
-                     parent = c->group;
             }
+            assert(parent);
 
             /* Add config to the current sub menu */
             AppendMenu(parent->menu, MF_POPUP, (UINT_PTR) hMenuConn[i], c->config_name);
@@ -169,7 +166,7 @@ CreatePopupMenus()
 
 
         /* Create popup menus for every connection */
-        for (i=0; i < o.num_configs; i++) {
+        for (int i = 0; i < o.num_configs; i++) {
             if (o.service_only == 0) {
                 AppendMenu(hMenuConn[i], MF_STRING, IDM_CONNECTMENU + i, LoadLocalizedString(IDS_MENU_CONNECT));
                 AppendMenu(hMenuConn[i], MF_STRING, IDM_DISCONNECTMENU + i, LoadLocalizedString(IDS_MENU_DISCONNECT));
@@ -454,8 +451,8 @@ SetMenuStatusById(int i, conn_state_t state)
         config_group_t *parent = &o.groups[0];
         int pos = c->pos;
 
-        if (USE_NESTED_CONFIG_MENU && c->group)
+        if (USE_NESTED_CONFIG_MENU && CONFIG_GROUP(c))
-            parent = c->group;
+            parent = CONFIG_GROUP(c);
 
         CheckMenuItem(parent->menu, pos, MF_BYPOSITION | (checked ? MF_CHECKED : MF_UNCHECKED));
 
@@ -464,10 +461,10 @@ SetMenuStatusById(int i, conn_state_t state)
 
         if (checked) /* also check all parent groups */
         {
-            while (parent->parent)
+            while (PARENT_GROUP(parent))
             {
                 pos = parent->pos;
-                parent = parent->parent;
+                parent = PARENT_GROUP(parent);
                 CheckMenuItem(parent->menu, pos, MF_BYPOSITION | MF_CHECKED);
             }
         }