Migrate universal variables to env_var_t structure. Encapsulate

universal variable storage into a class for better testability.

env.cpp

@@ -80,20 +80,6 @@ extern char **environ;
 */
 extern char **__environ;
 
-/**
-   A variable entry. Stores the value of a variable and whether it
-   should be exported. Obviously, it needs to be allocated large
-   enough to fit the value string.
-*/
-struct var_entry_t
-{
-    wcstring val; /**< The value of the variable */
-    bool exportv; /**< Whether the variable should be exported */
-
-    var_entry_t() : exportv(false) { }
-};
-
-typedef std::map<wcstring, var_entry_t> var_table_t;
 
 bool g_log_forks = false;
 bool g_use_posix_spawn = false; //will usually be set to true
@@ -824,7 +810,7 @@ int env_set(const wcstring &key, const wchar_t *val, int var_mode)
                 env_universal_barrier();
             }
 
-            if (env_universal_get(key))
+            if (! env_universal_get(key).missing())
             {
                 bool exportv;
                 if (var_mode & ENV_EXPORT)
@@ -1071,9 +1057,9 @@ env_var_t env_get_string(const wcstring &key)
             env_universal_barrier();
         }
 
-        const wchar_t *item = env_universal_get(key);
+        env_var_t item = env_universal_get(key);
 
-        if (!item || (wcscmp(item, ENV_NULL)==0))
+        if (item.missing() || (wcscmp(item.c_str(), ENV_NULL)==0))
         {
             return env_var_t::missing_var();
         }
@@ -1087,7 +1073,6 @@ env_var_t env_get_string(const wcstring &key)
 bool env_exist(const wchar_t *key, int mode)
 {
     env_node_t *env;
-    const wchar_t *item = NULL;
 
     CHECK(key, false);
 
@@ -1151,9 +1136,7 @@ bool env_exist(const wchar_t *key, int mode)
             env_universal_barrier();
         }
 
