* ./src/misc/variables.c: callback loops are now detected; this means you

    can now use var_* functions from within your callback, they can even
    trigger callback themselves, to any level. The main restriction is that
    you shouldn't meddle with a variable that is already being triggered by
    the current thread (other threads will just wait).
  * ./src/misc/objects.c: fixed a deadlock in the "tree" command.
  * ./modules/misc/testsuite/test4.c: added a "callback-test" command to the
    rc interface to test callback loop detection and concurrent triggers.

include/variables.h

@@ -2,7 +2,7 @@
  * variables.h: variables handling
  *****************************************************************************
  * Copyright (C) 2002 VideoLAN
- * $Id: variables.h,v 1.4 2002/10/16 19:39:42 sam Exp $
+ * $Id: variables.h,v 1.5 2002/10/17 13:15:30 sam Exp $
  *
  * Authors: Samuel Hocevar <sam@zoy.org>
  *
@@ -37,9 +37,13 @@ struct variable_t
     u32          i_hash;
     int          i_type;
 
-    /* Lots of other things that can be added */
+    /* Creation count: we only destroy the variable if it reaches 0 */
     int          i_usage;
 
+    /* Set to TRUE if the variable is in a callback */
+    vlc_bool_t   b_incallback;
+
+    /* Registered callbacks */
     int                i_entries;
     callback_entry_t * p_entries;
 };

include/vlc/vlc.h

@@ -2,7 +2,7 @@
  * vlc.h: global header for vlc
  *****************************************************************************
  * Copyright (C) 1998, 1999, 2000 VideoLAN
- * $Id: vlc.h,v 1.16 2002/10/14 16:46:55 sam Exp $
+ * $Id: vlc.h,v 1.17 2002/10/17 13:15:30 sam Exp $
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -50,6 +50,7 @@ typedef union
 #define VLC_SUCCESS         -0                                   /* No error */
 #define VLC_ENOMEM          -1                          /* Not enough memory */
 #define VLC_ETHREAD         -2                               /* Thread error */
+#define VLC_ETIMEOUT        -3                                    /* Timeout */
 
 #define VLC_ENOMOD         -10                           /* Module not found */
 

modules/misc/testsuite/test4.c

@@ -2,7 +2,7 @@
  * test4.c : Miscellaneous stress tests module for vlc
  *****************************************************************************
  * Copyright (C) 2002 VideoLAN
- * $Id: test4.c,v 1.2 2002/10/14 19:04:51 sam Exp $
+ * $Id: test4.c,v 1.3 2002/10/17 13:15:30 sam Exp $
  *
  * Authors: Samuel Hocevar <sam@zoy.org>
  *
@@ -41,6 +41,11 @@
 /*****************************************************************************
  * Local prototypes.
  *****************************************************************************/
+static int    Callback  ( vlc_object_t *, char *, char * );
+static int    MyCallback( vlc_object_t *, char const *,
+                          vlc_value_t, vlc_value_t, void * );
+static void * MyThread  ( vlc_object_t * );
+
 static int    Stress    ( vlc_object_t *, char *, char * );
 static void * Dummy     ( vlc_object_t * );
 
@@ -51,14 +56,139 @@ static int    Signal    ( vlc_object_t *, char *, char * );
  *****************************************************************************/
 vlc_module_begin();
     set_description( _("Miscellaneous stress tests") );
