Re: Autoconnect for mobile broadband (was: Re: nm_setting_gsm_get_pin() retuns null)

[Dan Williams <dcbw redhat com>]

On Thu, 2011-10-13 at 05:25 +0200, Anders Feder wrote:
> My pleasure - if only getting rid of bugs was this easy in all
> software...
> 
> Speaking of bugs, I have another open issue in Bugzilla:
> https://bugzilla.gnome.org/show_bug.cgi?id=659228
> 
> I've been told that autoconnect is not supported for mobile broadband
> yet:
> https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/848164/comments/3
> 
> Are there any plans to rectify this?

We had a discussion about this right before the 0.9 release.  On the one
hand, most laptop users probably don't want autoconnect for embedded
laptop cards; I've never seen Windows do autoconnect for embedded cards
by default either.  This is mainly because the card is always inserted
and you don't necessarily always want the card powered up and trying to
connect to a network.

On the other hand, embedded systems and possibly users with USB dongles
may want autoconnect, and I've seen any number of connection managers
for Windows autoconnect USB dongles, since it's an explicit decision by
the user to plug the device in, they probably want to use that device
soon.  This is something I'd forgotten about during the original
discussion a few months ago.

Perhaps the solution here is that if any 3G connection that applies to
the device is marked "autoconnect", that NM enables that device (ie,
power on an let it search for a network) and then attempts to
autoconnect it.  We could default to not auto-connecting 3G connections
(to ensure we don't charge them money), though this would mean that
after implementing this change, existing 3G connections marked
autoconnect=true would change behavior.

nm-applet, at least, always made 3G connections autoconnect=false.  Not
sure what the KDE bits or gnome-shell bits do here.

> Thanks,
> Anders Feder
> 
> On 13-10-2011 01:49, Dan Williams wrote: 
> > On Wed, 2011-10-12 at 05:24 +0200, Anders Feder wrote:
> > > Yes, that did the trick.
> > > 
> > > I've added the keyring functionality and now have the attached diff. 
> > > I've tested it and it works (yay).
> > > 
> > > How can I get it included in the main source tree?
> > It's applied, thanks!  I did a few cleanups and converted the keyring
> > calls to by asynchronous, and pushed it to the 0.8 and git master (0.9)
> > branches.  Thanks for fixing this long-standing feature request.
> > 
> > Dan
> > 
> > > Thanks,
> > > Anders Feder
> > > 
> > > On 10-10-2011 23:30, Dan Williams wrote:
> > > > On Sun, 2011-10-09 at 17:28 +0200, Anders Feder wrote:
> > > > > I now have the attached diff. Any chance you can help me understand why
> > > > > it segfaults when I try running it? Thanks,
> > > > Missing trailing G_TYPE_INVALID in gsm_device_added() on the GetAll call
> > > > will make dbus-glib overrun the end of the argument list.  That's a
> > > > marker for the varargs list that tells dbus-glib it can stop processing
> > > > arguments.  Not sure if that's it but should be fixed.
> > > > 
> > > > Any idea where it's crashing?  can you get a backtrace when running the
> > > > applet in gdb?
> > > > 
> > > > Dan
> > > > 
> > > > > Anders Feder
> > > > > 
> > > > > On 05-10-2011 23:35, Dan Williams wrote:
> > > > > > On Wed, 2011-10-05 at 23:21 +0200, Anders Feder wrote:
> > > > > > > Alternatively, can I simply request "SimIdentifier" from unlock_reply()
> > > > > > > instead of gsm_device_added()? That way sim_id_reply() won't be invoked
> > > > > > > unless unlock_reply() has already completed.
> > > > > > You could chain them together that way, yes, if you moved the stuff from
> > > > > > gsm_device_added() up into unlock_reply() where you get a successful
> > > > > > reply for DeviceIdentifier.
> > > > > > 
> > > > > > Dan
> > > > > > 
> > > > > > > Anders Feder
> > > > > > > 
> > > > > > > On 05-10-2011 22:45, Dan Williams wrote:
> > > > > > > > On Wed, 2011-10-05 at 22:12 +0200, Anders Feder wrote:
> > > > > > > > > Thanks for that thorough explanation. However, I wonder if there isn't a
> > > > > > > > > race condition in that implementation: if we request all three
> > > > > > > > > properties in gsm_device_added(), can we be certain that we have all of
> > > > > > > > > them once we are in sim_id_reply()? Isn't there a risk that
> > > > > > > > > sim_id_reply() might be called back before unlock_reply()?
> > > > > > > > Yes, a small risk.  This can be alleviated by doing some logic in both
> > > > > > > > functions and tracking in the 'info' struct whether we've gotten replies
> > > > > > > > for both the initial modem properties and the initial card properties,
> > > > > > > > and then only doing the unlock dialog when both of those are true AND
> > > > > > > > the other stuff I wrote was true, and doing that check from both places.
> > > > > > > > It just means more variables.
> > > > > > > > 
> > > > > > > > Dan
> > > > > > > > 
> > > > > > > > > Anders Feder
> > > > > > > > > 
> > > > > > > > > On 03-10-2011 23:18, Dan Williams wrote:
> > > > > > > > > > On Mon, 2011-10-03 at 14:20 +0200, Anders Feder wrote:
> > > > > > > > > > > Where would you propose the new code be added?
> > > > > > > > > > > 
> > > > > > > > > > > In the first iteration, I imagine it would work something like this:
> > > > > > > > > > > 
> > > > > > > > > > > If (UnlockRequired)
> > > > > > > > > > >          Get SimIdentifier
> > > > > > > > > > >          If (SimIdentifier is found)
> > > > > > > > > > >              Get PIN for SimIdentifier from keyring
> > > > > > > > > > >              If (PIN is found for SimIdentifier)
> > > > > > > > > > >                  Try unlock using saved PIN
> > > > > > > > > > >                  While (unlock failed)
> > > > > > > > > > >                      Prompt user for new PIN
> > > > > > > > > > >                      Try unlock using new PIN
> > > > > > > > > > >                      If (unlock succeeded)
> > > > > > > > > > >                          Save new PIN for SimIdentifier to keyring
> > > > > > > > > > > 
> > > > > > > > > > > If this works, a similar procedure could be applied for
> > > > > > > > > > > DeviceIdentifier, if SimIdentifier is not found.
> > > > > > > > > > > 
> > > > > > > > > > > Is this the best approach?
> > > > > > > > > > Yeah, that's basically it.  So UnlockRequired and DeviceIdentifier are
> > > > > > > > > > both properties of the org.freedesktop.ModemManager.Modem interface, and
> > > > > > > > > > thus you can retrieve them both in the same D-Bus properties call using
> > > > > > > > > > GetAll().  For that, in src/applet-device-gsm.c's gsm_device_added()
> > > > > > > > > > function, where it calls the Get() method with UnlockRequired, what you
> > > > > > > > > > want to do is call "GetAll" instead and kill the "UnlockRequired"
> > > > > > > > > > argument.  Then in the unlock_reply() for the dbus_g_proxy_end_call()
> > > > > > > > > > you'll do something like:
> > > > > > > > > > 
> > > > > > > > > > GHashTable *props = NULL;
> > > > > > > > > > 
> > > > > > > > > > if (dbus_g_proxy_end_call (proxy, call,&error,
> > > > > > > > > >                                DBUS_TYPE_G_MAP_OF_VARIANT,&props,
> > > > > > > > > >                                G_TYPE_INVALID)) {
> > > > > > > > > >         GHashTableIter iter;
> > > > > > > > > >         const char *prop_name;
> > > > > > > > > >         GValue *value;
> > > > > > > > > > 
> > > > > > > > > >         g_hash_table_iter_init (&iter, props);
> > > > > > > > > >         while (g_hash_table_iter_next (&iter, (gpointer)&prop_name, (gpointer)&value)) {
> > > > > > > > > >             if ((strcmp (prop_name, "UnlockRequired") == 0)&&     G_VALUE_HOLDS_STRING (value)) {
> > > > > > > > > > 	    g_free (info->unlock_required);
> > > > > > > > > >                 info->unlock_required = parse_unlock_required (value);
> > > > > > > > > >             }
> > > > > > > > > > 
> > > > > > > > > >             if ((strcmp (prop_name, "DeviceIdentifier") == 0)&&     G_VALUE_HOLDS_STRING (value)) {
> > > > > > > > > >                 g_free (info->devid);
> > > > > > > > > >                 info->devid = g_value_dup_string (value);
> > > > > > > > > >             }
> > > > > > > > > >         }
> > > > > > > > > > }
> > > > > > > > > > 
> > > > > > > > > > That takes care of the UnlockRequired and DeviceIdentifier properties.
> > > > > > > > > > Now you need to get the SimIdentifier property, which is a GSM-specific
> > > > > > > > > > property (unlike UnlockRequired and DeviceIdentifier which are provided
> > > > > > > > > > for CDMA devices too).  For that you'll want to do something like this
> > > > > > > > > > in gsm_device_added():
> > > > > > > > > > 
> > > > > > > > > > 	dbus_g_proxy_begin_call (info->props_proxy, "Get",
> > > > > > > > > > 	                         sim_id_reply, info, NULL,
> > > > > > > > > > 	                         G_TYPE_STRING, MM_DBUS_INTERFACE_MODEM_GSM_CARD,
> > > > > > > > > > 	                         G_TYPE_STRING, "SimIdentifier",
> > > > > > > > > > 	                         G_TYPE_INVALID);
> > > > > > > > > > 
> > > > > > > > > > and then basically copy&     paste the current unlock_reply() function as
> > > > > > > > > > sim_id_reply(), fix up the property stuff there (obviously you're
> > > > > > > > > > handling the SimIdentifier property instead of UnlockRequired) and then
> > > > > > > > > > at the end of that function, you want something like this:
> > > > > > > > > > 
> > > > > > > > > >         if (info->unlock_required&&     (info->simid || info->devid))
> > > > > > > > > > 	unlock_dialog_new (info->device, info);
> > > > > > > > > > 
> > > > > > > > > > which will actually do the unlock.  You dont' want to call
> > > > > > > > > > unlock_dialog_new() in unlock_reply() because you don't have the
> > > > > > > > > > SimIdentifier yet.
> > > > > > > > > > 
> > > > > > > > > > Then unlock_dialog_new() is where you'd query the keyring for existing
> > > > > > > > > > PINs using SimIdentifier as one of the attributes the PIN would be
> > > > > > > > > > stored with (using gnome_keyring_find_itemsv_sync()) and if there wasn't
> > > > > > > > > > any result try again with DeviceIdentifier.  Check out utils/utils.c and
> > > > > > > > > > src/applet-agent.c for more examples of how to write and read items from
> > > > > > > > > > the keyring.
> > > > > > > > > > 
> > > > > > > > > > Dan
> > > > > > > > > > 
> > > > > > > > > > > Anders Feder
> > > > > > > > > > > 
> > > > > > > > > > > On 03-10-2011 06:00, Dan Williams wrote:
> > > > > > > > > > > > On Sat, 2011-10-01 at 02:41 +0200, Anders Feder wrote:
> > > > > > > > > > > > > Thanks. I've left you a question in the bug report. I may be
> > > > > > > > > > > > > interested in taking a stab at this if it isn't overwhelmingly
> > > > > > > > > > > > > difficult.
> > > > > > > > > > > > Responded.  It shouldn't be too difficult; there's already code there to
> > > > > > > > > > > > track the modem object from ModemManager and do the right thing.  So in
> > > > > > > > > > > > addition to the UnlockRequired property, you'd also want to grab and
> > > > > > > > > > > > watch the SimIdentifier and DeviceIdentifier properties, and use those
> > > > > > > > > > > > to look in the keyring for the right PIN.
> > > > > > > > > > > > 
> > > > > > > > > > > > Dan
> > > > > > > > > > > > 
> > > > > > > > > > > > > Anders Feder
> > > > > > > > > > > > > 
> > > > > > > > > > > > > On 30-09-2011 07:31, Dan Williams wrote:
> > > > > > > > > > > > > > On Wed, 2011-09-21 at 08:34 +0200, Anders Feder wrote:
> > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > I'm trying to write a fix for this bug. I've been experimenting with
> > > > > > > > > > > > > > > testing whether a given connection is configured to autoconnect, using
> > > > > > > > > > > > > > > this code (inside applet-device-gsm.c):
> > > > > > > > > > > > > > >              NMSettingConnection *setting =
> > > > > > > > > > > > > > >              nm_connection_get_setting_connection (connection);
> > > > > > > > > > > > > > >              NMSettingGsm *setting_gsm = nm_connection_get_setting_gsm
> > > > > > > > > > > > > > >              (connection);
> > > > > > > > > > > > > > >              if ((autoconnects = autoconnects ||
> > > > > > > > > > > > > > >              (nm_setting_connection_get_autoconnect (setting)&&
> > > > > > > > > > > > > > >              nm_setting_gsm_get_pin (setting_gsm))))
> > > > > > > > > > > > > > >                  break;
> > > > > > > > > > > > > > > However, nm_setting_gsm_get_pin (setting_gsm) seems to always return
> > > > > > > > > > > > > > > null, even when a PIN is set for the connection. Can someone pelase
> > > > > > > > > > > > > > > tell me why this might be? Under what circumstances may this function
> > > > > > > > > > > > > > > return null even when a PIN is set?
> > > > > > > > > > > > > > PIN codes in the connection data are deprecated because PINs are
> > > > > > > > > > > > > > specific to the SIM card, not to the connection.  If you lose your SIM
> > > > > > > > > > > > > > and get another from the same provider, the PIN will be different, and
> > > > > > > > > > > > > > the PIN in the connection data will be wrong.  I've written up some
> > > > > > > > > > > > > > notes on what I think should be done in the GNOME bug report for this
> > > > > > > > > > > > > > issue:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > https://bugzilla.gnome.org/show_bug.cgi?id=618532
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Happy to help if there are more questions.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Dan
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > 
> > 
> > 
>