[vlc-devel] commit: Allocate variable and inherit value before the variables lock ( Rémi Denis-Courmont )
git version control
git at videolan.org
Tue Dec 29 21:33:10 CET 2009
vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Tue Dec 29 22:16:32 2009 +0200| [8b045d0d882c4509b5af9cc43597579679ec74bf] | committer: Rémi Denis-Courmont
Allocate variable and inherit value before the variables lock
The initial value of the variable must be correct when the variables
lock is released after the variable was created. Hence we could not
release the lock between Insert() and InheritValue(). If we did, there
would be a race where another thread could see the variable with the
generic default value (0, "", 0., ...) instead of the inherited value.
So instead, we inherit the value in a temporary variable on the stack,
before we take the variables lock. Then we can create the variable with
the correct value without taking the lock for the duration of
InheritValue().
This adds overhead when an existing variable is re-created (i.e.
reference count is increased but no new variable is created). But it
dramatically reduces contention on the variables lock. More importantly,
it allows calling InheritValue() without the variables lock. So when we
fix InheritValue(), which is currently not thread-safe, we won't have
any problem with nested locking.
> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=8b045d0d882c4509b5af9cc43597579679ec74bf
---
src/misc/variables.c | 132 +++++++++++++++++++++++++------------------------
1 files changed, 67 insertions(+), 65 deletions(-)
diff --git a/src/misc/variables.c b/src/misc/variables.c
index 45d8a24..b2e02af 100644
--- a/src/misc/variables.c
+++ b/src/misc/variables.c
@@ -179,55 +179,11 @@ static int TriggerCallback( vlc_object_t *, variable_t **, const char *,
*/
int __var_Create( vlc_object_t *p_this, const char *psz_name, int i_type )
{
- int i_new;
- variable_t *p_var;
+ variable_t var, *p_var = &var;
static vlc_list_t dummy_null_list = {0, NULL, NULL};
assert( p_this );
- vlc_object_internals_t *p_priv = vlc_internals( p_this );
-
- vlc_mutex_lock( &p_priv->var_lock );
-
- /* FIXME: if the variable already exists, we don't duplicate it. But we
- * duplicate the lookups. It's not that serious, but if anyone finds some
- * time to rework Insert() so that only one lookup has to be done, feel
- * free to do so. */
- i_new = Lookup( p_priv->p_vars, p_priv->i_vars, psz_name );
-
- if( i_new >= 0 )
- {
- /* If the types differ, variable creation failed. */
- if( (i_type & VLC_VAR_CLASS) != (p_priv->p_vars[i_new].i_type & VLC_VAR_CLASS) )
- {
- msg_Err( p_this, "Variable '%s' (0x%04x) already exist but with a different type (0x%04x)",
- psz_name, p_priv->p_vars[i_new].i_type, i_type );
- vlc_mutex_unlock( &p_priv->var_lock );
- return VLC_EBADVAR;
- }
-
- p_priv->p_vars[i_new].i_usage++;
- p_priv->p_vars[i_new].i_type |= ( i_type & VLC_VAR_ISCOMMAND );
- p_priv->p_vars[i_new].i_type |= ( i_type & VLC_VAR_HASCHOICE );
- vlc_mutex_unlock( &p_priv->var_lock );
- return VLC_SUCCESS;
- }
-
- i_new = Insert( p_priv->p_vars, p_priv->i_vars, psz_name );
-
- if( (p_priv->i_vars & 15) == 15 )
- {
- p_priv->p_vars = xrealloc( p_priv->p_vars,
- (p_priv->i_vars+17) * sizeof(variable_t) );
- }
-
- memmove( p_priv->p_vars + i_new + 1,
- p_priv->p_vars + i_new,
- (p_priv->i_vars - i_new) * sizeof(variable_t) );
-
- p_priv->i_vars++;
-
- p_var = &p_priv->p_vars[i_new];
memset( p_var, 0, sizeof(*p_var) );
p_var->i_hash = HashString( psz_name );
@@ -291,36 +247,82 @@ int __var_Create( vlc_object_t *p_this, const char *psz_name, int i_type )
break;
}
- /* Duplicate the default data we stored. */
- p_var->ops->pf_dup( &p_var->val );
-
if( i_type & VLC_VAR_DOINHERIT )
{
- vlc_value_t val;
+ if( InheritValue( p_this, psz_name, &p_var->val, p_var->i_type ) )
+ msg_Err( p_this, "cannot inherit value for %s", psz_name );
+ else if( i_type & VLC_VAR_HASCHOICE )
+ {
+ /* We must add the inherited value to our choice list */
+ p_var->i_default = 0;
+
+ INSERT_ELEM( p_var->choices.p_values, p_var->choices.i_count,
+ 0, p_var->val );
+ INSERT_ELEM( p_var->choices_text.p_values,
+ p_var->choices_text.i_count, 0, p_var->val );
+ p_var->ops->pf_dup( &p_var->choices.p_values[0] );
+ p_var->choices_text.p_values[0].psz_string = NULL;
+ }
+ }
+
+ vlc_object_internals_t *p_priv = vlc_internals( p_this );
+ int i_new;
+
+ vlc_mutex_lock( &p_priv->var_lock );
+
+ /* FIXME: if the variable already exists, we don't duplicate it. But we
+ * duplicate the lookups. It's not that serious, but if anyone finds some
+ * time to rework Insert() so that only one lookup has to be done, feel
+ * free to do so. */
+ i_new = Lookup( p_priv->p_vars, p_priv->i_vars, psz_name );
- if( InheritValue( p_this, psz_name, &val, p_var->i_type )
- == VLC_SUCCESS )
+ if( i_new >= 0 )
+ {
+ /* If the types differ, variable creation failed. */
+ if( (i_type & VLC_VAR_CLASS) != (p_priv->p_vars[i_new].i_type & VLC_VAR_CLASS) )
{
- /* Free data if needed */
- p_var->ops->pf_free( &p_var->val );
- /* Set the variable */
- p_var->val = val;
+ msg_Err( p_this, "Variable '%s' (0x%04x) already exist but with a different type (0x%04x)",
+ psz_name, p_priv->p_vars[i_new].i_type, i_type );
+ vlc_mutex_unlock( &p_priv->var_lock );
+ return VLC_EBADVAR;
+ }
+
+ p_priv->p_vars[i_new].i_usage++;
+ p_priv->p_vars[i_new].i_type |= ( i_type & VLC_VAR_ISCOMMAND );
+ p_priv->p_vars[i_new].i_type |= ( i_type & VLC_VAR_HASCHOICE );
+ vlc_mutex_unlock( &p_priv->var_lock );
- if( i_type & VLC_VAR_HASCHOICE )
+ /* We did not need to create a new variable, free everything... */
+ p_var->ops->pf_free( &p_var->val );
+ free( p_var->psz_name );
+ if( p_var->choices.i_count )
+ {
+ for( int i = 0 ; i < p_var->choices.i_count ; i++ )
{
- /* We must add the inherited value to our choice list */
- p_var->i_default = 0;
-
- INSERT_ELEM( p_var->choices.p_values, p_var->choices.i_count,
- 0, val );
- INSERT_ELEM( p_var->choices_text.p_values,
- p_var->choices_text.i_count, 0, val );
- p_var->ops->pf_dup( &p_var->choices.p_values[0] );
- p_var->choices_text.p_values[0].psz_string = NULL;
+ p_var->ops->pf_free( &p_var->choices.p_values[i] );
+ free( p_var->choices_text.p_values[i].psz_string );
}
+ free( p_var->choices.p_values );
+ free( p_var->choices_text.p_values );
}
+ return VLC_SUCCESS;
+ }
+
+ i_new = Insert( p_priv->p_vars, p_priv->i_vars, psz_name );
+
+ if( (p_priv->i_vars & 15) == 15 )
+ {
+ p_priv->p_vars = xrealloc( p_priv->p_vars,
+ (p_priv->i_vars+17) * sizeof(variable_t) );
}
+ memmove( p_priv->p_vars + i_new + 1,
+ p_priv->p_vars + i_new,
+ (p_priv->i_vars - i_new) * sizeof(variable_t) );
+
+ p_priv->i_vars++;
+
+ p_priv->p_vars[i_new] = var;
vlc_mutex_unlock( &p_priv->var_lock );
return VLC_SUCCESS;
More information about the vlc-devel
mailing list