-    var_Create( p_module->p_libvlc, "stress", VLC_VAR_COMMAND );
-    var_Set( p_module->p_libvlc, "stress", (vlc_value_t)(void*)Stress );
+    var_Create( p_module->p_libvlc, "callback-test", VLC_VAR_COMMAND );
+    var_Set( p_module->p_libvlc, "callback-test", (vlc_value_t)(void*)Callback );
+    var_Create( p_module->p_libvlc, "stress-test", VLC_VAR_COMMAND );
+    var_Set( p_module->p_libvlc, "stress-test", (vlc_value_t)(void*)Stress );
     var_Create( p_module->p_libvlc, "signal", VLC_VAR_COMMAND );
     var_Set( p_module->p_libvlc, "signal", (vlc_value_t)(void*)Signal );
 vlc_module_end();
 
 /*****************************************************************************
- * Stress: perform various tests
+ * Callback: test callback functions
+ *****************************************************************************/
+static int Callback( vlc_object_t *p_this, char *psz_cmd, char *psz_arg )
+{
+    int i;
+    char psz_var[20];
+    vlc_object_t *pp_objects[10];
+    vlc_value_t val;
+
+    /* Allocate a few variables */
+    for( i = 0; i < 1000; i++ )
+    {
+        sprintf( psz_var, "blork-%i", i );
+        var_Create( p_this, psz_var, VLC_VAR_INTEGER );
+        var_AddCallback( p_this, psz_var, MyCallback, NULL );
+    }
+
+    /*
+     *  Test #1: callback loop detection
+     */
+    printf( "Test #1: callback loop detection\n" );
+
+    printf( " - without boundary check (vlc should print an error)\n" );
+    val.i_int = 1234567;
+    var_Set( p_this, "blork-12", val );
+
+    printf( " - with boundary check\n" );
+    val.i_int = 12;
+    var_Set( p_this, "blork-12", val );
+
+    /*
+     *  Test #2: concurrent access
+     */
+    printf( "Test #2: concurrent access\n" );
+
+    printf( " - launch worker threads\n" );
+
+    for( i = 0; i < 10; i++ )
+    {
+        pp_objects[i] = vlc_object_create( p_this, VLC_OBJECT_GENERIC );
+        vlc_object_attach( pp_objects[i], p_this );
+        vlc_thread_create( pp_objects[i], "foo", MyThread, 0, VLC_TRUE );
+    }
+
+    msleep( 3000000 );
+
+    printf( " - kill worker threads\n" );
+
+    for( i = 0; i < 10; i++ )
+    {
+        pp_objects[i]->b_die = VLC_TRUE;
+        vlc_thread_join( pp_objects[i] );
+        vlc_object_detach( pp_objects[i] );
+        vlc_object_destroy( pp_objects[i] );
+    }
+
+    /* Clean our mess */
+    for( i = 0; i < 1000; i++ )
+    {
+        sprintf( psz_var, "blork-%i", i );
+        var_DelCallback( p_this, psz_var, MyCallback, NULL );
+        var_Destroy( p_this, psz_var );
+    }
+
+    return VLC_SUCCESS;
+}
+
+/*****************************************************************************
+ * MyCallback: used by callback-test
+ *****************************************************************************/
+static int MyCallback( vlc_object_t *p_this, char const *psz_var,
+                       vlc_value_t oldval, vlc_value_t newval, void *p_data )
+{
+    int i_var = 1 + atoi( psz_var + strlen("blork-") );
+    char psz_newvar[20];
+
+    if( i_var == 1000 )
+    {
+        i_var = 0;
+    }
+
+    /* If we are requested to stop, then stop. */
+    if( i_var == newval.i_int )
+    {
+        return VLC_SUCCESS;
+    }
+
+    /* If we're the blork-3 callback, set blork-4, and so on. */
+    sprintf( psz_newvar, "blork-%i", i_var );
+    var_Set( p_this, psz_newvar, newval );
+
+    return VLC_SUCCESS;   
+}
+
+/*****************************************************************************
+ * MyThread: used by callback-test, creates objects and then do nothing.
+ *****************************************************************************/
+static void * MyThread( vlc_object_t *p_this )
+{
+    char psz_var[20];
+    vlc_value_t val;
+    vlc_object_t *p_parent = p_this->p_parent;
+
+    vlc_thread_ready( p_this );
+
+    val.i_int = 42;
+
+    while( !p_this->b_die )
+    {
+        int i = (int) (100.0 * rand() / (RAND_MAX));
+
+        sprintf( psz_var, "blork-%i", i );
+        val.i_int = i + 200;
+        var_Set( p_parent, psz_var, val );
+
+        /* This is quite heavy, but we only have 10 threads. Keep cool. */
+        msleep( 1000 );
+    }
+
+    return NULL;
+}
+
+/*****************************************************************************
+ * Stress: perform various stress tests
  *****************************************************************************/
 static int Stress( vlc_object_t *p_this, char *psz_cmd, char *psz_arg )
 {
@@ -212,7 +342,7 @@ static int Stress( vlc_object_t *p_this, char *psz_cmd, char *psz_arg )
 }
 
 /*****************************************************************************
- * Dummy: used by test, creates objects and then do nothing.
+ * Dummy: used by stress-test, creates objects and then do nothing.
  *****************************************************************************/
 static void * Dummy( vlc_object_t *p_this )
 {

src/misc/error.c

@@ -2,7 +2,7 @@
  * error.c: error handling routine
  *****************************************************************************
  * Copyright (C) 2002 VideoLAN
- * $Id: error.c,v 1.1 2002/10/14 16:35:18 sam Exp $
+ * $Id: error.c,v 1.2 2002/10/17 13:15:31 sam Exp $
  *
  * Authors: Samuel Hocevar <sam@zoy.org>
  *
@@ -43,6 +43,8 @@ char const * vlc_error ( int i_err )
             return "not enough memory";
         case VLC_ETHREAD:
             return "thread error";
+        case VLC_ETIMEOUT:
+            return "timeout";
 
         case VLC_ENOMOD:
             return "module not found";

src/misc/objects.c

@@ -2,7 +2,7 @@
  * objects.c: vlc_object_t handling
  *****************************************************************************
  * Copyright (C) 2002 VideoLAN
- * $Id: objects.c,v 1.25 2002/10/14 16:46:56 sam Exp $
+ * $Id: objects.c,v 1.26 2002/10/17 13:15:31 sam Exp $
  *
  * Authors: Samuel Hocevar <sam@zoy.org>
  *
@@ -532,8 +532,6 @@ vlc_list_t * __vlc_list_find( vlc_object_t *p_this, int i_type, int i_mode )
  *****************************************************************************/
 static int DumpCommand( vlc_object_t *p_this, char *psz_cmd, char *psz_arg )
 {
-    vlc_mutex_lock( &structure_lock );
-
     if( *psz_cmd == 't' )
     {
         char psz_foo[2 * MAX_DUMPSTRUCTURE_DEPTH + 1];
@@ -553,13 +551,19 @@ static int DumpCommand( vlc_object_t *p_this, char *psz_cmd, char *psz_arg )
             p_object = p_this->p_vlc ? VLC_OBJECT(p_this->p_vlc) : p_this;
         }
 
+        vlc_mutex_lock( &structure_lock );
+
         psz_foo[0] = '|';
         DumpStructure( p_object, 0, psz_foo );
+
+        vlc_mutex_unlock( &structure_lock );
     }
     else if( *psz_cmd == 'l' )
     {
         vlc_object_t **pp_current, **pp_end;
 
+        vlc_mutex_lock( &structure_lock );
+
         pp_current = p_this->p_libvlc->pp_objects;
         pp_end = pp_current + p_this->p_libvlc->i_objects;
 
@@ -576,9 +580,9 @@ static int DumpCommand( vlc_object_t *p_this, char *psz_cmd, char *psz_arg )
                         (*pp_current)->psz_object_type );
             }
         }
