[Merge] lp:~phablet-team/network-manager/lp1418077 into lp:~network-manager/network-manager/ubuntu
Tony Espy
espy at canonical.com
Tue Apr 7 22:35:36 UTC 2015
Yes, Dan and I discussed, and we iterated over this approach till it worked.
That said, my only concern is whether or not this approach has any negative impact on the modem-manager behavior.
I've responded to the comments inline.
Diff comments:
> === modified file 'debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch'
> --- debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch 2015-03-19 20:47:38 +0000
> +++ debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch 2015-04-06 21:01:06 +0000
> @@ -15,10 +15,10 @@
> create mode 100644 src/devices/wwan/nm-modem-ofono.c
> create mode 100644 src/devices/wwan/nm-modem-ofono.h
>
> -Index: b/configure.ac
> +Index: network-manager-0.9.10.0/configure.ac
> ===================================================================
> ---- a/configure.ac
> -+++ b/configure.ac
> +--- network-manager-0.9.10.0.orig/configure.ac
> ++++ network-manager-0.9.10.0/configure.ac
> @@ -593,6 +593,15 @@ else
> fi
> AM_CONDITIONAL(WITH_MODEM_MANAGER_1, test "${with_modem_manager_1}" = "yes")
> @@ -35,10 +35,10 @@
> # DHCP client support
> AC_ARG_WITH([dhclient], AS_HELP_STRING([--with-dhclient=yes|no|path], [Enable dhclient 4.x support]))
> AC_ARG_WITH([dhcpcd], AS_HELP_STRING([--with-dhcpcd=yes|no|path], [Enable dhcpcd 4.x support]))
> -Index: b/src/devices/wwan/Makefile.am
> +Index: network-manager-0.9.10.0/src/devices/wwan/Makefile.am
> ===================================================================
> ---- a/src/devices/wwan/Makefile.am
> -+++ b/src/devices/wwan/Makefile.am
> +--- network-manager-0.9.10.0.orig/src/devices/wwan/Makefile.am
> ++++ network-manager-0.9.10.0/src/devices/wwan/Makefile.am
> @@ -49,6 +49,13 @@ libnm_wwan_la_SOURCES += \
> nm-modem-broadband.h
> endif
> @@ -53,10 +53,10 @@
> WWAN_SYMBOL_VIS_FILE=$(srcdir)/wwan-exports.ver
>
> libnm_wwan_la_LDFLAGS = \
> -Index: b/src/devices/wwan/nm-modem-manager.c
> +Index: network-manager-0.9.10.0/src/devices/wwan/nm-modem-manager.c
> ===================================================================
> ---- a/src/devices/wwan/nm-modem-manager.c
> -+++ b/src/devices/wwan/nm-modem-manager.c
> +--- network-manager-0.9.10.0.orig/src/devices/wwan/nm-modem-manager.c
> ++++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-manager.c
> @@ -17,7 +17,7 @@
> *
> * Copyright (C) 2009 - 2014 Red Hat, Inc.
> @@ -300,11 +300,11 @@
> #if WITH_MODEM_MANAGER_1
> /* ModemManager >= 0.7 */
> clear_modem_manager_1_support (self);
> -Index: b/src/devices/wwan/nm-modem-ofono.c
> +Index: network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
> ===================================================================
> --- /dev/null
> -+++ b/src/devices/wwan/nm-modem-ofono.c
> -@@ -0,0 +1,1378 @@
> ++++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
> +@@ -0,0 +1,1395 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> +/* NetworkManager -- Network link manager
> + *
> @@ -414,7 +414,7 @@
> +
> + NMIP4Config *ip4_config;
> +
> -+ gboolean state;
> ++ gboolean enabled;
> +} NMModemOfonoPrivate;
> +
> +#define NM_OFONO_ERROR (nm_ofono_error_quark ())
> @@ -460,6 +460,7 @@
> +{
> + SimpleDisconnectContext *ctx = (SimpleDisconnectContext*) user_data;
> + NMModemOfono *self = ctx->self;
> ++ NMModemState state = nm_modem_get_state (NM_MODE (self));
> + GError *error = NULL;
> +
> + nm_log_dbg (LOGD_MB, "in %s", __func__);
> @@ -474,9 +475,10 @@
> +
> + simple_disconnect_context_free (ctx);
> +
> -+ nm_modem_set_state (NM_MODEM (self),
> -+ NM_MODEM_STATE_CONNECTED,
> -+ nm_modem_state_to_string (NM_MODEM_STATE_CONNECTED));
> ++ if (state != NM_MODEM_STATE_SEARCHING)
Yes, because the way the plumbing works, when the modem enabled changes to FALSE, the modem state is set to SEARCHING, however a disconnect is also triggered which was just setting the state back to REGISTERED ( well actually it was setting the state to CONNECTED which was very wrong, so when I changed it to REGISTERED instead, I need to guard again the case where the state is already SEARCHING and not change it ).
> ++ nm_modem_set_state (NM_MODEM (self),
> ++ NM_MODEM_STATE_REGISTERED,
> ++ nm_modem_state_to_string (NM_MODEM_STATE_REGISTERED));
> +}
> +
> +static void
> @@ -539,7 +541,7 @@
> + NMModemState new_state;
> + NMDeviceStateReason reason;
> +
> -+ if (new_enabled == priv->state)
> ++ if (new_enabled == priv->enabled)
> + return;
> +
> + if (new_enabled) {
> @@ -559,7 +561,7 @@
> + new_state,
> + nm_modem_state_to_string (new_state));
> +
> -+ priv->state = new_enabled;
> ++ priv->enabled = new_enabled;
> +
> + nm_log_info (LOGD_MB, "(%s) now in state: %s",
> + nm_modem_get_path (NM_MODEM (self)),
> @@ -633,7 +635,9 @@
> + }
> + else if (g_strcmp0 (key, "Attached") == 0 && G_VALUE_HOLDS_BOOLEAN (value)) {
> + priv->gprs_attached = g_value_get_boolean (value);
> -+ }
> ++ } else
> ++ /* No need to update enabled for other property changes */
> ++ return;
> +
> + update_ofono_enabled (self, priv->modem_online
> + && priv->gprs_powered
> @@ -875,6 +879,13 @@
> + g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE,
> + NM_DEVICE_STATE_REASON_MODEM_BUSY);
> +
> ++ /*
> ++ * FIXME: add code to check for InProgress so that the
> ++ * connection doesn't continue to try and activate,
> ++ * leading to the connection being disabled, and a 5m
> ++ * timeout...
> ++ */
> ++
> + g_error_free (error);
> + }
> +}
> @@ -1080,7 +1091,7 @@
> +
> + dbus_g_proxy_begin_call_with_timeout (priv->context_proxy,
> + "SetProperty", stage1_prepare_done,
> -+ self, NULL, 20000,
> ++ self, NULL, 40000,
> + G_TYPE_STRING, "Active",
> + G_TYPE_VALUE, &value,
> + G_TYPE_INVALID);
> @@ -1550,6 +1561,12 @@
> + nm_modem_get_path (self),
> + enabled ? "enabled" : "disabled");
> +
> ++ /*
> ++ * FIXME: this is code is a no-op; we should either make
> ++ * work, or get rid of this function, or at least remove
> ++ * the call to update_ofono_enabled.
> ++ */
> ++
As 'enabled' isn't passed to update_ofono_enabled(), this function currently does nothing other than log a misleading message. That's what I meant by a no-op. As we may want to restore this functionality at some point, I chose to add a FIXME comment to make this a little more obvious.
> + update_ofono_enabled (NM_MODEM_OFONO (self),
> + priv->modem_online
> + && priv->gprs_powered
> @@ -1683,10 +1700,10 @@
> + //dbus_g_error_domain_register (NM_OFONO_ERROR, NULL, NM_TYPE_OFONO_ERROR);
> +}
> +
> -Index: b/src/devices/wwan/nm-modem-ofono.h
> +Index: network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.h
> ===================================================================
> --- /dev/null
> -+++ b/src/devices/wwan/nm-modem-ofono.h
> ++++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.h
> @@ -0,0 +1,64 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> +/* NetworkManager -- Network link manager
> @@ -1752,10 +1769,10 @@
> +G_END_DECLS
> +
> +#endif /* NM_MODEM_OFONO_H */
> -Index: b/src/NetworkManagerUtils.c
> +Index: network-manager-0.9.10.0/src/NetworkManagerUtils.c
> ===================================================================
> ---- a/src/NetworkManagerUtils.c
> -+++ b/src/NetworkManagerUtils.c
> +--- network-manager-0.9.10.0.orig/src/NetworkManagerUtils.c
> ++++ network-manager-0.9.10.0/src/NetworkManagerUtils.c
> @@ -1221,7 +1221,13 @@ nm_utils_ip6_property_path (const char *
>
> len = g_snprintf (path, sizeof (path), IPV6_PROPERTY_DIR "%s/%s",
> @@ -1787,3 +1804,39 @@
> + return name;
> }
>
> +Index: network-manager-0.9.10.0/src/devices/wwan/nm-device-modem.c
> +===================================================================
> +--- network-manager-0.9.10.0.orig/src/devices/wwan/nm-device-modem.c
> ++++ network-manager-0.9.10.0/src/devices/wwan/nm-device-modem.c
> +@@ -201,6 +201,16 @@ modem_state_cb (NMModem *modem,
> + }
> + }
> +
> ++ if (dev_state >= NM_DEVICE_STATE_DISCONNECTED &&
> ++ new_state == NM_MODEM_STATE_REGISTERED && old_state < NM_MODEM_STATE_REGISTERED) {
> ++
> ++ nm_log_info (LOGD_MB, "(%s): modem re-registered; re-checking autoconnect",
> ++ nm_device_get_iface (device));
> ++
> ++ g_object_set (G_OBJECT (device), NM_DEVICE_AUTOCONNECT, TRUE, NULL);
> ++ nm_device_emit_recheck_auto_activate (device);
> ++ }
> ++
> + if (new_state < NM_MODEM_STATE_CONNECTING &&
> + old_state >= NM_MODEM_STATE_CONNECTING &&
> + dev_state >= NM_DEVICE_STATE_NEED_AUTH &&
> +@@ -271,6 +281,14 @@ device_state_changed (NMDevice *device,
> + nm_modem_state_to_string (nm_modem_get_state (priv->modem)));
> + }
> +
> ++ /* Block autoconnect until the modem is registered again */
> ++ if (new_state == NM_DEVICE_STATE_FAILED && nm_modem_get_state (priv->modem) == NM_MODEM_STATE_SEARCHING) {
> ++ nm_log_info (LOGD_MB, "(%s): modem searching; disabling autoconnect",
> ++ nm_device_get_iface (device));
> ++
> ++ g_object_set (G_OBJECT (device), NM_DEVICE_AUTOCONNECT, FALSE, NULL);
> ++ }
> ++
> + nm_modem_device_state_changed (priv->modem, new_state, old_state, reason);
> +
> + switch (reason) {
>
--
https://code.launchpad.net/~phablet-team/network-manager/lp1418077/+merge/255285
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~phablet-team/network-manager/lp1418077 into lp:~network-manager/network-manager/ubuntu.
More information about the Ubuntu-reviews
mailing list