Bug 237817 - Merge review request for pam_thinkfinger
Summary: Merge review request for pam_thinkfinger
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: thinkfinger
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Julian Sikorski
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 237760
TreeView+ depends on / blocked
 
Reported: 2007-04-25 15:15 UTC by Jose Plans
Modified: 2007-11-30 22:12 UTC (History)
3 users (show)

Fixed In Version: 0.3-2
Clone Of:
Environment:
Last Closed: 2007-09-05 17:19:31 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Updated spec with merged changes (4.11 KB, text/plain)
2007-05-18 22:30 UTC, Julian Sikorski
no flags Details

Description Jose Plans 2007-04-25 15:15:59 UTC
Spec URL: http://belial.dead.li/~jmp/thinkfinger/Fedora/thinkfinger.spec
SRPM URL: http://belial.dead.li/~jmp/thinkfinger/Fedora/SRPMS/pam_thinkfinger-0.3-2.src.rpm
Description: 
ThinkFinger is a driver for the UPEK/SGS Thomson Microelectronics fingerprint
reader (USB ID 0483:2016). The device is being found either as a standalone USB
device, built into USB keyboards or built into laptops.  The following laptop
vendors are using the device:

- Dell
- IBM/Lenovo
- Toshiba

Toshiba is shipping their laptops either with the UPEK/SGS Thomson
Microelectronics fingerprint reader or with a fingerprint reader built by
AuthenTec. The AuthenTec fingerprint reader is *not* supported by ThinkFinger.

SONY laptops with the UPEK/SGS Thomson Microelectronics fingerprint reader are
not supported.

Comment 1 Bastien Nocera 2007-04-25 15:38:43 UTC
A few random questions regarding the design of the program itself, rather than
the package:
- Why does pam_thinkfinger need uinput when pam_usb (http://www.pamusb.org/)
doesn't?
- Why does it need threading (which is likely to cause problems with PAM) (apart
from a bad library design?)

Comment 2 Jose Plans 2007-04-25 15:51:13 UTC
Bastien,
  I was expecting these :-)
- uinput has been added by the maintainer to send \n after the swiped image. Now
this was working fine with the code from 0.2.. To me this is just a gadget and I
will add --with-uinput in configure to give the choice. Ubuntu,debian and other
folks already spoke to me about this.

- threading... same as above! a choice from the maintainer and that I will find
time sometime to get them out. Feel free to assist ;-)
Jose

Comment 3 Kevin Fenzi 2007-04-25 16:16:46 UTC
Isn't this a duplicate of the already available/approved 'thinkfinger' package? 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229291

Perhaps you could talk to the maintainer of that package and offer improvements
and/or help co-maintain it? 

Comment 4 Bastien Nocera 2007-04-25 16:24:07 UTC
(In reply to comment #3)
> Isn't this a duplicate of the already available/approved 'thinkfinger' package? 
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229291
> 
> Perhaps you could talk to the maintainer of that package and offer improvements
> and/or help co-maintain it? 

It is indeed. As Jose is also helping upstream, it would be good if the original
package could include some of Jose's patches.

Comment 5 Jose Plans 2007-04-25 16:26:20 UTC
Also the guy is aware of this, CCing him.


Comment 6 Bastien Nocera 2007-04-25 16:28:05 UTC
Moving to thinkfinger. Julian, could you check with Jose which patches you'd
want to include in the package? The README might be useful as well.

Comment 7 Julian Sikorski 2007-04-25 17:28:59 UTC
I will include the readme as soon as F7 is released, since rawhide is at devel
freeze right now and update from 0.2 to 0.3 would probably need Release
Engineering Team approval.
As for the uinput dependency, I have been investigating how to make 0.2 -> 0.3
update, as well as install, require no config files editing from the users. I
was suggested that creating an udev rule is the best solution. Locally I am
running a version that loads uinput regardless of the devices present, with
snippet borrowed from, nomen omen, udev.
BTW, perhaps I should even hand the maintainership to you, given your coding skills?

Comment 8 Julian Sikorski 2007-04-25 17:31:27 UTC
Oh, one more thing. IIRC there is a problem that whenever system-config-auth is
run, pam_thinkfinger will be removed from the config. I don't know if this can
be solved at thinkfinger side.

Comment 9 Jose Plans 2007-04-25 17:39:40 UTC
Julian,
 in which case you need the patches at :

* http://belial.dead.li/~jmp/thinkfinger/0.2.3/patches/

unless the version you currently packaged is 0.2.2?
these have been comitted upstream by Timo.

I don't mind sharing or maintaining :-)
We should also envisage to rename the package as I've done in :

* http://belial.dead.li/~jmp/thinkfinger/Fedora/

   Jose

Comment 10 Jose Plans 2007-04-25 17:44:19 UTC
For the udev rules, that was an idea I had, however... nah :-) I rather prefered
to pass this onto the patch that opens different locations.
Also, what Bastien was suggesting was to remove uinput and put back the write on
the stdin from the terminal. I will work on a simple patch that adds into
configure.in the --without-uinput, so in Fedora and Ubuntu/Debian we can call
that and bypass the usage of uinput if we agree on this.

