Ubuntu xenial is getting reports of unity-settings-daemon segfaulting [1], the issue can be simulated by that simple testcase used with upowerd not started UpClient *client = NULL; client = up_client_new (); printf("lid is closed? %d\n", up_client_get_lid_is_closed (client)); I moved the binary away for testing but reporters seems to have kernel issues making the upower start slow/delayed The backtrace of that example is 0xb7f797ed in up_exported_daemon_get_lid_is_closed (object=0x0) at up-daemon-generated.c:747 0xb7f7417d in up_client_get_lid_is_closed (client=0x8053f08 [UpClient]) at up-client.c:208 0x080485f6 in main (argc=1, argv=0xbfffef34) at upower.c:8 [1] https://bugs.launchpad.net/bugs/1546641
Confirmed, this can be trivially reproduced with DBUS_SYSTEM_BUS_ADDRESS=foo upower --dump or running tools/upower in a chroot: 288 client = up_client_new (); (gdb) (upower:23099): libupower-glib-WARNING **: Couldn't connect to proxy: Could not connect: No such file or directory 290 if (opt_version) { (gdb) p client $1 = (UpClient *) 0x612a20 So up_client_new() fails but indeed returns something which is valid enough to make UP_IS_CLIENT() succeed. With client = up_client_new (); + g_print("XXX client: %p UP_IS_CLIENT: %i\n", client, UP_IS_CLIENT(client)); I get XXX client: 0x24d3a20 UP_IS_CLIENT: 1 which forfeits all the argument checks in up-client.c: g_return_val_if_fail (UP_IS_CLIENT (client), FALSE); However, please note that this is not a recent regression: I tested this down to 0.9.23 (in Ubuntu 14.04 LTS), exactly the same behaviour (i. e. segfault when upowerd is not running). So I do agree that it's worth fixing this, but this isn't the root cause for the referenced Launchpad bug.
I'm still totally unsure *how* to fix this and what the desirable behaviour would be: * Right now you always get a valid UpClient() object from up_client_new() that just causes assertions/segfaults left and right when you try to use it while upower is not (yet) running as priv->proxy is NULL. Which means that once upower actually does finish starting, it will still not begin to work as we don't re-connect to the dbus proxy. * We could change up_client_new() to return NULL instead of a valid (but useless) UpClient object, with something like --- a/libupower-glib/up-client.c +++ b/libupower-glib/up-client.c @@ -497,6 +497,13 @@ up_client_new (void) g_object_ref (up_client_object); } else { up_client_object = g_object_new (UP_TYPE_CLIENT, NULL); + if (UP_CLIENT(up_client_object)->priv->proxy == NULL) { + g_warning("XX up_client_new: new client_object proxy is NULL, failing"); + g_object_unref(up_client_object); + up_client_object = NULL; + return NULL; + } + g_warning("XX up_client_new: allocated new client_object %p", up_client_object); g_object_add_weak_pointer (up_client_object, &up_client_object); } return UP_CLIENT (up_client_object); This will match the argument checks like in up_client_get_devices(): g_return_val_if_fail (UP_IS_CLIENT (client), NULL); (Note that this will then crash in up-tool.c as device iteration does not expect a NULL return value, but that's easily fixed). * We could change the code to block until the dbus proxy is actually ready. But with a timeout client code still needs to handle failures, and without a timeout this would block forever. Neither are particularly attractive. So I think returning NULL from up_client_new() if connecting to upowerd fails would be the best solution for now. Opinions?
Created attachment 123831 [details] [review] lib: Make up_client_new() return NULL when connecting to daemon fails This implements the proposed behaviour change.
Created attachment 123832 [details] [review] up-tool: Exit early when connecting to upower fails This makes the behaviour of the "upower" tool less confusing if upowerd is not running: Instead of spitting out tons of assertions, showing some default values and exiting with 0 it now just says that it cannot connect to the daemon and exits with 1.
Space before the bracket, but looks fine for me. THanks.
Comment on attachment 123831 [details] [review] lib: Make up_client_new() return NULL when connecting to daemon fails Review of attachment 123831 [details] [review]: ----------------------------------------------------------------- I'm afraid this isn't good enough. What you want to do in this case is to implement GInitable. Remove the call to _new_for_bus_sync() from _init (you should never do blocking IO in GObject _init functions) and put it in the initable _init implementation instead. Take advantage of Cancellables and GError (consider providing a new API to the user). Consider implementing the async version of this as well, via GAsyncInitable. You can look at the generated code for the proxies to see how this is done. Your implementation should be pretty trivial...
Some remarks from desrt while reviewing this: desrt | but GObject::init should never ever block desrt | base GObject has some simple rules: desrt | 1) no blocking desrt | 2) always successful desrt | 3) always a new pointer desrt | 4) your _new function should do nothing except call g_object_new() and return its value desrt | anyway... i'd do 1) a patch to convert this to an initable, moving the proxy_new from the _init to the initable_init, and also adding a _full() call, and turning _new() into a call to _full(NULL, NULL); desrt | then separately 2) a patch to remove the singleton aspect desrt | and maybe 3) a patch to add asyncinitable too This is actually orthogonoal to changing the behaviour of _new(), but done "more correctly", as normal GObjects should not return NULL from constructor (but currnet libupower-glib behaviour is just as broken, and we want to retain _new() for current API users).
Comment on attachment 123832 [details] [review] up-tool: Exit early when connecting to upower fails Review of attachment 123832 [details] [review]: ----------------------------------------------------------------- ::: tools/up-tool.c @@ +286,4 @@ > > loop = g_main_loop_new (NULL, FALSE); > client = up_client_new (); > + if (!UP_IS_CLIENT(client)) { Why not just check for NULL?
Created attachment 123866 [details] [review] lib: Add proper error and cancellable handling to UpClient constructor I reworked the patches to implement Allison's suggestions for moving to GInitable. This is indeed much nicer/cleaner now, thanks! For existing clients the net effect is the same as in the original patch, i. e. when using the old up_client_new() constructor. But up_client_new_full() is now more flexible and provides proper error handling. This still keeps the singleton behaviour, I'll change/test that in a separate patch. I also didn't add async interfaces -- I propose we only do that when someone actually asks for this (which will likely never happen).
Created attachment 123867 [details] [review] up-tool: Exit early when connecting to upower fails This changes the up-tool patch to use the new constructor. This changes (upower:22910): libupower-glib-WARNING **: Couldn't connect to proxy: Could not connect: No such file or directory (upower:22910): UPower-WARNING **: Cannot connect to upowerd (first warning from _new(), second from up-tool) to a single (upower:25648): UPower-WARNING **: Cannot connect to upowerd: Could not connect: No such file or directory which is cleaner.
Created attachment 123872 [details] [review] lib: Remove hidden singleton instance This removes the UpClient singleton instance handling, as recommended by Allison. I'm not really sure what would break with the current code, and all libupower clients that I know only use one UpClient instance anyway. But if nothing else it makes the code a bit simpler.
Comment on attachment 123866 [details] [review] lib: Add proper error and cancellable handling to UpClient constructor Review of attachment 123866 [details] [review]: ----------------------------------------------------------------- This looks pretty much exactly correct. ::: libupower-glib/up-client.c @@ +535,5 @@ > +{ > + GError *error = NULL; > + UpClient *client; > + client = up_client_new_full (NULL, &error); > + if (error) { usually one checks the return value here rather than the error pointer itself. the effect is always the same, so this is really 100% style. @@ +536,5 @@ > + GError *error = NULL; > + UpClient *client; > + client = up_client_new_full (NULL, &error); > + if (error) { > + g_warning ("Couldn't connect to proxy: %s", error->message); I consider it to be a bit odd that you give g_warning() on a path that you consider to be a normal failure for the other function. g_warning() is definitely the right choice if you want to do this [vs. g_critical, for example]. but I'd mention that difference in the reference documentation of each function at least...
Comment on attachment 123872 [details] [review] lib: Remove hidden singleton instance Review of attachment 123872 [details] [review]: ----------------------------------------------------------------- It's also worth mentioning that this change very likely makes the library safe for use from multiple threads (as long as each thread gets their own client object). ::: libupower-glib/up-client.c @@ +510,4 @@ > UpClient * > up_client_new_full (GCancellable *cancellable, GError **error) > { > + return UP_CLIENT (g_initable_new (UP_TYPE_CLIENT, cancellable, error, NULL)); Drop the cast.
Created attachment 123878 [details] [review] lib: Add proper error and cancellable handling to UpClient constructor Adjusted the error check, improved the method documentation.
Created attachment 123879 [details] [review] lib: Remove hidden singleton instance Dropped the cast.
I pushed the patches now, Richard ack'ed on IRC.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.