* doc/TODO:

- Remove pending call locking todo item
- dbus_connection_open now holds hard ref.  Remove todo item
- do proper locking on _dbus_bus_check_connection_and_unref
  and handle DBUS_BUS_STARTER. Remove todo item
- Warn on closing of a shared connection.  Remove todo item

* bus/bus.c, bus/connection.c, bus/dispatch.c, dbus/dbus-bus.c,
  dbus/dbus-connection.c: Use the dbus_connection_close_internal
  so we don't get the warning when closing shared connections

* test/test-service.c, test/test-shell-service.c: Applications
  don't close shared connections themselves so we unref instead of
  close

* test/test-utils.c (test_connection_shutdown): Close the connection

* dbus/dbus-bus.c (_dbus_bus_check_connection_and_unref): Changed to
  _dbus_bus_check_connection_and_unref_unlocked since we only call this
  method on a locked connection.
  Make sure we call _dbus_connection_unref_unlocked instead of
  dbus_connection_unref also.
  Handle DBUS_BUS_STARTER correctly

* dbus/dbus-connection.c (connection_record_shared_unlocked):
  Mark as shared and hard ref the connection
  (connection_forget_shared_unlocked): Remove the hard ref from the
  connection
  (_dbus_connection_close_internal_and_unlock):  New internal function
  which takes a locked connection and unlocks it after closing it
  (_dbus_connection_close_internal): New internal function which acts
  like the origonal dbus_connection_close method by grabbing a connection
  lock and calling _dbus_connection_close_internal_and_unlock
  (dbus_connection_close): Public close method, warns when the app
  trys to close a shared connection

9 files changed, 111 insertions, 39 deletions

diff --git a/ChangeLog b/ChangeLog
index 247f3c9b..6dfff6eb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,43 @@
 2006-09-06  John (J5) Palmieri  <johnp@redhat.com>
 
+	* doc/TODO:
+	- Remove pending call locking todo item
+	- dbus_connection_open now holds hard ref.  Remove todo item
+	- do proper locking on _dbus_bus_check_connection_and_unref
+	  and handle DBUS_BUS_STARTER. Remove todo item
+	- Warn on closing of a shared connection.  Remove todo item
+
+	* bus/bus.c, bus/connection.c, bus/dispatch.c, dbus/dbus-bus.c,
+	dbus/dbus-connection.c: Use the dbus_connection_close_internal
+	so we don't get the warning when closing shared connections
+
+	* test/test-service.c, test/test-shell-service.c: Applications
+	don't close shared connections themselves so we unref instead of
+	close
+
+	* test/test-utils.c (test_connection_shutdown): Close the connection
+
+	* dbus/dbus-bus.c (_dbus_bus_check_connection_and_unref): Changed to
+	_dbus_bus_check_connection_and_unref_unlocked since we only call this
+	method on a locked connection.  
+	Make sure we call _dbus_connection_unref_unlocked instead of 
+	dbus_connection_unref also.
+	Handle DBUS_BUS_STARTER correctly
+
+	* dbus/dbus-connection.c (connection_record_shared_unlocked):
+	Mark as shared and hard ref the connection
+	(connection_forget_shared_unlocked): Remove the hard ref from the 
+	connection
+	(_dbus_connection_close_internal_and_unlock):  New internal function
+	which takes a locked connection and unlocks it after closing it
+	(_dbus_connection_close_internal): New internal function which acts
+	like the origonal dbus_connection_close method by grabbing a connection
+	lock and calling _dbus_connection_close_internal_and_unlock
+	(dbus_connection_close): Public close method, warns when the app
+	trys to close a shared connection
+
+2006-09-06  John (J5) Palmieri  <johnp@redhat.com>
+
 	* bus/driver.c:
 	(bus_driver_generate_introspect_string): New method for populating
 	a DBusString with the introspect data
