[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: RFS: ndpmon




While reading into your package I'm wondering about:

1.) ndpmon.init:
       - DAEMON and INIT must be defined in /etc/default/ndpmon. I doubt
that's a good default. You should define fall-back defaults in the init
script too

       - you background start-stop-daemon during starting yet try to evaluate
its return value. I doubt that's a good combination. Moreover you have a
added sleep without any useful value IMHO as the return code is pulled
before the sleep anyway.

       - during stop you unconditionally cat a PIDFILE without checking it
exists and run kill on the outcome. A better solution would be something
like PID=$(ps -C $DAEMON -o pid=) and check if that's not empty and kill
that (or check if that's equal to the PIDFILE and kill it then).

       - status should check PIDFILE and/or something like the ps -C $DAEMON
and report based on that.

         I'd recommend you check the manpage of ps for further options you may
see fit.

       - exit $? at the end is very likely to not match what you intended to
use as exit status. Maybe you should

I'm still not sure about the INIT script.  Before, the INIT script that was in the original source code did not support status or force-reload.  So I basically copied the script and added those sections.  Now, I tried to incorporate the suggestions above, which results in an INIT script that is different from the one in the source code, but I'm not sure of how good of a job I did.  Any more suggestions would be appreciated.
 

2.) Patches:
       - there is no need to modify Makefile to delete config.log and
config.status unless you want to report this back to upstream (which is
not indicated in the header). From a Debian POV debian/clean and/or
debian/rules will do (and should be used preferred)

       - I'm not sure install.patch is needed. You can as well use debian/tmp
as DESTDIR and move files from there using dh_install I guess. Reporting
the fixes back upstream seem to make sense yet there again is no
indication this was done.

I removed the patch to the Makefile and added a debian/clean file.  Also, the installation is now handled by the files debian/ndpmon.dirs and debian/install.

 
       - spelling errors - again look useful, but please make sure they don't
remain in Debian's archive alone but are included upstream.

I plan to send the spelling patches upstream.  I was holding off until I had a better idea of the status of getting the program into debian, and all of the changes that were necessary so I could just submit things once.
 
 
3.) debian/rules template header can be omited.

Done.
 
 
   as autotools-dev are already in Build-Depends (which is good) they
should also be activated in debian/rules (using --with autotools_dev)

Done.  I also added -with autoreconf.

 
4.) Fetching http://standards.ieee.org/regauth/oui/oui_public.txt.

   OUCH! There is no internet access guaranteed during building a
package. That means this is quite likely to fail.

   Moreover:
Fetching http://www.cavebear.com/CaveBear/Ethernet.txt
Error fetching http://www.cavebear.com/CaveBear/Ethernet/Ethernet.txt:
404 Not Found

   N.b. there is an attempt to make a shared package for that. See e.g.
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=481299 for details and
more bugs numbers. Please try to use that package (once avail) or an
offline copy in the meantime.

The purpose of make-manuf is to provide an up-to-date copy of manuf, which the program uses to match MAC addresses with manufacturers.  If Internet access is not available during the build, I thought it would be better to create an up-to-date copy of manuf offline, and simply produce a patch to bring the copy in the original source code up-to-date, and make sure make-manuf doesn't run. 

 
5.) Rebuilding your package twice in a row produces a Debian patch:
dpkg-source: info: local changes stored in
ndpmon-1.4.0/debian/patches/debian-changes-1.4.0-1, the modified files
are:
 ndpmon-1.4.0/Makefile
 ndpmon-1.4.0/config_ndpmon.xml
 ndpmon-1.4.0/ndpmon.sh
 ndpmon-1.4.0/ndpmon_defs.h
 ndpmon-1.4.0/neighbor_list.xml
 ndpmon-1.4.0/plugins/countermeasures/Makefile
 ndpmon-1.4.0/plugins/mac_resolv/Makefile
dpkg-source: info: building ndpmon in ndpmon_1.4.0-1.debian.tar.gz

I believe this problem is solved with the use of the debian/clean file. 

 
6.) You still ship source files in your binary package like:
./usr/lib/ndpmon/plugins/mac_resolv/mac_resolv.c
./usr/lib/ndpmon/plugins/mac_resolv/mac_resolv.h
./usr/lib/ndpmon/plugins/mac_resolv/Makefile.in
./usr/lib/ndpmon/plugins/countermeasures/icmp_lib_nd.c
./usr/lib/ndpmon/plugins/countermeasures/countermeasures.c
./usr/lib/ndpmon/plugins/countermeasures/countermeasures.h
./usr/lib/ndpmon/plugins/countermeasures/icmp_lib_nd.h
./usr/lib/ndpmon/plugins/countermeasures/icmp_lib.c
./usr/lib/ndpmon/plugins/countermeasures/Makefile.in
./usr/lib/ndpmon/plugins/countermeasures/icmp_lib.h
./usr/lib/ndpmon/plugins/countermeasures/countermeasures_on_link.h
./usr/lib/ndpmon/plugins/countermeasures/countermeasures_guard.h

This problem is solved with the use of the debian/install file.



John R. Baskwill, jrb28@psu.edu
Systems Analyst, Information Technology Services
Penn State Harrisburg
W303 Olmsted Building
777 West Harrisburg Pike
Middletown, PA 17057-4898
Phone: 717-948-6268
Fax: 717-948-6535

Reply to: