Sorry, I mentioned extra, but I meant fedora :)

Just a few comments for now:

- License GPL is wrong: GPLv2+ is the right one
- The builds fails:
gcc  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_REENTRANT -DUSE_SOCKET -DHAVE_CLOCK_GETTIME -DVERSION=\"3.2.0\" -Wp,-U_FORTIFY_SOURCE -U_FORTIFY_SOURCE -Wshadow -Wsign-compare -Werror -c -o failover_thread.o failover_thread.c
cc1: warnings being treated as errors
failover_thread.c: In function 'failover_thread':
failover_thread.c:109: error: dereferencing pointer 'uicp' does break strict-aliasing rules
failover_thread.c:109: note: initialized from here
failover_thread.c:110: error: dereferencing pointer 'uicp' does break strict-aliasing rules
failover_thread.c:110: note: initialized from here
failover_thread.c:162: error: dereferencing pointer 'uicp' does break strict-aliasing rules
failover_thread.c:162: note: initialized from here
failover_thread.c:267: error: dereferencing pointer '({anonymous})' does break strict-aliasing rules
failover_thread.c:267: note: initialized from here
make: *** [failover_thread.o] Error 1

- run make %{?_smp_mflags} and not just make, see https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

- use make install DESTDIR=$RPM_BUILD_ROOT and not %makeinstall, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

- in %clean, please just run rm -rf $RPM_BUILD_ROOT

- Source needs to contains a url, where you downloaded it, e.g.
  http://otpd.googlecode.com/files/%{name}-%{version}.tar.gz

- I'm unsure, if the Packager and Vendor tag is in fedora only not used or not allowed. From https://fedoraproject.org/wiki/Packaging/Guidelines#Rpmlint_Errors it's just not used...


- The description is misleading (at least for me):
  "otpd is part of a suite of software"... Which suite?

An inauspicious start as it does not build under CentOS 5, with minimal stuff brought down from RawHide as SRPMs and recompiled to solution

gcc  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_REENTRANT -DUSE_SOCKET -DHAVE_CLOCK_GETTIME -DVERSION=\"3.2.0\" -Wp,-U_FORTIFY_SOURCE -U_FORTIFY_SOURCE -Wshadow -Wsign-compare -Werror -c -o resynctool.o resynctool.c
cc1: warnings being treated as errors
resynctool.c: In function 'main':
resynctool.c:219: warning: format '%llu' expects type 'long long unsigned int', but argument 2 has type 'uint64_t'
resynctool.c:223: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type 'uint64_t'
resynctool.c:240: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type 'uint64_t'
make: *** [resynctool.o] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.43739 (%build)

I have to scratch my head at that warning enhancement, but ...

-- Russ herrold

I should have fixed the above:

SRPMS: http://www.gpaterno.com/external/otpd-3.2.1-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec

I have no way to check if resynctool.c is fixed today under rawhide, however I casted it to long long unsigned int and compile fine under F12.

New SRPMS that fixes manpages to reflect the fork and included missing man page of otppasswd (from: Russ herrold)

Confirmed that this package builds on rawhide (credit of: Russ herrold)

SRPMS: http://www.gpaterno.com/external/otpd-3.2.2-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec

this was what a private email to the maintainer referecned in #5

1) I see a lot of marketing cruft mentioning TRI-D in the files in the
README* --- would it be possible to re-write these to remove the 'marketing'
aspect?

2) I suspect that the 'changelog' entries convention of mentioning the V and
R was not known to you:

compare:
* Mon Feb 11 2008 Frank Cusack <frank>  3.1.0-1
        to
* Thu Nov 19 2009 Giuseppe Paterno' <gpaterno>

3) the man page URL field is probably stale

4) the man page mentions two additional man pages not present:

[herrold@dhcp-103 otpd]$ man gsmd
No manual entry for gsmd

[herrold@dhcp-103 otpd]$ man otppasswd
No manual entry for otppasswd

5) a commented sample /etc/otppasswd is REALLY needed, to show what is
possible, and marked as a (config) file

6) [herrold@dhcp-103 otpd]$ rpm -ql otpd | grep reqs
The 'reqsynctool' binary -- probably living in either /usr/bin/ or
/usr/sbin/ is missing?

=====================

I an in a TUI only environment presently -- I will file this later today in
the bugzilla, or you might do it for me?

sadly, the patch worked an a completely stock C5, i386, but fails a bit later on a souped up x86_64 build box:


