Re: nm_setting_gsm_get_pin() retuns null

[Dan Williams <dcbw redhat com>]

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
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>
> >
> >
>