GNOME Bugzilla – Bug 746901
gdbus utils and more gdbus porting
Last modified: 2015-04-03 21:03:06 UTC
danw/more-gdbus-bgoXXXXXX contains two things: - Some utility functions in nm-core-internal.h to make up for some of the annoying parts of gdbus. (The idea is that once we're happy with these, we'd propose something like them for inclusion in GLib.) - Porting of all of the remaining stuff in the daemon that can be ported independently. AFAICT, all remaining uses of dbus-glib need to be switched over all at once, because, directly or indirectly, they need to be on the D-Bus connection that is the owner of org.freedesktop.NetworkManager.
>> libnm-core: add _nm_dbus_signal_connect() +#define _nm_dbus_signal_connect(proxy, name, signature, handler, data) \ + _nm_dbus_signal_connect_data (proxy, name, signature, handler, data, NULL, (GConnectFlags) 0); ^^^ no semicolon after macro libnm-core/nm-dbus-utils.c has a trailing whitespace If you pass NULL as signature, you accept every response type. Is that desired behavior? Maybe enforce that the function has "void" return type? >> libnm, core: use _nm_dbus_signal_connect() maybe disconnect the signals in dispose()? -- though not really needed, because quite likely we unref the only reference to priv->proxy... >> libnm-core: add _nm_dbus_proxy_call_sync(), _nm_dbus_proxy_call_finish() + g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT, + "Method returned type '%s', but expected '%s'", localize error text? >> firewall: port nm-firewall-manager to gdbus + priv->proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, should handle failure of creating a proxy. Otherwise LGTM btw, we mostly try to create a DBus proxy, and if that fails we never retry. That means, if you install a DBus service after NM started, you have to restart NM too. Maybe, on SIGHUP, our singletons should try to recreate proxies? Or even retry periodically?
(In reply to Thomas Haller from comment #1) > If you pass NULL as signature, you accept every response type. Is that > desired behavior? Yes, that's documented: > * If @signature is %NULL, then the signal's parameters will be ignored, and > * @c_handler should take only the #GDBusProxy and #gpointer arguments. The idea is that if the signal's parameters aren't useful to you, you can just ignore them. Maybe I should be more explicit about that. (Or maybe I should remove that behavior... I had originally only planned to allow NULL for signals with no arguments, but this extension seemed nice...) > Maybe enforce that the function has "void" return type? The @c_handler function? There's no way we can do that is there? > >> libnm, core: use _nm_dbus_signal_connect() > > maybe disconnect the signals in dispose()? -- though not really needed, > because quite likely we unref the only reference to priv->proxy... That was my theory > >> firewall: port nm-firewall-manager to gdbus > > + priv->proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, > > should handle failure of creating a proxy. I didn't add any error handling where there wasn't any already, but I guess I can do that as a separate commit. > btw, we mostly try to create a DBus proxy, and if that fails we never retry. > That means, if you install a DBus service after NM started, you have to > restart NM too. I don't think so; you should be able to create a GDBusProxy for a service that doesn't exist. It will just have a NULL :g-name-owner. But it should start working automatically if the service is installed later.
(In reply to Dan Winship from comment #2) > (In reply to Thomas Haller from comment #1) > > If you pass NULL as signature, you accept every response type. Is that > > desired behavior? > > Yes, that's documented: > > > * If @signature is %NULL, then the signal's parameters will be ignored, and > > * @c_handler should take only the #GDBusProxy and #gpointer arguments. > > The idea is that if the signal's parameters aren't useful to you, you can > just ignore them. Maybe I should be more explicit about that. (Or maybe I > should remove that behavior... I had originally only planned to allow NULL > for signals with no arguments, but this extension seemed nice...) I agree to keep this (nice) extension. It's documented clearly. > > Maybe enforce that the function has "void" return type? > > The @c_handler function? There's no way we can do that is there? No, I meant, treating NULL as equalt to "()". But I agree, if you want signature checking for "()", you should pass "()". Otherwise, NULL means: accept any. > > >> libnm, core: use _nm_dbus_signal_connect() > > > > maybe disconnect the signals in dispose()? -- though not really needed, > > because quite likely we unref the only reference to priv->proxy... > > That was my theory ACK > > >> firewall: port nm-firewall-manager to gdbus > > > > + priv->proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, > > > > should handle failure of creating a proxy. > > I didn't add any error handling where there wasn't any already, but I guess > I can do that as a separate commit. That would be good > > btw, we mostly try to create a DBus proxy, and if that fails we never retry. > > That means, if you install a DBus service after NM started, you have to > > restart NM too. > > I don't think so; you should be able to create a GDBusProxy for a service > that doesn't exist. It will just have a NULL :g-name-owner. But it should > start working automatically if the service is installed later. I tested it, if I uninstall firewalld, the proxy is successfully created and it works as you describe. However when I remove /usr/lib/systemd/system/polkit.service, stop the service and do `systemctl daemon-reload`, the creation of the proxy fails with: Error calling StartServiceByName for org.freedesktop.PolicyKit1: GDBus.Error:org.freedesktop.systemd1.LoadFailed: Unit polkit.service failed to load: No such file or directory. So, it seems acceptable attempting to create the proxy only once. But we should anticipate failures, if something is seriously wrong.
> libnm, core: use _nm_dbus_signal_connect() What about signal disconnection? Is there a chance that some proxy will still be alive after the managing object has destroyed itself but the signal still fires? I dont' think that's a huge possibility, but ISTR some issue during porting the supplciant or VPN where that was the case, which might have inspired the signal disconnection in the supplicant code. > firewall: port nm-firewall-manager to gdbus Do we need to make sure the cancellable is canceled before we unref it in _cb_info_free()? Also don't we need to check cancellation in eg add_or_change_cb() and then not call info->callback? Or does info->completed handle that... Maybe use gs_free char *owner = NULL in name_owner_changed and *_init too? I found the gsystem local alloc stuff pretty useful when porting the supplicant and VPN. > core: port NMManager's aipd proxy to GDBus g_connection is no longer used in nm_manager_init().
Also, won't the VPN error patch obsolete https://bugzilla.gnome.org/show_bug.cgi?id=745997 since GDBus will now know about the old-style mapping?
One more thing I noticed about cancellation, now since cancellation returns a GError from g_dbus_proxy_call_finish(), a number of places will now log that error instead of ignoring it and just freeing the callback info. Like firewall's add_or_change_cb() or remove_cb(). I think those are the only ones where you added a cancellable though.
> libnm, core: use typechecked proxy_call methods The '@' in the GVariant type string near nm-bluez-device.c:885 needs to be removed, otherwise it's not a valid type string and GLib yells.
I remember why I added the signal disconnect on the proxy in the supplicant, which was because I got suspend/resume crashes. And I'm getting them with this branch too; there's an idle-initiated properties changed callback for the SupplicantInterface on resume, but of course we've already thrown it away already... For me a simple suspend/resume will trigger the crash.
> > libnm, core: use _nm_dbus_signal_connect() > > What about signal disconnection? Is there a chance that some proxy will > still be alive after the managing object has destroyed itself but the signal > still fires? Yeah, ok, I rewrote _nm_dbus_signal_connect() so that g_signal_handlers_disconnect_by_data() will work (though disconnect_by_func() still won't), and then added a fixup to restore the removed disconnects. > > firewall: port nm-firewall-manager to gdbus > > Do we need to make sure the cancellable is canceled before we unref it in > _cb_info_free()? Why? At that point nothing should be paying attention to the cancellable's state anyway. > Also don't we need to check cancellation in eg > add_or_change_cb() and then not call info->callback? Or does > info->completed handle that... nm_firewall_manager_cancel_call() sets info->callback to NULL. I could do this differently if you think that's non-obvious. (In reply to Dan Williams from comment #5) > Also, won't the VPN error patch obsolete > https://bugzilla.gnome.org/show_bug.cgi?id=745997 since GDBus will now know > about the old-style mapping? (dropped that commit; it should be handled as part of 745997) (In reply to Dan Williams from comment #6) > One more thing I noticed about cancellation, now since cancellation returns > a GError from g_dbus_proxy_call_finish(), a number of places will now log > that error instead of ignoring it and just freeing the callback info. Like > firewall's add_or_change_cb() or remove_cb(). I think those are the only > ones where you added a cancellable though. They both check for G_IO_ERROR_CANCELLED, and log a debug message rather than a warning in that case. repushed. everything should be fixed except for the additional error checking for proxy creation failing, which I think is a general problem we should handle later.
Looks good to me now, and I pushed a commit that cleans up the firewall code to just use the GCancellable instead of hand-rolling it with 'completed'. Does that look OK?
pushed with your firewall cleanup