diff --git a/dbus/dbus-bus.c b/dbus/dbus-bus.c
index eae46051..fd58fab8 100644
--- a/dbus/dbus-bus.c
+++ b/dbus/dbus-bus.c
@@ -28,6 +28,7 @@
 #include "dbus-message.h"
 #include "dbus-marshal-validate.h"
 #include "dbus-threads-internal.h"
+#include "dbus-connection-internal.h"
 #include <string.h>
 
 /**
@@ -303,20 +304,29 @@ ensure_bus_data (DBusConnection *connection)
 }
 
 /* internal function that checks to see if this
-   is a shared bus connection and if it is unref it */
+   is a shared connection owned by the bus and if it is unref it */
 void
-_dbus_bus_check_connection_and_unref (DBusConnection *connection)
+_dbus_bus_check_connection_and_unref_unlocked (DBusConnection *connection)
 {
+  _DBUS_LOCK (bus);
+
   if (bus_connections[DBUS_BUS_SYSTEM] == connection)
     {
       bus_connections[DBUS_BUS_SYSTEM] = NULL;
-      dbus_connection_unref (connection);
+      _dbus_connection_unref_unlocked (connection);
     }
   else if (bus_connections[DBUS_BUS_SESSION] == connection)
     {
       bus_connections[DBUS_BUS_SESSION] = NULL;
-      dbus_connection_unref (connection);
+      _dbus_connection_unref_unlocked (connection);
     }
+  else if (bus_connections[DBUS_BUS_STARTER] == connection)
+    {
+      bus_connections[DBUS_BUS_STARTER] = NULL;
+      _dbus_connection_unref_unlocked (connection);
+    }
+
+  _DBUS_UNLOCK (bus);
 }
 
 static DBusConnection *