-    }
 
-    vlc_mutex_unlock( &structure_lock );
+        vlc_mutex_unlock( &structure_lock );
+    }
 
     return VLC_SUCCESS;
 }

src/misc/variables.c

@@ -2,7 +2,7 @@
  * variables.c: routines for object variables handling
  *****************************************************************************
  * Copyright (C) 2002 VideoLAN
- * $Id: variables.c,v 1.7 2002/10/16 19:39:42 sam Exp $
+ * $Id: variables.c,v 1.8 2002/10/17 13:15:31 sam Exp $
  *
  * Authors: Samuel Hocevar <sam@zoy.org>
  *
@@ -42,6 +42,7 @@ struct callback_entry_t
 /*****************************************************************************
  * Local prototypes
  *****************************************************************************/
+static int GetUnused      ( vlc_object_t *, const char * );
 static u32 HashString     ( const char * );
 static int Insert         ( variable_t *, int, const char * );
 static int InsertInner    ( variable_t *, int, u32 );
@@ -106,6 +107,8 @@ int __var_Create( vlc_object_t *p_this, const char *psz_name, int i_type )
 
     p_var->i_usage = 1;
 
+    p_var->b_incallback = VLC_FALSE;
+
     p_var->i_entries = 0;
     p_var->p_entries = NULL;
 
