GNOME Bugzilla – Bug 709684
Two fixes for rfkill
Last modified: 2013-10-21 20:36:23 UTC
Re USB modems and HW killswitches Note: there should be an rfkill component
Created attachment 256762 [details] [review] rfkill: watch and set NetworkManager wwan-enabled USB modems don't expose an rfkill device at the kernel layer, so we need to go through NetworkManager (which in turn goes through ModemManager) to disable them.
Created attachment 256763 [details] [review] rfkill: expose in the API if the rfkill is blocked in hardware Hardware-blocked rfkills can't be toggled, so they should cause a different "Airplane mode" UI in the shell and control center (one that cannot be toggled)
Note: one side effect of patch 1 is that there are always at least two killswitches (one kernel one and one from NM/MM), so blocking one is never enough to trigger airplane mode, which fixes the shell showing airplane mode when wifi is off.
Review of attachment 256763 [details] [review]: ::: plugins/rfkill/gsd-rfkill-manager.c @@ +63,3 @@ " <annotation name='org.freedesktop.DBus.GLib.CSymbol' value='gsd_rfkill_manager'/>" " <property name='AirplaneMode' type='b' access='readwrite'/>" +" <property name='HwAirplaneMode' type='b' access='read'/>" We can spell it out. So: HardwareAirplaneMode @@ +140,3 @@ + state = GPOINTER_TO_INT (value); + + /* A single rfkill switch that's hw blocked? Hw airplane mode is on */ That's incorrect. We'd need all the killswitches to be hardware blocked for this to work, otherwise a Wi-Fi hard switch would turn on the hw airplane mode, when eg. Bluetooth is not blocked. @@ +191,3 @@ { GList *l; + int value; Use the actual enum name here.
Review of attachment 256762 [details] [review]: The idea is good, but we really need to know whether or not there are modems. If there are no modems, it doesn't matter what the state of wwan-enabled is.
*** Bug 709634 has been marked as a duplicate of this bug. ***
(In reply to comment #4) > Review of attachment 256763 [details] [review]: > > @@ +191,3 @@ > { > GList *l; > + int value; > > Use the actual enum name here. There is no enum name, it's #defines in the kernel headers.
Created attachment 257132 [details] [review] rfkill: watch and set NetworkManager wwan-enabled USB modems don't expose an rfkill device at the kernel layer, so we need to go through NetworkManager (which in turn goes through ModemManager) to disable them.
Created attachment 257133 [details] [review] rfkill: expose in the API if the rfkill is blocked in hardware Hardware-blocked rfkills can't be toggled, so they should cause a different "Airplane mode" UI in the shell and control center (one that cannot be toggled)
Review of attachment 257132 [details] [review]: ::: plugins/rfkill/gsd-rfkill-manager.c @@ +228,3 @@ + G_DBUS_CALL_FLAGS_NONE, + -1, /* timeout */ + NULL, NULL, NULL); print possible errors would be very useful. @@ +443,3 @@ + + /* Can only happen for two rapid start-stop cycles (because we don't + pass a GCancellable) */ Why not? Seems like both calls in start() should be using cancellables. @@ +477,3 @@ + "org.freedesktop.NetworkManager", + "/org/freedesktop/NetworkManager", + "org.freedesktop.NetworkManager", Are you listening on the correct interface name to receive the PropertiesChanged signal?
Review of attachment 257133 [details] [review]: Looks good. ::: plugins/rfkill/gsd-rfkill-manager.c @@ +143,3 @@ + + /* A single rfkill switch that's not hw blocked? Hw airplane mode is off */ + if (state != RFKILL_STATE_HARD_BLOCKED) { No need for the braces for a one-line conditional.
(In reply to comment #10) > Review of attachment 257132 [details] [review]: > > @@ +477,3 @@ > + "org.freedesktop.NetworkManager", > + "/org/freedesktop/NetworkManager", > + "org.freedesktop.NetworkManager", > > Are you listening on the correct interface name to receive the > PropertiesChanged signal? Yes, it's a custom signal, not the usual one.
Created attachment 257791 [details] [review] rfkill: watch and set NetworkManager wwan-enabled USB modems don't expose an rfkill device at the kernel layer, so we need to go through NetworkManager (which in turn goes through ModemManager) to disable them.
Review of attachment 257791 [details] [review]: Looks good otherwise, thanks for the iterations :) ::: plugins/rfkill/gsd-rfkill-manager.c @@ +128,3 @@ engine_get_has_airplane_mode (GsdRfkillManager *manager) { + /* FIXME: this does not account for USB modems (they don't get I'm guessing this is fixed now. @@ +217,3 @@ + variant = g_dbus_proxy_call_finish (G_DBUS_PROXY (object), result, &error); + + if (error != NULL) { if (variant == NULL) { @@ +218,3 @@ + + if (error != NULL) { + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { Remove the braces. @@ +458,3 @@ + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED) && + !g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN)) { + g_warning ("Failed to acquire NetworkManager proxy: %s", error->message); ModemManager? @@ +539,3 @@ + if (p->nm_cancellable) { + g_cancellable_cancel (p->nm_cancellable); + g_object_unref (p->nm_cancellable); I prefer g_clear_object() these days.
Attachment 257133 [details] pushed as bc47431 - rfkill: expose in the API if the rfkill is blocked in hardware Attachment 257791 [details] pushed as d74b6de - rfkill: watch and set NetworkManager wwan-enabled