@@ -394,7 +404,7 @@ internal_bus_get (DBusBusType  type,
   if (!dbus_bus_register (connection, error))
     {
       _DBUS_ASSERT_ERROR_IS_SET (error);
-      dbus_connection_close (connection);
+      _dbus_connection_close_internal (connection);
       dbus_connection_unref (connection);
 
       _DBUS_UNLOCK (bus);
diff --git a/dbus/dbus-bus.h b/dbus/dbus-bus.h
index c47af153..6cefe353 100644
--- a/dbus/dbus-bus.h
+++ b/dbus/dbus-bus.h
@@ -68,7 +68,7 @@ void            dbus_bus_remove_match     (DBusConnection *connection,
                                            const char     *rule,
                                            DBusError      *error);
 
-void           _dbus_bus_check_connection_and_unref (DBusConnection *connection);
+void           _dbus_bus_check_connection_and_unref_unlocked (DBusConnection *connection);
 
 DBUS_END_DECLS
 
diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h
index adf17862..4506f634 100644
--- a/dbus/dbus-connection-internal.h
+++ b/dbus/dbus-connection-internal.h
@@ -77,6 +77,7 @@ DBusConnection*   _dbus_connection_new_for_transport           (DBusTransport
 void              _dbus_connection_do_iteration_unlocked       (DBusConnection     *connection,
                                                                 unsigned int        flags,
                                                                 int                 timeout_milliseconds);
+void              _dbus_connection_close_internal              (DBusConnection *connection);
 
 DBusPendingCall*  _dbus_pending_call_new                       (DBusConnection     *connection,
                                                                 int                 timeout_milliseconds,
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 06c08d01..8033c4a5 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -239,7 +239,9 @@ struct DBusConnection
   char *server_guid; /**< GUID of server if we are in shared_connections, #NULL if server GUID is unknown or connection is private */
 
   unsigned int shareable : 1; /**< #TRUE if connection can go in shared_connections once we know the GUID */
-  
+ 
+  unsigned int shared : 1; /** < #TRUE if connection is shared and we hold a ref to it */
+
   unsigned int dispatch_acquired : 1; /**< Someone has dispatch path (can drain incoming queue) */
   unsigned int io_path_acquired : 1;  /**< Someone has transport io path (can use the transport to read/write messages) */
   
@@ -1360,6 +1362,7 @@ shared_connections_shutdown (void *data)
   _DBUS_LOCK (shared_connections);
 
   _dbus_assert (_dbus_hash_table_get_n_entries (shared_connections) == 0);
+
   _dbus_hash_table_unref (shared_connections);
   shared_connections = NULL;
   
@@ -1474,6 +1477,10 @@ connection_record_shared_unlocked (DBusConnection *connection,
     }
 
   connection->server_guid = guid_in_connection;
+  connection->shared = TRUE;
+
+  /* get a hard ref on this connection */
+  dbus_connection_ref (connection);
 
   _dbus_verbose ("stored connection to %s to be shared\n",
                  connection->server_guid);
@@ -1489,7 +1496,7 @@ static void
 connection_forget_shared_unlocked (DBusConnection *connection)
 {
   HAVE_LOCK_CHECK (connection);
-  
+ 
   if (connection->server_guid == NULL)
     return;
 
@@ -1505,6 +1512,8 @@ connection_forget_shared_unlocked (DBusConnection *connection)
   dbus_free (connection->server_guid);
   connection->server_guid = NULL;
 
+  /* remove the hash ref */
+  _dbus_connection_unref_unlocked (connection);
   _DBUS_UNLOCK (shared_connections);
 }
 
@@ -1605,7 +1614,7 @@ _dbus_connection_open_internal (const char     *address,
                   !connection_record_shared_unlocked (connection, guid))
                 {
                   _DBUS_SET_OOM (&tmp_error);
-                  dbus_connection_close (connection);
+                  _dbus_connection_close_internal (connection);
                   dbus_connection_unref (connection);
                   connection = NULL;
                 }
@@ -1896,6 +1905,32 @@ dbus_connection_unref (DBusConnection *connection)
     _dbus_connection_last_unref (connection);
 }
 
+static void
+_dbus_connection_close_internal_and_unlock (DBusConnection *connection)
+{
+  DBusDispatchStatus status;
+  
+  _dbus_verbose ("Disconnecting %p\n", connection);
+  
+  _dbus_transport_disconnect (connection->transport);
+
+  _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME);
+  status = _dbus_connection_get_dispatch_status_unlocked (connection);
+
+  /* this calls out to user code */
+  _dbus_connection_update_dispatch_status_and_unlock (connection, status);
+}
+
+void
+_dbus_connection_close_internal (DBusConnection *connection)
+{
+  _dbus_assert (connection != NULL);
+  _dbus_assert (connection->generation == _dbus_current_generation);
+
+  CONNECTION_LOCK (connection);
+  _dbus_connection_close_internal_and_unlock (connection);
+}
+
 /**
  * Closes the connection, so no further data can be sent or received.
  * Any further attempts to send data will result in errors.  This
@@ -1907,27 +1942,29 @@ dbus_connection_unref (DBusConnection *connection)
  * dbus_connection_set_dispatch_status_function(), as the disconnect
  * message it generates needs to be dispatched.
  *
+ * If the connection is a shared connection we print out a warning that
+ * you can not close shared connection and we return.  Internal calls
+ * should use _dbus_connection_close_internal() to close shared connections.
+ *
  * @param connection the connection.
  */
 void
 dbus_connection_close (DBusConnection *connection)
 {
-  DBusDispatchStatus status;
-  
   _dbus_return_if_fail (connection != NULL);
   _dbus_return_if_fail (connection->generation == _dbus_current_generation);
 
-  _dbus_verbose ("Disconnecting %p\n", connection);
-  
   CONNECTION_LOCK (connection);
-  
-  _dbus_transport_disconnect (connection->transport);
 
-  _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME);
-  status = _dbus_connection_get_dispatch_status_unlocked (connection);
+  if (connection->shared)
+    {
+      CONNECTION_UNLOCK (connection);
 
-  /* this calls out to user code */
-  _dbus_connection_update_dispatch_status_and_unlock (connection, status);
+      _dbus_warn ("Applications can not close shared connections.  Please fix this in your app.  Ignoring close request and continuing.");
+      return;
+    }
+
+  _dbus_connection_close_internal_and_unlock (connection);
 }
 
 static dbus_bool_t
@@ -3823,7 +3860,7 @@ dbus_connection_dispatch (DBusConnection *connection)
                                   DBUS_INTERFACE_LOCAL,
                                   "Disconnected"))
         {
-          _dbus_bus_check_connection_and_unref (connection);
+          _dbus_bus_check_connection_and_unref_unlocked (connection);
 
           if (connection->exit_on_disconnect)
             {
diff --git a/doc/TODO b/doc/TODO
index 2ef7fdef..d3ece326 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -21,21 +21,6 @@ Important for 1.0
 
  - just before 1.0, try a HAVE_INT64=0 build and be sure it runs
 
- - fix locking on DBusPendingCall
-
- - dbus_connection_open() is like dbus_bus_get() in that it returns a shared
-   connection; it needs the corresponding fix to hold a strong reference to 
-   these connections.
-
- - _dbus_bus_check_connection_and_unref does not do proper locking and 
-   doesn't handle all the shared connections, e.g. DBUS_BUS_STARTER
-
- - for both the dbus-bus.c shared connections and dbus_connection_open() 
-   shared connections, it is probably appropriate to warn() if someone 
-   calls close() on them ; essentially shared connections are not closeable
-   because it's unclear who would do the closing and when, short of 
-   dbus_shutdown()
-
 Important for 1.0 GLib Bindings
 ===
 
diff --git a/test/test-service.c b/test/test-service.c
index 6a633b77..0dbece08 100644
--- a/test/test-service.c
+++ b/test/test-service.c
@@ -115,6 +115,7 @@ check_hello_from_self_reply (DBusPendingCall *pcall,
   dbus_message_unref (reply);
   dbus_message_unref (echo_message);
   dbus_pending_call_unref (pcall);
+  dbus_connection_unref (connection);
 }
 
 static DBusHandlerResult
@@ -242,7 +243,7 @@ path_message_func (DBusConnection  *connection,
                                         "org.freedesktop.TestSuite",
                                         "Exit"))
     {
-      dbus_connection_close (connection);
+      dbus_connection_unref (connection);
       quit ();
       return DBUS_HANDLER_RESULT_HANDLED;
     }
@@ -319,7 +320,6 @@ filter_func (DBusConnection     *connection,
                               DBUS_INTERFACE_LOCAL,
                               "Disconnected"))
     {
-      dbus_connection_close (connection);
       quit ();
       return DBUS_HANDLER_RESULT_HANDLED;
     }
diff --git a/test/test-shell-service.c b/test/test-shell-service.c
index 71b4baae..08ed2077 100644
--- a/test/test-shell-service.c
+++ b/test/test-shell-service.c
@@ -85,7 +85,7 @@ path_message_func (DBusConnection  *connection,
                                         "org.freedesktop.TestSuite",
                                         "Exit"))
     {
-      dbus_connection_close (connection);
+      dbus_connection_unref (connection);
       quit ();
       return DBUS_HANDLER_RESULT_HANDLED;
     }
@@ -109,7 +109,6 @@ filter_func (DBusConnection     *connection,
                               DBUS_INTERFACE_LOCAL,
                               "Disconnected"))
     {
-      dbus_connection_close (connection);
       quit ();
       return DBUS_HANDLER_RESULT_HANDLED;
     }
diff --git a/test/test-utils.c b/test/test-utils.c
index 9665eda3..3577bf9c 100644
--- a/test/test-utils.c
+++ b/test/test-utils.c
@@ -171,6 +171,8 @@ void
 test_connection_shutdown (DBusLoop       *loop,
                           DBusConnection *connection)
 {
+  _dbus_connection_close_internal (connection);
+
   if (!dbus_connection_set_watch_functions (connection,
                                             NULL,
                                             NULL,