Also, note that 0.2.3 is a version pulled out from a devel cvs... it has never
been released :-/; I bet the one you are using is 0.2.2, for this, also check:

      http://belial.dead.li/~jmp/thinkfinger/0.2.2/

Jose


For the threads, that needs some time... And I'm not sure Timo will like the idea.

    Jose

Comment 11 Jose Plans 2007-04-25 18:13:37 UTC
Answering to comment 8, nop not a thinkfinger problem.

Comment 12 Julian Sikorski 2007-04-27 16:41:11 UTC
OK, email to Release Engineering Team sent.

Comment 13 Julian Sikorski 2007-04-27 18:18:53 UTC
The team have suggested to wait for F7. I'll add you as comaintainer in the
meantime.

Comment 14 Julian Sikorski 2007-05-18 22:09:35 UTC
OK, Fedora 7 is branched, so I will push this update to devel soonish.

Comment 15 Julian Sikorski 2007-05-18 22:18:25 UTC
One thing bothers me: with your patch .bir files are stored elsewhere, in
security subdir. How to handle that nicely with an update, so that users won't
have to re-enroll?

Comment 16 Julian Sikorski 2007-05-18 22:30:39 UTC
Created attachment 155035 [details]
Updated spec with merged changes

I have merged your changes with some minor tweaks. Until we figure out how to
handle .bir relocation nicehow to handly, patch is disabled. Also, as you
promised to work on dropping uinput requirement, I left my hacky solution in -
I don't know nothing about udev and given the circumstances it's not worth my
time to learn it. Rename deferred until I figure it how to handle it in CVS.

Comment 17 Julian Sikorski 2007-05-18 22:40:11 UTC
Changes now committed for rawhide.

Comment 18 Julian Sikorski 2007-05-18 22:58:10 UTC
Jose, why did you change the .bir files storage location at all?

Comment 19 Julian Sikorski 2007-05-18 23:47:28 UTC
Built for devel.

Comment 20 Jose Plans 2007-05-20 21:13:53 UTC
Hello! /etc/security was my first choice, however after reading FHS, no binaries
should be stored under /etc/, therefore none of these directories are correct
for this package. I would recommend /var/lib/ as OpenLDAP does to store it's data.

   *  http://www.pathname.com/fhs/pub/fhs-2.3.html#PURPOSE31

But this is just a detail where to store the bir files - yes an upgrade could be
problematic.
Keep the original for the time being.

Comment 21 Julian Sikorski 2007-05-20 21:22:57 UTC
OK, everything seems to be clear now, so I'm planning to close this bug once new
version gets built for all active branches, which will happen after Fedora 7
release. As you are a maintainer now, if you have any changes, feel free to
commit them to CVS directly. Cheers.


Note You need to log in before you can comment on or make changes to this bug.