gcc  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_REENTRANT -DUSE_SOCKET -DHAVE_CLOCK_GETTIME -DVERSION=\"3.2.1\" -Wall -Wp,-U_FORTIFY_SOURCE -U_FORTIFY_SOURCE -Wshadow -Wsign-compare -Werror -c -o failover_thread.o failover_thread.c
cc1: warnings being treated as errors
failover_thread.c: In function 'failover_thread':
failover_thread.c:112: warning: cast to pointer from integer of different size
failover_thread.c:113: warning: cast to pointer from integer of different size
make: *** [failover_thread.o] Error 1
make: *** Waiting for unfinished jobs....
error: Bad exit status from /var/tmp/rpm-tmp.28908 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.28908 (%build)
[herrold@centos-5 otpd]$ gcc -v
Using built-in specs.
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-libgcj-multifile --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --enable-plugin --with-java-home=/usr/lib/jvm/java-1.4.2-gcj-1.4.2.0/jre --with-cpu=generic --host=x86_64-redhat-linux
Thread model: posix
gcc version 4.1.2 20080704 (Red Hat 4.1.2-46)
[herrold@centos-5 otpd]$

Let's try again :)))

SRPM: http://www.gpaterno.com/external/otpd-3.2.3-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec  

I tried only i386, x86_64

compiled on x86_64 -- thank you

Comment 6, item 5 
The /etc/ file mentioned /etc/otppasswd does not exist in the binary RPM

Newly noticed: looks like there is a spelling or man page section error at the bottom of:

   ...
/usr/share/man/man8/otppasswd.5.gz
/usr/share/man/man8/resynctool.8.gz
/var/run/otpd
[herrold@centos-5 ~]$ man otpd
[herrold@centos-5 ~]$ man otppasswd
No manual entry for otppasswd
[herrold@centos-5 ~]$    

but otppasswd(5) and NOT otpasswd or otppasswd(8) appears there ... I suspect you have it mis-placed in examining the file /usr/share/man/man8/otppasswd.5.gz and that sed -e '@man8@man5@' is intended

[herrold@centos-5 otpd]$ sudo rpm -Uvh   /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.3-1.x86_64.rpm
Password:
Preparing...                ########################################### [100%]
   1:otpd                   ########################################### [100%]
[herrold@centos-5 otpd]$ rpm -qlp /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.3-1.x86_64.rpm | grep etc
/etc/otpd.conf
/etc/rc.d/init.d/otpd
/etc/sysconfig/otpd
[herrold@centos-5 otpd]$       

thank you 

-- Russ herrold

I will fix the manpage.
However I'm torn to include or not to include an empty /etc/otppasswd file.
The file is similar to /etc/passwd and contains the otp secrets in the format:

user:otp-type:secret

And it's not meant to be commented, in a similar way of /etc/passwd.
What should I do??
Create an empty file?
Insert two demo users??? (might be a security concern tough of default conf!)

Any suggestion is appreciated.
Thanks.

Hello,

Suggestion concerning #10:

As a potential user of otpd, I think that an empty /etc/otppasswd file would be OK provided that an example otppasswd file is made available in the docs since a real-life looking example is always nice to have.

Of course the example file should have comments explaining that comments are not supported and that the secrets in the example file are NOT to be used: a command like "dd if=/dev/random bs=4096 count=1 |sha1sum" should be used instead.

By the way, do you know what TRI-D Systems became ? Their home page said something like "We've been bought, we'll be back soon" and nothing came, and nowadays the domain www.tri-dsystems.com does not resolve any more...
I've read somewhere that Redhat was the buyer, and that they were investigating the "IP"... 

Best regards,
Antoine

