On Sun, Mar 10, 2013 at 03:25:19PM +0000, David Woodhouse wrote: > On Sun, 2013-03-10 at 11:18 -0400, John Morrissey wrote: > > On Sat, Mar 09, 2013 at 10:40:30PM +0000, David Woodhouse wrote: > > > On Fri, 2013-03-08 at 21:48 -0500, John Morrissey wrote: > > > > On Thu, Mar 07, 2013 at 11:55:18PM +0000, David Woodhouse wrote: > > > > > On Thu, 2013-03-07 at 18:39 -0500, John Morrissey wrote: > > > > > > - openconnect_set_stoken_mode no longer accepts the use_stoken > > > > > > argument and instead always tries to initialize libstoken when > > > > > > called. This makes sense in openconnect(8), but I'm not sure > > > > > > how much of a concern this API change is for upstream > > > > > > consumers of libopenconnect. I also wasn't sure how to account > > > > > > for this in libopenconnect.map.in. > > > > > > > > > > You can't account for it. It's an ABI break and it would take us to > > > > > libopenconnect.so.3. I'd like to avoid this change, if possible. > > > > > > > > Sure, it's easy enough. See this iteration of the patch. > > > > > > Hm, but now your openconnect_set_oath_mode() API is inconsistent with > > > openconnect_set_stoken_mode(). > > > > > > I'd probably be inclined to make them match. > > > > > > Or even to use openconnect_set_stoken_mode() for *both*. Just pass zero > > > to disable, 1 for stoken and 2 for OATH. > > > > Actually, I'm wondering whether revving the API is unavoidable at this > > point. 4.99 was released last month, which revved the API to include > > libstoken support, and if we add TOTP support to API v2.1, it makes it > > harder for consumers to determine whether the OATH-related functions are > > available (they'd have to do their own autoconf checks instead of simply > > checking the API version). It seems that adding symbols to the API has > > resulted in a version bump in the past, too. > > There's a different in bumping from 2.1 to 2.2, and bumping from 2.1 to > 3.0. [snip] Yes, I know that. You mentioned wanting to 'avoid this ABI change', which I incorrectly took to mean *any* ABI version change. We missed having to do this minor-version API change in previous iterations of this patch, so that's why I'm bring it up now, since I just realized it. > > How about revving the API, making the TOKEN_MODE_* enum a typedef in > > openconnect.h, and adding openconnect_set_token_mode() that takes an > > OC_TOKEN_MODE_*? > > > > That way, the symbol naming is clear in what it's doing (i.e., that it's not > > RSA/libstoken-specific), there are no magic numbers to pass thanks to the > > enum typedef, and there's a single symbol to call. We can leave > > openconnect_set_stoken_mode() as a shim in front of > > openconnect_set_token_mode() for transparent backwards compatibility. > > Sounds reasonable. I'll plan to wrap up the TOTP patch in the next few days, then. john -- John Morrissey _o /\ ---- __o jwm at horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
