GNOME Bugzilla – Bug 688238
Integrate the new ModemManager1 interface
Last modified: 2013-05-10 06:15:30 UTC
ModemManager >= 0.7.x will come with a new 'ModemManager1' DBus interface, and a helper libmm-glib library which makes it easier to use the new interface from GLib-based applications. gnome-control-center should try to play well with modems exposed by either the old or the new ModemManager interface. The path of the modem object in DBus, exposed as 'udi' in the NMDevice, helps to decide which interface to use.
Added dependency on the NM integration bug.
Created attachment 228872 [details] [review] Patch with the implementation. The attached patch applies on top of the ones for bugs 688211 and 688212. Sorry for this, but I thought it just made more sense to first fix the issues I found and then implement the new stuff also with those issues fixed in the new implementation. The patch introduces a new configure-time check for libmm-glib, which will be distributed by ModemManager. If libmm-glib is found, the control center is compiled supporting both the old and new interfaces. If libmm-glib is not found, only the old interface support is included. Whenever (if ever) NetworkManager decides to stop supporting the old ModemManager interface, this implementation will be simplified quite a lot.
Seems I left an old chunk of text in the commit log after rebasing the stuff... please forget about the new configure switch mentioned, there's no such thing in the patch; will fix it when the patch gets reviewed.
Comment on attachment 228872 [details] [review] Patch with the implementation. >Subject: [PATCH] network: include support for the `ModemManager1' interface ew. don't use GNU-style dumb quotes. 'ModemManager1' Also, the comment should explain why we care about the new interfaces. >+ AC_MSG_WARN(*** Network panel will not be built with ModemManager1 support (libmm-glib not found) ***) Not sure if we actually want to make this optional or just add libmm-glib to the required libraries for the network panel... Bastien? >+ if (!priv->modem_manager) { >+ g_warning ("Cannot grab information for modem at %s: No ModemManager support", >+ nm_device_get_udi (device)); >+ goto out; Can't you just fall back to doing everything the old way, rather than bailing out completely? >+ panel->priv->modem_manager = (mm_manager_new_sync ( >+ system_bus, The "(" before mm_manager_new_sync is unnecessary, and the indentation here is weird... control-center seems to be happy with long lines, so just keep system_bus on the first line and line this all up normally. Also, you need to unref system_bus afterward.
(In reply to comment #4) > >+ AC_MSG_WARN(*** Network panel will not be built with ModemManager1 support (libmm-glib not found) ***) > > Not sure if we actually want to make this optional or just add libmm-glib to > the required libraries for the network panel... Bastien? The latter. Otherwise we'll have questions about why the shell can see it, and not the settings, etc.
(In reply to comment #4) > (From update of attachment 228872 [details] [review]) > >Subject: [PATCH] network: include support for the `ModemManager1' interface > > ew. don't use GNU-style dumb quotes. 'ModemManager1' > > Also, the comment should explain why we care about the new interfaces. > Sure, will change that. > > >+ if (!priv->modem_manager) { > >+ g_warning ("Cannot grab information for modem at %s: No ModemManager support", > >+ nm_device_get_udi (device)); > >+ goto out; > > Can't you just fall back to doing everything the old way, rather than bailing > out completely? > Not in this case. We bail out completely here because the path that we got from NM is the MM1-specific path, e.g. /org/freedesktop/ModemManager1/Modem. If we had a path like that, but priv->modem_manager (the one providing the MM1 interface) is not available, then we can hardly do anything. This is really a corner case that may only happen if in the weird case where NM exposes a MM1 object while MM1 disappeared from the bus, not really likely to happen. > >+ panel->priv->modem_manager = (mm_manager_new_sync ( > >+ system_bus, > > The "(" before mm_manager_new_sync is unnecessary, and the indentation here is > weird... control-center seems to be happy with long lines, so just keep > system_bus on the first line and line this all up normally. > Sure. > Also, you need to unref system_bus afterward. Oops, yes, will change that.
(In reply to comment #5) > (In reply to comment #4) > > >+ AC_MSG_WARN(*** Network panel will not be built with ModemManager1 support (libmm-glib not found) ***) > > > > Not sure if we actually want to make this optional or just add libmm-glib to > > the required libraries for the network panel... Bastien? > > The latter. Otherwise we'll have questions about why the shell can see it, and > not the settings, etc. If that's the case I'll also remove all the #ifdefs here and there. But then jhbuild would also need to be aware of the ModemManager repository...
(In reply to comment #7) > If that's the case I'll also remove all the #ifdefs here and there. But then > jhbuild would also need to be aware of the ModemManager repository... We can do that when we land the changes.
Created attachment 235156 [details] [review] Updated patch
Comment on attachment 235156 [details] [review] Updated patch looks good to me. presumably we want to wait until after the 3.7.5 release today to commit this...
(In reply to comment #10) > (From update of attachment 235156 [details] [review]) > looks good to me. presumably we want to wait until after the 3.7.5 release > today to commit this... Yes, I guess so.
Review of attachment 235156 [details] [review]: ::: panels/network/cc-network-panel.c @@ +1372,3 @@ + /* Setup ModemManager client */ + system_bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &error); + if (!system_bus) { Do away with error checking if things will fail badly when you run mm_manager_new_sync() anyway. ::: panels/network/net-device-mobile.c @@ +249,3 @@ + + /* Set equipment ID */ + if (equipment_id) braces around multi-line statements. @@ +653,3 @@ + priv->mm_object = g_value_dup_object (value); + + if (priv->mm_object) { Do all this in a net_device_mobile_set_modem_object() function instead.
(In reply to comment #13) > Review of attachment 235156 [details] [review]: > > ::: panels/network/cc-network-panel.c > @@ +1372,3 @@ > + /* Setup ModemManager client */ > + system_bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &error); > + if (!system_bus) { > > Do away with error checking if things will fail badly when you run > mm_manager_new_sync() anyway. > We shouldn't really pass a NULL system_bus to mm_manager_new_sync(); so I really think we need to check that and if getting the system bus fails, just skip trying to create the MMManager (i.e. only run mm_manager_new_sync() in an else{}, which was actually missing in the patch)... what do you think?
(In reply to comment #14) > We shouldn't really pass a NULL system_bus to mm_manager_new_sync(); so I > really think we need to check that and if getting the system bus fails, just > skip trying to create the MMManager (i.e. only run mm_manager_new_sync() in an > else{}, which was actually missing in the patch)... what do you think? That's fine.
Created attachment 235770 [details] [review] Updated patch
Review of attachment 235770 [details] [review]: Looks good other than that. ::: panels/network/net-device-mobile.c @@ +644,3 @@ + modem_3gpp = mm_object_peek_modem_3gpp (self->priv->mm_object); + if (modem_3gpp != NULL) { + self->priv->operator_name_updated = g_signal_connect (modem_3gpp, It looks possible for ->operator_name_updated to be set multiple times. If not, either add an assertion, or an early exit clause, as necessary. @@ +699,3 @@ g_clear_object (&priv->gsm_proxy); + if (priv->operator_name_updated) { if (priv->operator_name_updated && priv->mm_object)?
(In reply to comment #17) > Review of attachment 235770 [details] [review]: > > Looks good other than that. > > ::: panels/network/net-device-mobile.c > @@ +644,3 @@ > + modem_3gpp = mm_object_peek_modem_3gpp (self->priv->mm_object); > + if (modem_3gpp != NULL) { > + self->priv->operator_name_updated = g_signal_connect > (modem_3gpp, > > It looks possible for ->operator_name_updated to be set multiple times. If not, > either add an assertion, or an early exit clause, as necessary. > I'll add an assert; the signal is really only supposed to be added once. > @@ +699,3 @@ > g_clear_object (&priv->gsm_proxy); > > + if (priv->operator_name_updated) { > > if (priv->operator_name_updated && priv->mm_object)? Same here; as 'operator_name_updated' is not supposed to get updated, it will never be the case that we have a valid 'operator_name_updated' with a NULL mm_object. Will also add an assert.
This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.
Created attachment 235937 [details] [review] network: make ModemManager dependency optional There probably won't be a stable ModemManager 0.7 release before GNOME 3.8, so make support for it optional (Mostly based on Aleksander's original patch.)
Review of attachment 235937 [details] [review]: ::: panels/network/cc-network-panel.c @@ +55,2 @@ #include <libmm-glib.h> +#endif Add /* HAVE_MM_GLIB */ on the same line as the #endif for all the calls. ::: panels/network/net-device-mobile.c @@ +57,2 @@ /* New MM >= 0.7 support */ MMObject *mm_object; GObject, and you can define this even if mm-glib isn't available. @@ +69,3 @@ }; +#ifdef HAVE_MM_GLIB I don't think it's necessary to make the properties a compile-time option.
Created attachment 235940 [details] [review] network: make ModemManager dependency optional ===== updated for comments; I went with gpointer rather than GObject, to avoid have to add casts elsewhere
Attachment 235940 [details] pushed as 4ef8ae4 - network: make ModemManager dependency optional
(In reply to comment #23) > Attachment 235940 [details] pushed as 4ef8ae4 - network: make ModemManager dependency > optional This makes modemmanager dependency automagic: http://www.gentoo.org/proj/en/qa/automagic.xml it should, instead, allow to control that dependency with a configure switch. The problem is that I don't know what are the exact plans on this option, will a hard requirement be re-added once modemmanager-0.8 is released? Also, do you have any idea about why 0.8 release is taking so long? Is it safe to use 0.7.990? (that is the version currently used in OpenSuSE) Thanks
> it should, instead, allow to control that dependency with a configure switch. > The problem is that I don't know what are the exact plans on this option, will > a hard requirement be re-added once modemmanager-0.8 is released? Also, do you > have any idea about why 0.8 release is taking so long? Is it safe to use > 0.7.990? (that is the version currently used in OpenSuSE) > MM 0.8 will come with a new DBus API, making the 0.6.x one obsolete. We just want to make sure that the API is good enough before making the new release (which will be available pretty soon probably).