(In reply to comment #11)

> not supported and that the secrets in the example file are NOT to be used: a
> command like "dd if=/dev/random bs=4096 count=1 |sha1sum" should be used
> instead.

Try "mkpasswd".

I cannot comment on #11 on TRI-D as I'm a Red Hat employee.

This code came off the TRI-D public web site *before* it has been acquired. Please consider this a fork from the original project and the long-term goal is to get rid of the hooks to the proprietary code and introduce new algorithms and database support.

However, in this relase I created an empty /etc/otppasswd, examples are provided in the man page otppasswd.5 (which has been fixed however for #9).

New pakages
SRPM: http://www.gpaterno.com/external/otpd-3.2.4-1.src.rpm
SPEC: http://www.gpaterno.com/external/otpd.spec

Sorry ... I forgot to thank you!!! :)

clean compile, but in a upgrade situation, I get the following error.  I assume a conditional to test state is needed in the relevant script?

[herrold@centos-5 otpd]$ sudo rpm -Uvh  /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.4-1.x86_64.rpm
Preparing...                ########################################### [100%]
   1:otpd                   ########################################### [100%]
Stopping /usr/sbin/otpd: [FAILED]
Starting /usr/sbin/otpd: /usr/sbin/otpd: /etc/otpd.conf: statedir '/etc/otpstate': No such file or directory
[FAILED]
error: %postun(otpd-3.2.3-1.x86_64) scriptlet failed, exit status 1
[herrold@centos-5 otpd]$                      

or, perhaps the state directory is now in /var/run ?

-- Russ herrold

Mmmh, I should create an empty state dir, forgot :(((
Getting old ... ;)

SRPM: http://www.gpaterno.com/external/otpd-3.2.4-2.src.rpm 
SPEC: http://www.gpaterno.com/external/otpd.spec

No feedback??
Should I assume everything is fine???

Hello Giuseppe,

I am trying to build otpd on my centos5 workstation, but the src rpm does not work for me (I guess that the rpm format has changed between fedora12 and rhel5 because even trying to download multiple times did not help):

error: unpacking of archive failed on file rpmbuild/otpd/otpd-3.2.4.tar.gz;4b1247dc: cpio: MD5 sum mismatch

Here is my md5sum for the srpm otpd-3.2.4-2.src.rpm
a8cf26978f45a69c3dbd6c65a353218f  otpd-3.2.4-2.src.rpm


Anyway, I thought I could try to build from the spec file directly. However, the released otpd-3.2.4.tar.gz is not available on google code:

wget http://otpd.googlecode.com/files/otpd-3.2.4.tar.gz 
  Resolving otpd.googlecode.com... 74.125.79.82
  Connecting to otpd.googlecode.com|74.125.79.82|:80... connected.
  HTTP request sent, awaiting response... 404 Not Found
  11:04:21 ERROR 404: Not Found.


Can you please add otpd-3.2.4.tar.gz to the downloadable files on googlecode ?

Best regards,
Antoine

Hi Anto'
 
From Centos5 you can get-around for install .src.rpms builded on f12 (rpm + XZa payload).

Try install mc, use mc to /enter/ into rpm, /enter/ in the CONTENT.cpio, select and copy the files -but .spec- in SOURCES and spec file in SPECS :)

Hello,

The mc trick worked like a charm, and the rpm was built without any errors.

Thanks,
Antoine

Hello,

I am not sure if I should further comment on this bug or open a new one since what I am reporting is not 'packaging' related but more about the software itself. I could also report on the bugtracker of http://otpd.googlecode.com if needed. Please let me know which method is preferred. (I think googlecode might be the best place)

As for my current issue, it is related to resynctool. The man page states that the output of resynctool (using a > redirection) is suitable as /etc/otpstate/username directly.

However on my system, resynctool does not produce a one-liner suitable as state, but two lines. Example below:
$ /usr/sbin/resynctool -1 990878 -2 457035 -u testuser -k 3cd0f53cd4d94b08d249b1f861e655d774fcf0e5
0: 990878
5:testuser:0000000000000002:::0:0:0:
$

I do not know where the first line ("0: 990878\n") comes from, but it needs to be removed to fix the "5 (service error)" I get when testing with otpauth.

IMO, this is a bug in resynctool.

Regards,
Antoine


PS:
A small command line HOTP client in the documentation folder would be a nice addition (for tests).

Sorry for the missing tar.gz, I'm always travelling and it's difficult to cope with everythihg :)

I strongly suggest to try to get out of SVN the tree if the tar.gz is not in googlecode.

the resynctool stuff is not a bug, it's an output of the sequence of OTPs, therefore only the last line has to be taken into consideration to be put in the status file.

However, since it's more practical to "script" resynctool, I might decide to put a "verbose" flag into resynctool and get rid of that output if not requested.

Hello,

If the resynctool output is not a bug, then the bug is in the manpage of resynctool, that states that the output CAN be used to generate the status file.

I agree with you that a verbose flag in resynctool would be a nicer way of fixing the problem.

Regards,
Antoine

PS:
Concerning the bugtracker choice, should I continue to report all my issues here ?

The question was asked about how to unpack a SRPM made by a later RPM version -- the '--nomd5' sub option solved that, by omitting the checksum test.

I see no problems with the packaging at this point.  As I am not eligible to 'sign off' on a packaging, I will ask a friend to do the formal review.

Was it possible to get the 'resynctool' added here, or in a side packaging?

-- Russ herrold

[herrold@centos-5 ~]$ cd build
[herrold@centos-5 build]$ cd otpd/
[herrold@centos-5 otpd]$ wget http://www.gpaterno.com/external/otpd-3.2.4-2.src.rpm
--2009-11-30 09:43:59--  http://www.gpaterno.com/external/otpd-3.2.4-2.src.rpm
Resolving www.gpaterno.com... 78.46.44.108
Connecting to www.gpaterno.com|78.46.44.108|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1198392 (1.1M) [application/x-rpm]
Saving to: `otpd-3.2.4-2.src.rpm'

100%[======================================>] 1,198,392    280K/s   in 5.4s

2009-11-30 09:44:06 (218 KB/s) - `otpd-3.2.4-2.src.rpm' saved [1198392/1198392]

[herrold@centos-5 otpd]$ rpm -Uvh --nomd5 otpd-3.2.4-2.src.rpm
   1:otpd                   ########################################### [100%]
[herrold@centos-5 otpd]$ rpmbuild -ba ~/rpmbuild/SPECS/otpd.spec

I put a more confortable "-d" (debug) in resynctool in the new version.

SPEC: http://www.gpaterno.com/external/otpd.spec
SRPM: http://www.gpaterno.com/external/otpd-3.2.5-2.src.rpm

Sources: http://otpd.googlecode.com/files/otpd-3.2.5.tar.gz

Thank you all for your cooperation!!!

@Antoine: if are related to this package, please do.
If they are related to features/general issues, please consider also on 
http://code.google.com/p/otpd/issues/list

Thanks!

I see that even though I had disabled the otpd, it was started by a upgrade -- should there be a '[condrestart]' rather than an a formal [stop and start always] here?

[herrold@centos-5 otpd]$ sudo rpm -Uvh /home/herrold/rpmbuild/RPMS/x86_64/otpd-3.2.5-2.x86_64.rpm
Password:
Preparing...                ########################################### [100%]
   1:otpd                   ########################################### [100%]
Stopping /usr/sbin/otpd: [FAILED]
Starting /usr/sbin/otpd: [  OK  ]
[herrold@centos-5 otpd]$

An admin might want the man pages or the resynctool, but not the daemon, I think

This is sort of discussed with implementations, at:
   https://bugzilla.redhat.com/show_bug.cgi?id=484345

-- Russ herrold

I'm working to update the sources and the spec for a condrestart.

I don't agree with you on splitting the package, IMHO in this release it doesn't have much sense to have resynctool and man pages without daemon.

In next releases, where I think to use DB support, it might have sense to split packages.

I'll wait for more feedbacks before regenerating the SRPM.

Couple of things, after a quick glance over the spec
- Unless you have very very good reasons to not do it, please consider using %dist in the release field. It'll save you some headaches later
- I do not see the reason for 
# Create an empty otppasswd file
[ ! -f $RPM_BUILD_ROOT/etc/otppasswd ] && echo "" > $RPM_BUILD_ROOT/etc/otppasswd
Since you are in a clean buildroot, the file does not exist unless make install created it. And a simple "touch $RPM_BUILD_ROOT/etc/otppasswd" seems more elegant to me. Or even "> $RPM_BUILD_ROOT/etc/otppasswd" (no need for the "echo " part ). However, nothing incorrect in your approach either. Just a matter of style
- Please consider the comment #26. It's ugly to restart a daemon which was not specifically allowed to be on by the admin

And please ditch the packager tag. The fedora buildsystem will fill it for you.

   With respect to #26 and #28: please make sure the initscript does not install with the daemon started by default. Common Fedora policy is to allow the admin to activate the required services, not enforce them in a "started" state.
   Also, according to http://fedoraproject.org/wiki/Packaging:SysVInitScript#Required_Actions it would be nice if you could implement the missing actions (and also use the exit codes listed in that page)

I agree with comment #30, it would really be nice to have all the required actions in the init script... (It did look very weird to not have at least status...)

I've tried to implemented what you asked, please try again:

SPEC: http://www.gpaterno.com/external/otpd.spec
SRPM: http://www.gpaterno.com/external/otpd-3.2.6-1.fc11.src.rpm

Skimming through the output of rpmlint (and skipping the non relevant stuff),
- the new src version has a cosmetic issue:
 otpd.src: W: mixed-use-of-spaces-and-tabs (spaces: line 12, tab: line 1)
- you forgot to upload the new source.tar.gz to googlecode
- you forgot to add a new entry in the changelog (last entry still points to 3.2.5-2, which makes rpmlint unhappy:
 otpd.x86_64: W: incoherent-version-in-changelog 3.2.5-2 ['3.2.6-1.fc13', '3.2.6-1']
- the service is still installed as default enabled
  otpd.x86_64: W: service-default-enabled /etc/rc.d/init.d/otpd
- and you still do a service restart in %post, which, combined with the above entry, makes the service start even on the systems where the admin decided to stop it
- and we still have
 otpd.x86_64: W: no-reload-entry /etc/rc.d/init.d/otpd
I am not familiar at all with the code and I cannot even test as I have no tokens, but maybe reload could be implemented as a synonym to restart ?



As a second matter, do you have any other submissions or did you perform any package reviews? If you have, please be as kind as to point me to those bugzilla entries. If not, please consider doing so, as this is still a mandatory step before being sponsored.

Sorry guys, back from holidays. 
Will do in next days.

(In reply to comment #34)
> Sorry guys, back from holidays. 
> Will do in next days.  

please remove NotReady from the Whiteboard once you have addressed the mentioned issues.