Re: nm_setting_gsm_get_pin() retuns null

[Dan Williams <dcbw redhat com>]

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