After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 688238 - Integrate the new ModemManager1 interface
Integrate the new ModemManager1 interface
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Network
3.6.x
Other Linux
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on: 688221
Blocks:
 
 
Reported: 2012-11-13 11:24 UTC by Aleksander Morgado
Modified: 2013-05-10 06:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch with the implementation. (18.76 KB, patch)
2012-11-13 11:31 UTC, Aleksander Morgado
reviewed Details | Review
Updated patch (15.76 KB, patch)
2013-02-04 16:15 UTC, Aleksander Morgado
reviewed Details | Review
Updated patch (18.31 KB, patch)
2013-02-12 09:42 UTC, Aleksander Morgado
committed Details | Review
network: make ModemManager dependency optional (11.28 KB, patch)
2013-02-13 18:03 UTC, Dan Winship
needs-work Details | Review
network: make ModemManager dependency optional (11.11 KB, patch)
2013-02-13 18:27 UTC, Dan Winship
committed Details | Review

Description Aleksander Morgado 2012-11-13 11:24:51 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.
Comment 1 Aleksander Morgado 2012-11-13 11:25:16 UTC
Added dependency on the NM integration bug.
Comment 2 Aleksander Morgado 2012-11-13 11:31:08 UTC
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.
Comment 3 Aleksander Morgado 2012-11-14 09:11:13 UTC
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 4 Dan Winship 2013-01-31 22:31:27 UTC
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.
Comment 5 Bastien Nocera 2013-01-31 22:38:32 UTC
(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.
Comment 6 Aleksander Morgado 2013-02-01 09:35:35 UTC
(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.
Comment 7 Aleksander Morgado 2013-02-01 09:38:00 UTC
(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...
Comment 8 Bastien Nocera 2013-02-01 14:23:47 UTC
(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.
Comment 9 Aleksander Morgado 2013-02-04 16:15:43 UTC
Created attachment 235156 [details] [review]
Updated patch
Comment 10 Dan Winship 2013-02-04 23:01:14 UTC
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...
Comment 11 Aleksander Morgado 2013-02-05 08:44:24 UTC
(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.
Comment 12 Bastien Nocera 2013-02-11 15:02:14 UTC
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.
Comment 13 Bastien Nocera 2013-02-11 15:22:48 UTC
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.
Comment 14 Aleksander Morgado 2013-02-12 08:38:23 UTC
(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?
Comment 15 Bastien Nocera 2013-02-12 08:45:53 UTC
(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.
Comment 16 Aleksander Morgado 2013-02-12 09:42:08 UTC
Created attachment 235770 [details] [review]
Updated patch
Comment 17 Bastien Nocera 2013-02-12 15:27:15 UTC
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)?
Comment 18 Aleksander Morgado 2013-02-12 16:12:56 UTC
(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.
Comment 19 Aleksander Morgado 2013-02-12 16:44:13 UTC
This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.
Comment 20 Dan Winship 2013-02-13 18:03:32 UTC
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.)
Comment 21 Bastien Nocera 2013-02-13 18:09:21 UTC
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.
Comment 22 Dan Winship 2013-02-13 18:27:02 UTC
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
Comment 23 Dan Winship 2013-02-19 18:49:24 UTC
Attachment 235940 [details] pushed as 4ef8ae4 - network: make ModemManager dependency optional
Comment 24 Pacho Ramos 2013-05-09 19:50:47 UTC
(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
Comment 25 Aleksander Morgado 2013-05-10 06:15:30 UTC
> 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).