-        item = env_universal_get(key);
+        if (! env_universal_get(key).missing())
-
-        if (item != NULL)
         {
             if (mode & ENV_EXPORT)
             {
@@ -1419,9 +1402,9 @@ static void update_export_array_if_necessary(bool recalc)
         for (i=0; i<uni.size(); i++)
         {
             const wcstring &key = uni.at(i);
-            const wchar_t *val = env_universal_get(key);
+            const env_var_t val = env_universal_get(key);
 
-            if (wcscmp(val, ENV_NULL))
+            if (! val.missing() && wcscmp(val.c_str(), ENV_NULL))
             {
                 // Note that std::map::insert does NOT overwrite a value already in the map,
                 // which we depend on here

env.h

@@ -242,5 +242,18 @@ extern int g_fork_count;
 
 extern bool g_use_posix_spawn;
 
+/**
+ A variable entry. Stores the value of a variable and whether it
+ should be exported.
+ */
+struct var_entry_t
+{
+    wcstring val; /**< The value of the variable */
+    bool exportv; /**< Whether the variable should be exported */
+    
+    var_entry_t() : exportv(false) { }
+};
+
+typedef std::map<wcstring, var_entry_t> var_table_t;
 
 #endif

env_universal.cpp

@@ -38,6 +38,7 @@
 #include "wutil.h"
 #include "env_universal_common.h"
 #include "env_universal.h"
+#include "env.h"
 
 /**
    Maximum number of times to try to get a new fishd socket
@@ -345,10 +346,10 @@ int env_universal_read_all()
     }
 }
 
-const wchar_t *env_universal_get(const wcstring &name)
+env_var_t env_universal_get(const wcstring &name)
 {
     if (!s_env_univeral_inited)
-        return NULL;
+        return env_var_t::missing_var();
 
     return env_universal_common_get(name);
 }
@@ -462,14 +463,15 @@ int env_universal_remove(const wchar_t *name)
 
     CHECK(name, 1);
 
-    res = !env_universal_common_get(name);
+    wcstring name_str = name;
+    res = env_universal_common_get(name_str).missing();
     debug(3,
           L"env_universal_remove( \"%ls\" )",
           name);
 
     if (is_dead())
     {
-        env_universal_common_remove(wcstring(name));
+        env_universal_common_remove(name_str);
     }
     else
     {

env_universal.h

@@ -8,6 +8,7 @@
 #include <wchar.h>
 
 #include "env_universal_common.h"
+#include "env.h"
 
 /**
    Data about the universal variable server.
@@ -29,7 +30,7 @@ void env_universal_destroy();
 /**
    Get the value of a universal variable
 */
-const wchar_t *env_universal_get(const wcstring &name);
+env_var_t env_universal_get(const wcstring &name);
 
 /**
    Get the export flag of the variable with the specified

env_universal_common.cpp

@@ -86,28 +86,13 @@
 */
 #define ENV_UNIVERSAL_EOF   0x102
 
-/**
-   A variable entry. Stores the value of a variable and whether it
-   should be exported. Obviously, it needs to be allocated large
-   enough to fit the value string.
-*/
-typedef struct var_uni_entry
-{
-    bool exportv; /**< Whether the variable should be exported */
-    wcstring val; /**< The value of the variable */
-    var_uni_entry():exportv(false), val() { }
-}
-var_uni_entry_t;
-
-
 static void parse_message(wchar_t *msg,
                           connection_t *src);
 
 /**
    The table of all universal variables
 */
-typedef std::map<wcstring, var_uni_entry_t> env_var_table_t;
+static env_universal_t s_env_universal_var;
-env_var_table_t env_universal_var;
 
 /**
    Callback function, should be called on all events
@@ -269,7 +254,7 @@ void read_message(connection_t *src)
 */
 void env_universal_common_remove(const wcstring &name)
 {
-    env_universal_var.erase(name);
+    s_env_universal_var.remove(name);
 }
 
 /**
@@ -291,10 +276,8 @@ void env_universal_common_set(const wchar_t *key, const wchar_t *val, bool expor
 {
     CHECK(key,);
     CHECK(val,);
-
+    
-    var_uni_entry_t &entry = env_universal_var[key];
+    s_env_universal_var.set(key, val, exportv);
-    entry.exportv=exportv;
-    entry.val = val;
 
     if (callback)
     {
@@ -601,60 +584,26 @@ message_t *create_message(fish_message_type_t type,
 /**
   Put exported or unexported variables in a string list
 */
-void env_universal_common_get_names(wcstring_list_t &lst,
+void env_universal_common_get_names(wcstring_list_t &lst, bool show_exported, bool show_unexported)
-                                    bool show_exported,
-                                    bool show_unexported)
 {
-    env_var_table_t::const_iterator iter;
+    wcstring_list_t names = s_env_universal_var.get_names(show_exported, show_unexported);
-    for (iter = env_universal_var.begin(); iter != env_universal_var.end(); ++iter)
+    lst.insert(lst.end(), names.begin(), names.end());
-    {
-        const wcstring &key = iter->first;
-        const var_uni_entry_t &e = iter->second;
-        if ((e.exportv && show_exported) || (! e.exportv && show_unexported))
-        {
-            lst.push_back(key);
-        }
-    }
-
 }
 
 
-const wchar_t *env_universal_common_get(const wcstring &name)
+env_var_t env_universal_common_get(const wcstring &name)
 {
-    env_var_table_t::const_iterator result = env_universal_var.find(name);
+    return s_env_universal_var.get(name);
-    if (result != env_universal_var.end())
-    {
-        const var_uni_entry_t &e = result->second;
-        return const_cast<wchar_t*>(e.val.c_str());
-    }
-
-    return NULL;
 }
 
 bool env_universal_common_get_export(const wcstring &name)
 {
-    env_var_table_t::const_iterator result = env_universal_var.find(name);
+    return s_env_universal_var.get_export(name);
-    if (result != env_universal_var.end())
-    {
-        const var_uni_entry_t &e = result->second;
-        return e.exportv;
-    }
-    return false;
 }
 
 void enqueue_all(connection_t *c)
 {
-    env_var_table_t::const_iterator iter;
+    s_env_universal_var.enqueue_all(c);
-    for (iter = env_universal_var.begin(); iter != env_universal_var.end(); ++iter)
-    {
-        const wcstring &key = iter->first;
-        const var_uni_entry_t &entry = iter->second;
-
-        message_t *msg = create_message(entry.exportv ? SET_EXPORT : SET, key.c_str(), entry.val.c_str());
-        msg->count=1;
-        c->unsent.push(msg);
-    }
-
     try_send_all(c);
 }
 
@@ -679,3 +628,81 @@ void connection_destroy(connection_t *c)
         }
     }
 }
+
+env_universal_t::env_universal_t()
+{
+    VOMIT_ON_FAILURE(pthread_mutex_init(&lock, NULL));
+}
+
+env_universal_t::~env_universal_t()
+{
+    pthread_mutex_destroy(&lock);
+}
+
+env_var_t env_universal_t::get(const wcstring &name) const
+{
+    env_var_t result = env_var_t::missing_var();
+    var_table_t::const_iterator where = vars.find(name);
+    if (where != vars.end())
+    {
+        result = where->second.val;
+    }
+    return result;
+}
+
+bool env_universal_t::get_export(const wcstring &name) const
+{
+    bool result = false;
+    var_table_t::const_iterator where = vars.find(name);
+    if (where != vars.end())
+    {
+        result = where->second.exportv;
+    }
+    return result;
+}
+
+void env_universal_t::set(const wcstring &key, const wcstring &val, bool exportv)
+{
+    scoped_lock locker(lock);
+    var_entry_t *entry = &vars[key];
+    entry->val = val;
+    entry->exportv = exportv;
+}
+
+void env_universal_t::remove(const wcstring &key)
+{
+    scoped_lock locker(lock);
+    vars.erase(key);
+}
+
+wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexported) const
+{
+    wcstring_list_t result;
+    scoped_lock locker(lock);
+    var_table_t::const_iterator iter;
+    for (iter = vars.begin(); iter != vars.end(); ++iter)
+    {
+        const wcstring &key = iter->first;
+        const var_entry_t &e = iter->second;
+        if ((e.exportv && show_exported) || (! e.exportv && show_unexported))
+        {
+            result.push_back(key);
+        }
+    }
+    return result;
+}
+
+void env_universal_t::enqueue_all(connection_t *c) const
+{
+    scoped_lock locker(lock);
+    var_table_t::const_iterator iter;
+    for (iter = vars.begin(); iter != vars.end(); ++iter)
+    {
+        const wcstring &key = iter->first;
+        const var_entry_t &entry = iter->second;
+        
+        message_t *msg = create_message(entry.exportv ? SET_EXPORT : SET, key.c_str(), entry.val.c_str());
+        msg->count=1;
+        c->unsent.push(msg);
+    }
+}

