Use a global R/W lock for configuration

Previously, we had one configuration mutex per module.
With a global read/write lock, resetting, loading, saving and
auto-saving the configuration becomes atomic (and use only one lock &
unlock pair). Also, multiple threads can now read the configuration
item of the same module at the same time.

Note that, as the earlier configuration mutex, only configuration item
values are protected. The list of items and their meta-data cannot
change while VLC runs (they're hard-coded in the plugin descriptors).

diff --git a/src/config/configuration.h b/src/config/configuration.h
index bd2b123c1e6be15db3579875201db60109cd16f3..a419552e84335c9034008cb7f55b832490a08d09 100644 (file)
--- a/src/config/configuration.h
+++ b/src/config/configuration.h
@@ -54,6 +54,8 @@ static inline int IsConfigFloatType (int type)
 uint_fast32_t ConfigStringToKey( const char * );
 char *ConfigKeyToString( uint_fast32_t );
 
+extern vlc_rwlock_t config_lock;
+
 /* The configuration file */
 #define CONFIG_FILE                     "vlcrc"
 

diff --git a/src/config/core.c b/src/config/core.c
index 97fd1e106b55d1dd40eb3dc0e4fec4eb68934e2a..d6a3f2ad78e4c82ad9829b91fdcb1990b7757227 100644 (file)
--- a/src/config/core.c
+++ b/src/config/core.c
@@ -35,6 +35,8 @@
 #include "configuration.h"
 #include "modules/modules.h"
 
+vlc_rwlock_t config_lock;
+
 static inline char *strdupnull (const char *src)
 {
     return src ? strdup (src) : NULL;
@@ -220,9 +222,9 @@ char * __config_GetPsz( vlc_object_t *p_this, const char *psz_name )
     }
 
     /* return a copy of the string */
-    vlc_mutex_lock( p_config->p_lock );
+    vlc_rwlock_rdlock (&config_lock);
     char *psz_value = strdupnull (p_config->value.psz);
-    vlc_mutex_unlock( p_config->p_lock );
+    vlc_rwlock_unlock (&config_lock);
 
     return psz_value;
 }
@@ -256,7 +258,7 @@ void __config_PutPsz( vlc_object_t *p_this,
         return;
     }
 
-    vlc_mutex_lock( p_config->p_lock );
+    vlc_rwlock_wrlock (&config_lock);
 
     /* backup old value */
     oldval.psz_string = (char *)p_config->value.psz;
@@ -270,7 +272,7 @@ void __config_PutPsz( vlc_object_t *p_this,
 
     val.psz_string = (char *)p_config->value.psz;
 
-    vlc_mutex_unlock( p_config->p_lock );
+    vlc_rwlock_unlock (&config_lock);
 
     if( p_config->pf_callback )
     {
@@ -506,6 +508,7 @@ void __config_ResetAll( vlc_object_t *p_this )
     module_t *p_module;
     module_t **list = module_list_get (NULL);
 
+    vlc_rwlock_wrlock (&config_lock);
     for (size_t j = 0; (p_module = list[j]) != NULL; j++)
     {
         if( p_module->b_submodule ) continue;
@@ -514,7 +517,6 @@ void __config_ResetAll( vlc_object_t *p_this )
         {
             module_config_t *p_config = p_module->p_config + i;
 
-            vlc_mutex_lock (p_config->p_lock);
             if (IsConfigIntegerType (p_config->i_type))
                 p_config->value.i = p_config->orig.i;
             else
@@ -527,9 +529,9 @@ void __config_ResetAll( vlc_object_t *p_this )
                 p_config->value.psz =
                         strdupnull (p_config->orig.psz);
             }
-            vlc_mutex_unlock (p_config->p_lock);
         }
     }
+    vlc_rwlock_unlock (&config_lock);
 
     module_list_free (list);
 }

diff --git a/src/config/file.c b/src/config/file.c
index db1128dc266597eada44da777a956e3e3393ef18..e1fa786b76866136816c9fc2c408f57ca4104400 100644 (file)
--- a/src/config/file.c
+++ b/src/config/file.c
@@ -189,6 +189,7 @@ int __config_LoadConfigFile( vlc_object_t *p_this, const char *psz_module_name )
     locale_t loc = newlocale (LC_NUMERIC_MASK, "C", NULL);
     locale_t baseloc = uselocale (loc);
 
+    vlc_rwlock_wrlock (&config_lock);
     while (fgets (line, 1024, file) != NULL)
     {
         /* Ignore comments and empty lines */
@@ -263,7 +264,6 @@ int __config_LoadConfigFile( vlc_object_t *p_this, const char *psz_module_name )
             /* We found it */
             errno = 0;
 
-            vlc_mutex_lock( p_item->p_lock );
             switch( p_item->i_type )
             {
                 case CONFIG_ITEM_BOOL:
@@ -301,10 +301,10 @@ int __config_LoadConfigFile( vlc_object_t *p_this, const char *psz_module_name )
                     p_item->saved.psz = strdupnull (p_item->value.psz);
                     break;
             }
-            vlc_mutex_unlock( p_item->p_lock );
             break;
         }
     }
