Bug 197198

Summary: Review Request: ntop
Product: [Fedora] Fedora Reporter: Michael J Knox <michael>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED DUPLICATE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: bjohnson, kevin, pertusus, yaneti
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-09-16 20:37:23 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 201449    

Description Michael J Knox 2006-06-29 06:59:14 UTC
Spec URL: http://www.knox.net.nz/~michael/ntop.spec
SRPM URL: http://www.knox.net.nz/~michael/ntop-3.2-2.src.rpm

Description: 

ntop is a network and traffic analyzer that provides a wealth 
of information on various networking hosts and protocols. ntop
is primarily accessed via a built-in web interface. Optionally,
data may be stored into a database for analysis or extracted 
from the web server in formats suitable for manipulation in 
perl or php.

Comment 1 Michael J Knox 2006-06-29 07:02:19 UTC
to save some time and maybe some help :) 

rpmlint output:

[mjk@fuzzy SPECS]$ rpmlint /home/mjk/rpmbuild/RPMS/i386/ntop-3.2-2.i386.rpm
W: ntop conffile-without-noreplace-flag /etc/ntop/etter.finger.os.gz
W: ntop conffile-without-noreplace-flag /etc/ntop/ntop-cert.pem
W: ntop conffile-without-noreplace-flag /etc/ntop/oui.txt.gz
W: ntop conffile-without-noreplace-flag /etc/ntop/p2c.opt.table.gz
W: ntop conffile-without-noreplace-flag /etc/ntop/specialMAC.txt.gz
W: ntop conffile-without-noreplace-flag /etc/rc.d/init.d/ntop
W: ntop wrong-file-end-of-line-encoding
/usr/share/doc/ntop-3.2/RedHat-rpmbuild-HOWTO.txt
E: ntop non-standard-uid /var/ntop ntop
E: ntop non-standard-gid /var/ntop nobody
E: ntop non-standard-dir-perm /var/ntop 0775
W: ntop devel-file-in-non-devel-package /usr/lib/libntop.so
W: ntop devel-file-in-non-devel-package /usr/lib/libmyrrd.so
E: ntop non-readable /etc/ntop.conf 0600
E: ntop script-without-shellbang /usr/share/doc/ntop-3.2/ntop-autotools.pdf
W: ntop devel-file-in-non-devel-package /usr/lib/libntopreport.so
E: ntop script-without-shellbang /usr/share/ntop/html/privacyNotice.html
E: ntop executable-marked-as-config-file /etc/rc.d/init.d/ntop
E: ntop script-without-shellbang /usr/share/ntop/html/ntopdump.dtd
E: ntop wrong-script-end-of-line-encoding /usr/share/ntop/html/ntopdump.dtd
E: ntop executable-marked-as-config-file /etc/ntop/specialMAC.txt.gz
E: ntop executable-marked-as-config-file /etc/ntop/oui.txt.gz
W: ntop dangerous-command-in-%postun userdel
E: ntop incoherent-subsys /etc/rc.d/init.d/ntop \$prog


I wasn't sure if the *.so files should be split into a devel package or not. 

Comment 2 Ralf Corsepius 2006-06-29 07:38:22 UTC
checking for GDOME... configure: error: could not find Gdome2
error: Bad exit status from /var/tmp/rpm-tmp.34360 (%build)



Comment 3 Michael J Knox 2006-06-29 07:51:49 UTC
argh.. I uploaded the wrong files. Fixed. 