env_universal_common.h

@@ -5,6 +5,7 @@
 #include <queue>
 #include <string>
 #include "util.h"
+#include "env.h"
 
 /**
    The set command
@@ -176,7 +177,7 @@ void env_universal_common_remove(const wcstring &key);
    This function operate agains the local copy of all universal
    variables, it does not communicate with any other process.
 */
-const wchar_t *env_universal_common_get(const wcstring &name);
+env_var_t env_universal_common_get(const wcstring &name);
 
 /**
    Get the export flag of the variable with the specified
@@ -199,4 +200,32 @@ void enqueue_all(connection_t *c);
 */
 void connection_destroy(connection_t *c);
 
+/** Class representing universal variables */
+class env_universal_t
+{
+    var_table_t vars;
+    mutable pthread_mutex_t lock;
+public:
+    env_universal_t();
+    ~env_universal_t();
+    
+    /* Get the value of the variable with the specified name */
+    env_var_t get(const wcstring &name) const;
+    
+    /* Returns whether the variable with the given name is exported, or false if it does not exist */
+    bool get_export(const wcstring &name) const;
+    
+    /* Sets a variable */
+    void set(const wcstring &key, const wcstring &val, bool exportv);
+    
+    /* Removes a variable */
+    void remove(const wcstring &name);
+    
+    /* Gets variable names */
+    wcstring_list_t get_names(bool show_exported, bool show_unexported) const;
+    
+    /* Writes variables to the connection */
+    void enqueue_all(connection_t *c) const;
+};
+
 #endif

fishd.cpp

@@ -720,8 +720,7 @@ static env_var_t fishd_env_get(const char *key)
     else
     {
         const wcstring wkey = str2wcstring(key);
-        const wchar_t *tmp = env_universal_common_get(wkey);
+        return env_universal_common_get(wkey);
-        return tmp ? env_var_t(tmp) : env_var_t::missing_var();
     }
 }