+    vlc_rwlock_unlock (&config_lock);
 
     if (ferror (file))
     {
@@ -532,6 +532,9 @@ static int SaveConfigFile( vlc_object_t *p_this, const char *psz_module_name,
         goto error;
     }
 
+    /* Configuration lock must be taken before vlcrc serializer below. */
+    vlc_rwlock_rdlock (&config_lock);
+
     /* The temporary configuration file is per-PID. Therefore SaveConfigFile()
      * should be serialized against itself within a given process. */
     static vlc_mutex_t lock = VLC_STATIC_MUTEX;
@@ -540,6 +543,7 @@ static int SaveConfigFile( vlc_object_t *p_this, const char *psz_module_name,
     int fd = utf8_open (temporary, O_CREAT|O_WRONLY|O_TRUNC, S_IRUSR|S_IWUSR);
     if (fd == -1)
     {
+        vlc_rwlock_unlock (&config_lock);
         vlc_mutex_unlock (&lock);
         module_list_free (list);
         goto error;
@@ -547,6 +551,7 @@ static int SaveConfigFile( vlc_object_t *p_this, const char *psz_module_name,
     file = fdopen (fd, "wt");
     if (file == NULL)
     {
+        vlc_rwlock_unlock (&config_lock);
         close (fd);
         vlc_mutex_unlock (&lock);
         module_list_free (list);
@@ -560,6 +565,10 @@ static int SaveConfigFile( vlc_object_t *p_this, const char *psz_module_name,
     locale_t loc = newlocale (LC_NUMERIC_MASK, "C", NULL);
     locale_t baseloc = uselocale (loc);
 
+    /* We would take the config lock here. But this would cause a lock
+     * inversion with the serializer above and config_AutoSaveConfigFile().
+    vlc_rwlock_rdlock (&config_lock);*/
+
     /* Look for the selected module, if NULL then save everything */
     for( i_index = 0; (p_parser = list[i_index]) != NULL; i_index++ )
     {
@@ -591,8 +600,6 @@ static int SaveConfigFile( vlc_object_t *p_this, const char *psz_module_name,
              || p_item->b_unsaveable)          /* ignore volatile option */
                 continue;
 
-            vlc_mutex_lock (p_item->p_lock);
-
             /* Do not save the new value in the configuration file
              * if doing an autosave, and the item is not an "autosaved" one. */
             bool b_retain = b_autosave && !p_item->b_autosave;
@@ -663,9 +670,9 @@ static int SaveConfigFile( vlc_object_t *p_this, const char *psz_module_name,
 
             if (!b_retain)
                 p_item->b_dirty = false;
-            vlc_mutex_unlock (p_item->p_lock);
         }
     }
+    vlc_rwlock_unlock (&config_lock);
 
     module_list_free (list);
     if (loc != (locale_t)0)
@@ -721,14 +728,15 @@ error:
 
 int config_AutoSaveConfigFile( vlc_object_t *p_this )
 {
-    size_t i_index;
+    int ret = VLC_SUCCESS;
     bool save = false;
 
     assert( p_this );
 
     /* Check if there's anything to save */
     module_t **list = module_list_get (NULL);
-    for( i_index = 0; list[i_index] && !save; i_index++ )
+    vlc_rwlock_rdlock (&config_lock);
+    for (size_t i_index = 0; list[i_index] && !save; i_index++)
     {
         module_t *p_parser = list[i_index];
         module_config_t *p_item, *p_end;
@@ -739,14 +747,17 @@ int config_AutoSaveConfigFile( vlc_object_t *p_this )
              p_item < p_end && !save;
              p_item++ )
         {
-            vlc_mutex_lock (p_item->p_lock);
             save = p_item->b_autosave && p_item->b_dirty;
-            vlc_mutex_unlock (p_item->p_lock);
         }
     }
-    module_list_free (list);
 
-    return save ? VLC_SUCCESS : SaveConfigFile( p_this, NULL, true );
+    if (save)
+        /* Note: this will get the read lock recursively. Ok. */
+        ret = SaveConfigFile (p_this, NULL, true);
+    vlc_rwlock_unlock (&config_lock);
+
+    module_list_free (list);
+    return ret;
 }
 
 int __config_SaveConfigFile( vlc_object_t *p_this, const char *psz_module_name )

diff --git a/src/modules/modules.c b/src/modules/modules.c
index b6592024e6fa9d9e1b926752bb89985eac520d89..8f20a8ebbb9f3b3508709dd47ad507a235cc1620 100644 (file)
--- a/src/modules/modules.c
+++ b/src/modules/modules.c
@@ -137,6 +137,7 @@ void __module_InitBank( vlc_object_t *p_this )
          * options of main will be available in the module bank structure just
          * as for every other module. */
         AllocateBuiltinModule( p_this, vlc_entry__main );
+        vlc_rwlock_init (&config_lock);
     }
     else
         p_module_bank->i_usage++;
@@ -180,6 +181,7 @@ void module_EndBank( vlc_object_t *p_this, bool b_plugins )
         vlc_mutex_unlock( &module_lock );
         return;
     }
+    vlc_rwlock_destroy (&config_lock);
     p_module_bank = NULL;
     vlc_mutex_unlock( &module_lock );