> core/dbus: debug log unix file descriptor when closing private dbus connection

>+		nm_log_dbg (LOGD_CORE, "(%s) closed connection %p on private socket (fd %"G_GINT64_FORMAT").",
>+		            s->tag, conn, dbus_connection_get_unix_fd (conn, &fd) ? fd : G_MININT64);

"fd" is an int, so there's no need to print it as an int64. Also, I'd use -1 on error.

> libnm: add NMObject:dbus-connection property to inject DBUS connection

If we're going to have the property again, it might as well be READWRITE.

> libnm: share private DBUS connection
> ...
> We already pass the existing "parent" DBUS connection when creating
> the proxy objects. However, when creating two independent objects
> (e.g. nm_client_new() and nm_remote_settings_new()), their private
> DBUS connections were not shared.

FWIW, NMClient and NMRemoteSettings will soon be merged into a single object, which will take care of making sure that we use the same connection for both proxies. So this won't actually be needed for that (and having 2 connections rather than 1 is not a huge problem anyway).

So maybe drop this?

If not, the code looks correct, other than this comment:

>+	 * If getting it once fails, we don't try again (get_private_failed). */

(In reply to comment #1)
> FWIW, NMClient and NMRemoteSettings will soon be merged into a single object,
> which will take care of making sure that we use the same connection for both
> proxies. So this won't actually be needed for that (and having 2 connections
> rather than 1 is not a huge problem anyway).
> 
> So maybe drop this?
> 
> If not, the code looks correct, other than this comment:

I addressed your comments and repushed the branch.


I did not (yet) drop the commit. I think, this "internalization" of private connections is anyway the right thing to do. I would not drop it...

What's the g_idle_add() for?  Wouldn't it be simpler to g_simple_async_result_complete_in_idle(), eg:

p = g_weak_ref_get (&private_connection.weak_ref);
if (p) {
	g_simple_async_result_set_op_res_gpointer (simple, p, g_object_unref);
	g_simple_async_result_complete_in_idle (simple);
	g_object_unref (simple);
} else {
	g_dbus_connection_new_for_address ("unix:path=" NMRUNDIR "/private",
	                                   G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT,
	                                   NULL,
	                                   cancellable,
	                                   new_connection_async_got_private, simple);
}

In _private_dbus_connection_internalize() the mutex isn't released if 'p != NULL'.

Also, why not put the dbus connection 'closed' logic into NMObject?  NMObject should know when the connection is closed becuase that'll affect whether it can do anything with NM.  Maybe NMClient should even just clean up + reinitialize when that happens, which means dropping all refs to the connection anyway.  Then nm-dbus-helpers.c can just watch for the last reference drop, and when that happens, clear out it's internal pointer/weak_ref.

(yes, nm-secret-agent.c uses nm-dbus-helpers.c too, but it should *also* have it's own closed tracking including re-registration when the new connection is acquired)

I guess it just seems more natural to me to have the closed handling in NMObject/NMSecretAgent.

(In reply to comment #3)
> What's the g_idle_add() for?  Wouldn't it be simpler to
> g_simple_async_result_complete_in_idle(), eg:
> 
> p = g_weak_ref_get (&private_connection.weak_ref);
> if (p) {
>     g_simple_async_result_set_op_res_gpointer (simple, p, g_object_unref);
>     g_simple_async_result_complete_in_idle (simple);
>     g_object_unref (simple);
> } else {
>     g_dbus_connection_new_for_address ("unix:path=" NMRUNDIR "/private",
>                                       
> G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT,
>                                        NULL,
>                                        cancellable,
>                                        new_connection_async_got_private,
> simple);
> }

The original reason was that I wasn't aware of g_simple_async_result_complete_in_idle() :$

But a better reason is that _nm_dbus_new_connection() should never return a connection that is already closed. With your suggestion there is a race and the connection could be closed in the meantime. That would  be different behaviour then plain g_bus_get_sync() / g_dbus_connection_new_for_address_sync().



> In _private_dbus_connection_internalize() the mutex isn't released if 'p !=
> NULL'.

Fixed.


> Also, why not put the dbus connection 'closed' logic into NMObject?  NMObject
> should know when the connection is closed becuase that'll affect whether it can
> do anything with NM.  Maybe NMClient should even just clean up + reinitialize
> when that happens, which means dropping all refs to the connection anyway. 
> Then nm-dbus-helpers.c can just watch for the last reference drop, and when
> that happens, clear out it's internal pointer/weak_ref.
> 
> (yes, nm-secret-agent.c uses nm-dbus-helpers.c too, but it should *also* have
> it's own closed tracking including re-registration when the new connection is
> acquired)
> 
> I guess it just seems more natural to me to have the closed handling in
> NMObject/NMSecretAgent.

I think any user of the connection should have it's own 'closed' logic as needed (and it would certainly make NMObject more robust). But as stated above, nm-dbus-helper needs such logic too to avoid returning already closed connections.


pushed fixup!.

Your points are correct.  I'm OK with the current iteration of the branch.

merged as:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=d4cd8b578596e7698c48eb6be02bcbfd678256b0