Comment 4 Ralf Corsepius 2006-06-29 08:00:26 UTC
(In reply to comment #3)
> argh.. I uploaded the wrong files. Fixed. 
Please increment the release tag.



Comment 5 Michael J Knox 2006-06-29 08:16:33 UTC
As I said, the wrong files were uploaded, I had multiple copies... anyways,
bumped the release. 

Spec URL: http://www.knox.net.nz/~michael/ntop.spec
SRPM URL: http://www.knox.net.nz/~michael/ntop-3.2-3.src.rpm

Comment 6 Ralf Corsepius 2006-06-29 08:38:35 UTC
(In reply to comment #5)
> As I said, the wrong files were uploaded, 
Replacing packages without release-tag make them abiguous when looking at them
from remote :)


Comment 7 Ralf Corsepius 2006-06-30 06:59:38 UTC
(In reply to comment #1)
> to save some time and maybe some help :) 

Well, I regret having to say this, but, ATM, this package is quite far from
being ready for approval.

> rpmlint output:
> 
> [mjk@fuzzy SPECS]$ rpmlint /home/mjk/rpmbuild/RPMS/i386/ntop-3.2-2.i386.rpm
I presume, you'll be addressing them ... ;)

> I wasn't sure if the *.so files should be split into a devel package or not. 
Well, this package applies a rather weird SONAME'ing scheme.

=> The %{_libdir}/lib*-<version>.so's should be part of the main package.
=> The %{_libdir}/lib*.so (without version inside) should be made part of a
devel package or not be installed at all. Without the package providing an API
(headers) to the libraries, installing the lib*.so's would be pointless.

=> The *.so inside of the plugin dir seem to be needed.

Finally, ... given what I see, I am not sure, I want to see this package in
Fedora :(

Comment 8 Yanko Kaneti 2006-06-30 07:10:41 UTC
The upstream for ntop has a rather distorted interpretation of the GPL as can be
seen in the distribution terms of another one of their wares - nProbe.
http://www.ntop.org/nProbe.html. 

So even if it says GPL, its most likely not what the authors really mean.

Comment 9 Michael J Knox 2006-06-30 10:09:04 UTC
(In reply to comment #7)
> > I wasn't sure if the *.so files should be split into a devel package or not. 
> Well, this package applies a rather weird SONAME'ing scheme.
> 
> => The %{_libdir}/lib*-<version>.so's should be part of the main package.
> => The %{_libdir}/lib*.so (without version inside) should be made part of a
> devel package or not be installed at all. Without the package providing an API
> (headers) to the libraries, installing the lib*.so's would be pointless.
> 
> => The *.so inside of the plugin dir seem to be needed.

And those are the .so files rpmlint is complaining about.
 
> Finally, ... given what I see, I am not sure, I want to see this package in
> Fedora :(

Its GPL'ed, its a functional and useful application. I see no reason why it
should not be consider a worthy application. 


Comment 10 Michael J Knox 2006-06-30 10:13:15 UTC
(In reply to comment #8)
> The upstream for ntop has a rather distorted interpretation of the GPL as can be
> seen in the distribution terms of another one of their wares - nProbe.
> http://www.ntop.org/nProbe.html. 
> 
> So even if it says GPL, its most likely not what the authors really mean.

He sells nProbe, a GPl'ed application he authors... nothing wrong there. 

Please, this is a GPL'ed application and as such, subject to the terms and
conditions of the GPL and should be evaluated as such. 

Comment 11 Ralf Corsepius 2006-06-30 11:41:31 UTC
(In reply to comment #9)
> (In reply to comment #7)

> > => The *.so inside of the plugin dir seem to be needed.
> 
> And those are the .so files rpmlint is complaining about.
I don't know why rpmlint complains. All I can say is, the package's plugin stuff
dlopens the *.so's, i.e. the package is not correctly building the plugins.

> > Finally, ... given what I see, I am not sure, I want to see this package in
> > Fedora :(
> 
> Its GPL'ed, its a functional
Well, the code lets me have doubts on this ...

> and useful application. I see no reason why it
> should not be consider a worthy application. 
I won't hinder you from continuing your submission, ...


Comment 12 Yanko Kaneti 2006-07-02 17:07:47 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > The upstream for ntop has a rather distorted interpretation of the GPL as can be
> > seen in the distribution terms of another one of their wares - nProbe.
> > http://www.ntop.org/nProbe.html. 
> > 
> > So even if it says GPL, its most likely not what the authors really mean.
> 
> He sells nProbe, a GPl'ed application he authors... nothing wrong there. 

Right. Except when he contradicts the GPL with placing additional restrictions,
say: "Note that for nProbe OEM reselling (including device embed) you need a
written commercial licence that's available on request from its author."
Perhaps he believes the GPL to be that restrictive, which I don't think it is.

> Please, this is a GPL'ed application and as such, subject to the terms and
> conditions of the GPL and should be evaluated as such. 

My point was that perhaps the author reads the GPL differently than everybody
else. A possible argument about whats on paper (GPL) and what the author means
("reselling (including device embed) you need a written commercial licence
that's available on request") is an invitation to lawyerland.


Comment 13 Michael J Knox 2006-07-02 19:40:48 UTC
You are talking about nProbe. If you are that concerned or feel strongly that
Luca is not compling to the GPL properly, in regards to nProbe, please take that
to him and/or FSF.. this review request is not the place to discuss license
concerns on applications not under this review.  

This is ntop, a freely available and GPL'ed application. 

Comment 14 Yanko Kaneti 2006-07-02 20:46:20 UTC
(In reply to comment #13)
> You are talking about nProbe. 

Not really. I am talking about the legally challenged upstream for ntop. How
much its relevant for the review of ntop? I'll leave that to the powers that be...

Comment 15 Michael J Knox 2006-07-02 20:57:34 UTC
OK, itemize your concerns. Instead of saying "legally challenged upstream" tell
me excatly what you believe the problem is with ntop. I will seek some
clarification from Luca. 

But at the end of the day though, the application is licensed under the terms
and conditions of the GPL. How the author reads the GPL is kind of irrelevent IMO. 

So, unless you can clarify you concern, I can't see how nProbes commerical
license affects the GPL'ed ntop. 

Comment 16 Yanko Kaneti 2006-07-02 22:13:32 UTC
I'll try this one last time, hopefuly this time it becomes clearer.
My one and only (but grave enough for me) concern with ntop is that it has an
upstream author/maintainer that cannot be trusted. nProbe and its licensing was
just an example of why I think so.

Comment 17 Michael J Knox 2006-07-04 05:59:23 UTC
I really don't get what your concern is.... 

He sells nProbe? So?? Some info I found for Harald Welte, who runs
gpl-violations.org

[»]  Re: GPL?!?
by Harald Welte - May 19th 2005 03:56:50

> I don't understand how this is GPL? Can
> anyone explain?

Of course it is GPL. You get the program licensed under GPL if you pay 99 EUR.
You then have the right to redistribute it modified or unmodified as often and
as long you please. So as soon as one buyer puts it's on his homepage, you will
be able to download it for free.

-- 

So can we please drop this rubbish about nProbe when this is a ntop review?????
If you have any further concerns on the licensing of nProbe email Luca!

Comment 18 Ralf Corsepius 2006-07-04 06:13:31 UTC
Michael, can we get back to business? If you're really interested in ntop, you
better start working on fixing this broken src.rpm.


Comment 19 Michael J Knox 2006-07-04 09:55:26 UTC
updates

Spec URL: http://www.knox.net.nz/~michael/ntop.spec
SRPM URL: http://www.knox.net.nz/~michael/ntop-3.2-4.src.rpm

Comment 20 Patrice Dumas 2006-07-05 21:30:40 UTC
The perl substitutions are unusefull: the first one is allready done in a 
patch. The second operates on a file that is regenerated. I couldn't 
find 'user =' in the main.c file. It is in prefs.c however. I think 
that it should be replaced with a patch it is more robust than a 
substitution (it fails more easily). There is also a nobody in 
webInterface.c. Also maybe a comment could explain why other users 
are used.

unix2dos may be replaced by
sed -i 's/\r//' 


Comment 21 Ralf Corsepius 2006-07-06 05:39:10 UTC
configure: error: GLib2 distribution not found.
error: Bad exit status from /var/tmp/rpm-tmp.31577 (%build)

Comment 22 Michael J Knox 2006-07-06 05:43:23 UTC
glib2-devel is listed as a BR... 

BuildRequires: openssl-devel, gdbm-devel, libpcap, rrdtool-devel, zlib-devel,
glib2-devel

Comment 23 Ralf Corsepius 2006-07-06 05:54:21 UTC
(In reply to comment #22)
> glib2-devel is listed as a BR... 
Yes, nevertheless building fails ... 
... but now that you say it, I noticed this to be a followup error to something
else:

# rpmbuild --rebuild ntop-3.2-4.src.rpm
...
checking for GLIB - version >= 2.0.0...
*** 'pkg-config --modversion glib-2.0' returned 2.10.3, but GLIB (1.2.10)
*** was found! If pkg-config was correct, then it is best
*** to remove the old version of GLib. You may also be able to fix the error
*** by modifying your LD_LIBRARY_PATH enviroment variable, or by editing
*** /etc/ld.so.conf. Make sure you have run ldconfig if that is
*** required on your system.
*** If pkg-config was wrong, set the environment variable PKG_CONFIG_PATH
*** to point to the correct configuration files
no
configure: error: GLib2 distribution not found.
error: Bad exit status from /var/tmp/rpm-tmp.42330 (%build)


Comment 24 Michael J Knox 2006-07-06 06:47:54 UTC
(In reply to comment #20)
> The perl substitutions are unusefull: the first one is allready done in a 
> patch. The second operates on a file that is regenerated. I couldn't 
> find 'user =' in the main.c file. It is in prefs.c however. I think 
> that it should be replaced with a patch it is more robust than a 
> substitution (it fails more easily). There is also a nobody in 
> webInterface.c. Also maybe a comment could explain why other users 
> are used.

Thanks, made a patch. This spec was imported from dag rpms, so I (wrongly)
assumed that the perl usage was correct. 
 
> unix2dos may be replaced by
> sed -i 's/\r//' 

Thanks, made that change. 

updates:

Spec URL: http://www.knox.net.nz/~michael/ntop.spec
SRPM URL: http://www.knox.net.nz/~michael/ntop-3.2-5.src.rpm

Comment 25 Michael J Knox 2006-07-06 07:07:47 UTC
(In reply to comment #23)
> (In reply to comment #22)
> > glib2-devel is listed as a BR... 
> Yes, nevertheless building fails ... 
> ... but now that you say it, I noticed this to be a followup error to something
> else:
> 
> # rpmbuild --rebuild ntop-3.2-4.src.rpm
> ...
> checking for GLIB - version >= 2.0.0...
> *** 'pkg-config --modversion glib-2.0' returned 2.10.3, but GLIB (1.2.10)
> *** was found! If pkg-config was correct, then it is best
> *** to remove the old version of GLib. You may also be able to fix the error
> *** by modifying your LD_LIBRARY_PATH enviroment variable, or by editing
> *** /etc/ld.so.conf. Make sure you have run ldconfig if that is
> *** required on your system.
> *** If pkg-config was wrong, set the environment variable PKG_CONFIG_PATH
> *** to point to the correct configuration files
> no
> configure: error: GLib2 distribution not found.
> error: Bad exit status from /var/tmp/rpm-tmp.42330 (%build)
> 


Looks like the patch for glib2 is not 100%... 

This builds in mock fine, so I would not call it a show stopper. I will review
the patch tonight all the same. 

Comment 26 Ralf Corsepius 2006-07-06 07:33:03 UTC
(In reply to comment #25)
> Looks like the patch for glib2 is not 100%... 
> 
> This builds in mock fine, so I would not call it a show stopper. 
I do, because this error implies rebuilding the package produces
non-deterministic results, esp. when users rebuild the package.


Comment 27 Patrice Dumas 2006-07-06 07:36:12 UTC
(In reply to comment #24)
> (In reply to comment #20)


> updates:
> 
> Spec URL: http://www.knox.net.nz/~michael/ntop.spec
> SRPM URL: http://www.knox.net.nz/~michael/ntop-3.2-5.src.rpm

Seems like you've uploaded the wrong file, there is a 
ntop-3.2-5.i386.rpm but no ntop-3.2-5.src.rpm...

Comment 28 Michael J Knox 2006-07-06 07:44:20 UTC
bugger.. Sorry.. I have just uploaded the src.rpm this time. 

Comment 29 Patrice Dumas 2006-07-06 10:36:10 UTC
More issues:

* plugins loaded twice

After some look at the code and some testing I have found that all
the plugins in the plugins directory are loaded, so they are loaded
twice since they appear twice in the package... In my opinion 
either the *.so without version number should be removed, or they
should be moved to *.so without version numbers.

* a plugin is not functionnal
**WARNING** Unable to load plugin /usr/lib/ntop/plugins/libxmldumpPlugin-3.2.so'
**WARNING** Message is '/usr/lib/ntop/plugins/libxmldumpPlugin-3.2.so: undefined
symbol: gdome_str_mkref'

And there is also another error, but it may only happen on the first run
**ERROR** LASTSEEN: Unable to open LsWatch database (/var/ntop/LsWatch.db)- the
plugin will be disabled

* many unneeded files in the docs: CONTENTS FILES ntop.txt README.Suse
INSTALL BUILD-MinGW.txt BUILD-NTOP.txt ntop-autotools.* PORTING 
RedHat-rpmbuild-HOWTO.txt, and certainly DAG

* should depend on logrotate

* less rpmbuild warnings with the following line in %setup
chmod -x docs/ntop-autotools.pdf *.c *.h plugins/*.c plugins/*.xml

* you should remove  /usr/lib/libntop.so, /usr/lib/libmyrrd.so and 
/usr/lib/libntopreport.so since there are no associated headers
and also since they use -release, see below.

* myrrd seems to be an old included version of rrdtools. It shouldn't
be used but instead linked against the system rrd, unless there is
a very good reason not to do so.

* upstream uses the -release for libtool using the package version,
this is wrong in general, since it trigggers a soname change even
when the abi don't change, however those libraries are not meant to
ne linked against, so if the *.so that don't have the release within
their names are not distributed it is right.

* The mechanisme described under PRIVACY NOTICE should be disabled in 
the default case (and reenabled with, for example --version-check)
or completly disabled.

* ntop doesn't seems to be interruptible by a control-C. It doesn't seems 
right to me but I may be wrong.
Maybe for a similar reason, when killing ntop on the first run when ntop 
asks for a password, the console is broken.

This last issue is not blocking the inclusion in extras in my opinion,
it is more for upstream.

Comment 30 Michael J Knox 2006-07-07 06:18:18 UTC
(In reply to comment #29)
> More issues:
> 
> * plugins loaded twice
> 
> After some look at the code and some testing I have found that all
> the plugins in the plugins directory are loaded, so they are loaded
> twice since they appear twice in the package... In my opinion 
> either the *.so without version number should be removed, or they
> should be moved to *.so without version numbers.

Ok, I will remove the plugins dangling in the /usr/lib directory. 

> * a plugin is not functionnal
> **WARNING** Unable to load plugin /usr/lib/ntop/plugins/libxmldumpPlugin-3.2.so'
> **WARNING** Message is '/usr/lib/ntop/plugins/libxmldumpPlugin-3.2.so: undefined
> symbol: gdome_str_mkref'

hrmm.. good spotting. 

> And there is also another error, but it may only happen on the first run
> **ERROR** LASTSEEN: Unable to open LsWatch database (/var/ntop/LsWatch.db)- the
> plugin will be disabled

Did you test to see if it re-occurs ? The file is present on my system. 

> * many unneeded files in the docs: CONTENTS FILES ntop.txt README.Suse
> INSTALL BUILD-MinGW.txt BUILD-NTOP.txt ntop-autotools.* PORTING 
> RedHat-rpmbuild-HOWTO.txt, and certainly DAG

I will remove the %doc doc/* line

> * should depend on logrotate

Done

> * less rpmbuild warnings with the following line in %setup
> chmod -x docs/ntop-autotools.pdf *.c *.h plugins/*.c plugins/*.xml

Done
 
> * you should remove  /usr/lib/libntop.so, /usr/lib/libmyrrd.so and 
> /usr/lib/libntopreport.so since there are no associated headers
> and also since they use -release, see below.

Removed
 
> * myrrd seems to be an old included version of rrdtools. It shouldn't
> be used but instead linked against the system rrd, unless there is
> a very good reason not to do so.

myrrd is indeed an older rrdtool, however, according to Luca, it has been
modified enough to warrant using it instead of the system provided rrdtool. I
will leave it with myrrd.
 
> * upstream uses the -release for libtool using the package version,
> this is wrong in general, since it trigggers a soname change even
> when the abi don't change, however those libraries are not meant to
> ne linked against, so if the *.so that don't have the release within
> their names are not distributed it is right.

I am not sure I follow you here. Could you clarify? 

> * The mechanisme described under PRIVACY NOTICE should be disabled in 
> the default case (and reenabled with, for example --version-check)
> or completly disabled.

I have disabled the version check in the init script. 

> * ntop doesn't seems to be interruptible by a control-C. It doesn't seems 
> right to me but I may be wrong.
> Maybe for a similar reason, when killing ntop on the first run when ntop 
> asks for a password, the console is broken.

A control-C does shut down ntop here. Its not instant, it performs a graceful
shutdown. 

> This last issue is not blocking the inclusion in extras in my opinion,
> it is more for upstream.

I am working on Ralf's glib concern at the moment, so I wont post the updated
srpm and spec. I have mostly corrected it, however, it still looks for
libglib.so incorrectly when building the xmldump plugin.

Thanks!

Comment 31 Ralf Corsepius 2006-07-07 06:33:50 UTC
(In reply to comment #30)
> (In reply to comment #29)

> > * upstream uses the -release for libtool using the package version,
> > this is wrong in general, since it trigggers a soname change even
> > when the abi don't change, however those libraries are not meant to
> > ne linked against, so if the *.so that don't have the release within
> > their names are not distributed it is right.
> 
> I am not sure I follow you here. Could you clarify? 

info libtool 'Release numbers'

=> using -release is bad idea.

Comment 32 Patrice Dumas 2006-07-07 15:38:05 UTC
(In reply to comment #30)
> (In reply to comment #29)

> Ok, I will remove the plugins dangling in the /usr/lib directory. 

I was speaking about the /usr/lib/ntop/plugins/ directory: every
file in this directory ending in .so is loaded.

> > And there is also another error, but it may only happen on the first run
> > **ERROR** LASTSEEN: Unable to open LsWatch database (/var/ntop/LsWatch.db)- the
> > plugin will be disabled
> 
> Did you test to see if it re-occurs ? The file is present on my system. 

It re-occured. I guess I know why, since for an unknown reason most
of the files in  /var/ntop/ belong to root, although I never started
ntop with -u root. Maybe this comes from the kills of ntop while it
asked for a password?


> myrrd is indeed an older rrdtool, however, according to Luca, it has been
> modified enough to warrant using it instead of the system provided rrdtool. I
> will leave it with myrrd.

Ok. But at least put a comment in the spec file. And you'll have to
watch out the rrdtool updates, especially the security issues...

> I have disabled the version check in the init script. 

It is still used during the required first run that sets the password.
I think it shouldn't.

> A control-C does shut down ntop here. Its not instant, it performs a graceful
> shutdown. 

Not when ntop is asking for a password. At least seems so to me. I'll
retest with the next srpm.

(and I get that with ntop -t 4 interrupted by control-C:

ven 07 jui 2006 17:37:06 CEST  RRD: Thanks for using the rrdPlugin
ven 07 jui 2006 17:37:06 CEST  RRD: Done
ven 07 jui 2006 17:37:06 CEST  CLEANUP: Freeing device eth0 (idx=0)
*** glibc detected *** ntop: free(): invalid next size (normal): 0x0962c2d8 ***
======= Backtrace: =========
/lib/libc.so.6[0x1fec88]
/lib/libc.so.6(__libc_free+0x78)[0x20216f]
/usr/lib/libntop-3.2.so(ntop_safefree+0x2c)[0x935d4c]
/usr/lib/libntop-3.2.so(cleanup+0xf13)[0x937953]
[0xb7f08420]
/usr/lib/libntop-3.2.so(_ntopSleepWhileSameState+0x2f)[0x95b09f]
ntop[0x804b12f]
/lib/libc.so.6(__libc_start_main+0xdc)[0x1b0794]
ntop[0x8049d31]
======= Memory map: ========
Abandon
) 

Comment 33 Michael J Knox 2006-07-27 08:18:06 UTC
OK, updated ntop. 

Dropped the effort to make it work with Glib2, this should fix what Ralf was
seeing.  

Spec URL: http://www.knox.net.nz/~michael/ntop.spec
SRPM URL: http://www.knox.net.nz/~michael/ntop-3.2-6.src.rpm

Comment 34 Kevin Fenzi 2006-08-27 21:40:19 UTC
Ralf or Patrice: Do either of you intend to formally review this package? 
If not, I would be happy to start in on a formal review... 

Comment 35 Patrice Dumas 2006-08-27 21:46:44 UTC
Please do. I don't know how much time I have, so I'll do
some testing if I have time.

Comment 36 Kevin Fenzi 2006-08-27 23:12:24 UTC
OK - Package name
OK  - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:
cd29a876b34a7dd76555e9acd8f160bb  ntop-3.2.tgz
cd29a876b34a7dd76555e9acd8f160bb  ntop-3.2.tgz.1
OK - Package compiles and builds on at least one arch.
See below - BuildRequires correct
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package doesn't own any directories other packages own.
See below - No rpmlint output.
SHOULD Items:
OK - Should include License or ask upstream to include it.
 - Should build in mock.
See below - Should have sane scriptlets.

Issues:

1. Oddly the web site ( http://www.ntop.org/ ) doesn't mention the sourceforge
src download. It points only to their CVS repository for getting the source.
Is the sourceforge download official for upstream? Perhaps just a bug in their
download page on the web site?
They do mention in the CVS FAQ file:
http://prdownloads.sourceforge.net/n/nt/ntop/

2. Doesn't build under mock/devel. The libpcap BuildRequires should be
libpcap-devel ? (Note that this changed between fc5 and devel)

3. Why the post and postun calls to ldconfig? If ntop does dlopen directly on 
the
.so files, there should be no need to call ldconfig. Also, if that is the case
perhaps the .so files shouldn't be polluting libdir? I removed the non versioned
files and ntop starts fine, so I think we can remove:
/usr/lib/libntop.so
/usr/lib/libmyrrd.so
/usr/lib/libntopreport.so

4. rpmlint says:
E: ntop non-standard-uid /var/ntop ntop
E: ntop non-standard-dir-perm /var/ntop 0775
Those can be ignored.
W: ntop devel-file-in-non-devel-package /usr/lib/libntop.so
W: ntop devel-file-in-non-devel-package /usr/lib/libmyrrd.so
W: ntop devel-file-in-non-devel-package /usr/lib/libntopreport.so
                                                                                          
I think those can be removed.
W: ntop mixed-use-of-spaces-and-tabs
Cosmetic, but would be nice to fix up.
E: ntop-debuginfo script-without-shellbang /usr/src/debug/ntop-3.2/fcUtils.c
E: ntop-debuginfo script-without-shellbang /usr/src/debug/ntop-3.2/globals-
structtypes.h
Permissions on those files should be 644?

5. There is currently nothing in the plugins directory. Do you intend to
ship no plugins at all?

6. You need to run ntop "manually" the first time to set the password.
Would there be some way to detect this in the init script and print a
warning and tell the user exactly what they need to run?

7. Starting up after setting the password results in:
/sbin/service ntop start
Starting ntop daemon:    Processing file /etc/ntop.conf for parameters...
Sun Aug 27 16:54:45 2006  NOTE: Interface merge enabled by default
Sun Aug 27 16:54:45 2006  Initializing gdbm databases
NOTE: --use-syslog, no facility specified, using default value.  Did you forget 
the =?
                                                           [  OK  ]
Can that be redirected to the log or /dev/null? init scripts shouldn't
print verbose information to the starting console.

8. Instead of removing the .a files you could just pass '--disable-static'
to configure. Possibly also enable: --enable-snmp ?


Comment 37 Patrice Dumas 2006-08-27 23:50:32 UTC
* Among the rpmling warning there is:
W: ntop devel-file-in-non-devel-package /usr/lib/libntop.so
W: ntop devel-file-in-non-devel-package /usr/lib/libmyrrd.so
W: ntop devel-file-in-non-devel-package /usr/lib/libntopreport.so

I think those files shouldn't be shipped.

* Seems like you removed too much in the plugins directory, 
there aren't plugins anymore...

I propose something along
for file in %{buildroot}/%{_libdir}/%{name}/plugins/*.so; do
  if test -L $file; then
     base=`basename $file .so`
     mv %{buildroot}/%{_libdir}/%{name}/plugins/$base-%{version}.so $file
  fi
done




The proper fix should be done upstream, by changing, for each 
plugin:
libicmpPlugin_la_LDFLAGS = -shared -release @PACKAGE_VERSION@ @DYN_FLAGS@
into
libicmpPlugin_la_LDFLAGS = -module -avoid-version @DYN_FLAGS@
Strangely, -shared doesn't seems to be a libtool option. It
is certainly unneeded.

* there are still bad perms, that should be fixed in %setup:

chmod -x docs/ntop-autotools.pdf *.c *.h plugins/*.c

* --enable-tcpwrap should be replaced by --with-tcpwrap

* you could add to the configue call: --disable-dependency-tracking

* there is still a dependency on logrotate missing

* some doc files are missing: README in source directory, and
in docs: 1STRUN.txt, KNOWN_BUGS, README, FAQ*, BUG_REPORT
some shouldn't be packaged (INSTALL, CONTENTS)

* during the first run with -t 5, I get:
Mon Aug 28 01:31:32 2006 [MSGID0026798] **WARNING** ASN: Unable to open file
'AS-list.txt'

Also ntop cannot be interrupted during that first run: hitting 
ctrl-C does nothing while ntop is prompting:

Please enter the password for the admin user: 

I can kill ntop from another console but then the first one
is broken

* there is still
lun 28 aoû 2006 01:36:52 CEST [MSGID0757670] **WARNING** Message is
'/usr/lib/ntop/plugins/libxmldumpPlugin.so: undefined symbol:
gdome_doc_documentElement'

Otherwise it seems to work.

Comment 38 Patrice Dumas 2006-08-27 23:58:28 UTC
2 other remarks:
ntop segfaults when interrupted by ctrl-C when launched on
the console.

in /var/ntop many files belong to root. I don't know if it
is right or wrong.

Comment 39 Michael J Knox 2006-09-16 20:37:23 UTC
Sorry. Due to my stepping out for a while, I am unable to complete this submission. 

Comment 40 Bernard Johnson 2006-12-09 14:28:18 UTC
I started a new package review with bug #219025.  The FE instruction say to mark
a FE-DEADREVIEW bug as a duplicate of the new bug when you take it over, but I
don't have the permissions.

Unfortunately, I had to create mine from scratch as the prior srpm and spec file
had been taken offline.

Comment 41 Patrice Dumas 2006-12-09 16:50:58 UTC
I have uploaded the latest srpm here:
http://www.environnement.ens.fr/perso/dumas/fc-srpms/ntop-3.2-6.src.rpm

Comment 42 Kevin Fenzi 2006-12-09 17:36:56 UTC

*** This bug has been marked as a duplicate of 219025 ***

Comment 43 Bernard Johnson 2006-12-09 22:49:29 UTC
Thanks Patrice, I'll look through it and integrate any prior work into my package.