@@ -152,20 +155,19 @@ int __var_Create( vlc_object_t *p_this, const char *psz_name, int i_type )
  *****************************************************************************/
 int __var_Destroy( vlc_object_t *p_this, const char *psz_name )
 {
-    int i_del;
+    int i_var;
     variable_t *p_var;
 
     vlc_mutex_lock( &p_this->var_lock );
 
-    i_del = Lookup( p_this->p_vars, p_this->i_vars, psz_name );
-
-    if( i_del < 0 )
+    i_var = GetUnused( p_this, psz_name );
+    if( i_var < 0 )
     {
         vlc_mutex_unlock( &p_this->var_lock );
-        return VLC_ENOVAR;
+        return i_var;
     }
 
-    p_var = &p_this->p_vars[i_del];
+    p_var = &p_this->p_vars[i_var];
 
     if( p_var->i_usage > 1 )
     {
@@ -197,9 +199,9 @@ int __var_Destroy( vlc_object_t *p_this, const char *psz_name )
 
     free( p_var->psz_name );
 
-    memmove( p_this->p_vars + i_del,
-             p_this->p_vars + i_del + 1,
-             (p_this->i_vars - i_del - 1) * sizeof(variable_t) );
+    memmove( p_this->p_vars + i_var,
+             p_this->p_vars + i_var + 1,
+             (p_this->i_vars - i_var - 1) * sizeof(variable_t) );
 
     if( (p_this->i_vars & 15) == 0 )
     {
@@ -253,12 +255,11 @@ int __var_Set( vlc_object_t *p_this, const char *psz_name, vlc_value_t val )
 
     vlc_mutex_lock( &p_this->var_lock );
 
-    i_var = Lookup( p_this->p_vars, p_this->i_vars, psz_name );
-
+    i_var = GetUnused( p_this, psz_name );
     if( i_var < 0 )
     {
         vlc_mutex_unlock( &p_this->var_lock );
-        return VLC_ENOVAR;
+        return i_var;
     }
 
     p_var = &p_this->p_vars[i_var];
@@ -276,23 +277,41 @@ int __var_Set( vlc_object_t *p_this, const char *psz_name, vlc_value_t val )
     /* Backup needed stuff */
     oldval = p_var->val;
 
-    /* Set the variable */
-    p_var->val = val;
-
-    /* Deal with callbacks */
+    /* Deal with callbacks. Tell we're in a callback, release the lock,
+     * call stored functions, retake the lock. */
     if( p_var->i_entries )
     {
-        int i;
+        int i_var;
+        int i_entries = p_var->i_entries;
+        callback_entry_t *p_entries = p_var->p_entries;
 
-        for( i = p_var->i_entries ; i-- ; )
+        p_var->b_incallback = VLC_TRUE;
+        vlc_mutex_unlock( &p_this->var_lock );
+
+        /* The real calls */
+        for( ; i_entries-- ; )
+        {
+            p_entries[i_entries].pf_callback( p_this, psz_name, oldval, val,
+                                              p_entries[i_entries].p_data );
+        }
+
+        vlc_mutex_lock( &p_this->var_lock );
+
+        i_var = Lookup( p_this->p_vars, p_this->i_vars, psz_name );
+        if( i_var < 0 )
         {
-            /* FIXME: oooh, see the deadlocks! see the race conditions! */
-            p_var->p_entries[i].pf_callback( p_this, p_var->psz_name,
-                                             oldval, val,
-                                             p_var->p_entries[i].p_data );
+            msg_Err( p_this, "variable %s has disappeared" );
+            vlc_mutex_unlock( &p_this->var_lock );
+            return VLC_ENOVAR;
         }
+
+        p_var = &p_this->p_vars[i_var];
+        p_var->b_incallback = VLC_FALSE;
     }
 
+    /* Set the variable */
+    p_var->val = val;
+
     /* Free data if needed */
     switch( p_var->i_type )
     {
@@ -385,20 +404,19 @@ int __var_Get( vlc_object_t *p_this, const char *psz_name, vlc_value_t *p_val )
 int __var_AddCallback( vlc_object_t *p_this, const char *psz_name,
                        vlc_callback_t pf_callback, void *p_data )
 {
-    int i_var, i_entry;
+    int i_entry, i_var;
     variable_t *p_var;
 
     vlc_mutex_lock( &p_this->var_lock );
 
-    i_var = Lookup( p_this->p_vars, p_this->i_vars, psz_name );
+    i_var = GetUnused( p_this, psz_name );
     if( i_var < 0 )
     {
         vlc_mutex_unlock( &p_this->var_lock );
-        return VLC_ENOVAR;
+        return i_var;
     }
 
     p_var = &p_this->p_vars[i_var];
-
     i_entry = p_var->i_entries++;
 
     if( i_entry )
@@ -428,16 +446,16 @@ int __var_AddCallback( vlc_object_t *p_this, const char *psz_name,
 int __var_DelCallback( vlc_object_t *p_this, const char *psz_name,
                        vlc_callback_t pf_callback, void *p_data )
 {
-    int i_var, i_entry;
+    int i_entry, i_var;
     variable_t *p_var;
 
     vlc_mutex_lock( &p_this->var_lock );
 
-    i_var = Lookup( p_this->p_vars, p_this->i_vars, psz_name );
+    i_var = GetUnused( p_this, psz_name );
     if( i_var < 0 )
     {
         vlc_mutex_unlock( &p_this->var_lock );
-        return VLC_ENOVAR;
+        return i_var;
     }
 
     p_var = &p_this->p_vars[i_var];
@@ -481,6 +499,41 @@ int __var_DelCallback( vlc_object_t *p_this, const char *psz_name,
 
 /* Following functions are local */
 
+/*****************************************************************************
+ * GetUnused: find an unused variable from its name
+ *****************************************************************************
+ * We do i_tries tries before giving up, just in case the variable is being
+ * modified and called from a callback.
+ *****************************************************************************/
+static int GetUnused( vlc_object_t *p_this, const char *psz_name )
+{
+    int i_var, i_tries = 0;
+
+    while( VLC_TRUE )
+    {
+        i_var = Lookup( p_this->p_vars, p_this->i_vars, psz_name );
+        if( i_var < 0 )
+        {
+            return VLC_ENOVAR;
+        }
+
+        if( ! p_this->p_vars[i_var].b_incallback )
+        {
+            return i_var;
+        }
+
+        if( i_tries++ > 100 )
+        {
+            msg_Err( p_this, "caught in a callback deadlock?" );
+            return VLC_ETIMEOUT;
+        }
+
+        vlc_mutex_unlock( &p_this->var_lock );
+        msleep( THREAD_SLEEP );
+        vlc_mutex_lock( &p_this->var_lock );
+    }
+}
+
 /*****************************************************************************
  * HashString: our cool hash function
  *****************************************************************************