Re: ModemManager [PATCH 2/2] Take 3 of: Improvements to SIM PIN handling - Add EnabledFacilityLocks property

[Dan Williams <dcbw redhat com>]

On Thu, 2011-12-01 at 09:11 +0100, Aleksander Morgado wrote:
> > > > I guess I don't have a problem with merging them, although strictly
> > > > speaking they refer to different things - locks vs. the PINs needed to
> > > > unlock those locks.
> > > > 
> > > Yeah, you're totally right... but at the end the PIN/PUK codes are
> > > directly related to facilities. E.g "SIM facility", has "PIN code" and
> > > "PUK code". But it really seems broken to use the enum of codes as index
> > > for facilities, not the best idea.
> > > > 
> > > > Two other points to consider regarding lock/PIN handling in the new
> > > > API:
> > > >       * PIN, PUK, PIN2, and PUK2 are PINs for SIM locks. The rest are
> > > >         device locks, so having them belong to the *.Sim interface
> > > >         doesn't seem completely right. But 3gpp gloms them all
> > > >         together, so maybe we should just live with this.
> > > 
> > > In the 0.6 API they are not handled in the SIM interface. UnlockRetries
> > > and PinRetryCounts are handled in the Modem interface, along with
> > > UnlockRequired; and the EnabledFacilityLocks one is handled in the 3GPP
> > > interface. Anyway, I'm not very convinced yet, on why we do keep
> > > EnabledFacilityLocks in one interface and
> > > UnlockRetries/PinRetryCounts/UnlockRequired in another one. Shouldn't we
> > > have all in the 3GPP-specific interface? Are there CDMA-only modems with
> > > facility locks?
> > 
> > In Russia and Korea there's RUIMs which are basically SIMs for CDMA and
> > aren't 3GPP at all.  They may use the same basic interfaces, but there's
> > a lot of Qualcomm-iness in there.  I kept the PIN/Lock stuff on the
> > Modem interface so that we could more easily support these types of
> > devices in the future.
> > 
> 
> I see, good to know.
> 
> > > >       * Shouldn't we add an argument to EnablePin and ChangePin that
> > > >         specifies the lock to which the operation should apply? This
> > > >         would mirror the CLCK and CPWD AT commands, which take the
> > > >         facility name as an argument. This would correct what seems to
> > > >         be an oversight in the current API.
> > > 
> > > Not an oversight, just that the main use case is to be able to
> > > Enable/Change the SIM PIN, and the idea was to keep it simple. But now
> > > that we report if given facility locks are enabled or disabled, it could
> > > make sense to also allow trying to enable/disable all reported ones. We
> > > would need to move Enable() and Change() out from the SIM object and
> > > back to some interface in the Modem object.
> > > 
> > > And now that you talk about EnablePin() and ChangePin(); the same logic
> > > could be applied to SendPin() and SendPuk(). SendPin()/SendPuk() should
> > > be used only when UnlockRequired says that there is something to unlock;
> > > they are actually backed by the same CPIN command. So, UnlockRequired
> > > may be saying "ph-sim-pin" (phone-specific PIN required), and we can
> > > then use SendPin() to send that specific code to the modem. Therefore,
> > > SendPin() is really not related to the SIM, but to UnlockRequired, and
> > > so they ought to be in the same interface (in the 0.6 API, SendPin() and
> > > SendPuk() are managed in the SIM object).
> > 
> > That makes some sense.  They were originally added when the world was a
> > simpler place :)
> > 
> 
> If we do move SendPin(), SendPuk, ChangePin(), EnablePin() out of the
> SIM object, not sure if it is worth having it around.
> 
> > > 
> > > And a new brainstorm here... given that we're going to break the DBus
> > > API, we can try to rework and consolidate all related properties.
> > > 
> > > We could assume that each facility has 2 codes (PIN, PUK), and that the
> > > PUK code will only be required once all PIN attempts have been used
> > > (didn't look at any documentation, but that seems to me always the
> > > case).
> > 
> > That looks to be the case, yes.  The non-call-barring CLCK facilities
> > have corresponding lock codes that would be reported by +CPIN?.
> > 
> > > If we do assume this, we could then setup a "FacilityLocks" dictionary
> > > with signature "a{ubuu}" where:
> > >  * The uint32 in the first position identifies the facility.
> > >  * The boolean in the second position tells whether the facility has a
> > > lock.
> > >  * The uint32 in the third position has the PIN retry count.
> > >  * The uint32 in the fourth position has the PUK retry count.
> > > 
> > > Then, we could have UnlockRequired be just a uint32 where we identify
> > > the 'facility' locked (as opposed to identifying which pin code we
> > > need).
> > 
> > I assume you're suggesting we do an enum for facilities?  (which is
> > fine)
> > 
> 
> We already have that enum, MMModemGsmFacility is called in git master.
> 
> > > So, if we get "SIM" facility in UnlockRequired, we could then check the
> > > corresponding entry in the FacilityLocks dictionary and see how many
> > > retry counts we have for PIN and PUK. If retries for PIN is 0; it means
> > > we need the PUK code, unless PUK retries is also 0, which would mean we
> > > need to buy a new SIM card. Instead of just the key of the dictionary
> > > entry, we could also have the UnlockRequired property be a (ubuu); which
> > > would contain the whole entry of the dictionary.
> > > 
> 
> I'm wondering if it would be a good idea to have a Modem.Lock interface,
> with all this stuff (methods to send/enable/change PINs and PUKs,
> facility lock enabled flags, pin retry counts, unlock-required value...)

Sure?  Though to keep it easier for clients to deal with I tried to keep
the number of interfaces a minimum.  At least with most C bindings for
D-Bus you need to keep separate C objects around for each interface
which makes it annoying when listening to signals.  That's mainly a
problem with dbus-glib which we all want to die die die; I'm not sure
about dbus-c++ or gdbus or the Qt bindings.

Dan