Re: nm_setting_gsm_get_pin() retuns null

[Dan Williams <dcbw redhat com>]

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