This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 441378 - Review Request: smokeping - Latency Logging and Graphing System
Review Request: smokeping - Latency Logging and Graphing System
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Extras Quality Assurance
:
: 187326 758443 (view as bug list)
Depends On: 461565 465710
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-07 15:39 EDT by Terje Røsten
Modified: 2011-12-02 08:01 EST (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-27 16:50:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wolfy: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Terje Røsten 2008-04-07 15:39:33 EDT
Spec URL: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
SRPM URL: http://terjeros.fedorapeople.org/smokeping/smokeping-2.3.5-1.fc8.src.rpm

Description:

SmokePing is a latency logging and graphing system. It consists of a
daemon process which organizes the latency measurements and a CGI
which presents the graphs.


Note rrdtool is currently broken in rawhide:

 https://bugzilla.redhat.com/show_bug.cgi?id=441359

making testing there somewhat difficult, however fc8 works.
Comment 1 Nicolas A. Corrarello 2008-04-07 16:11:53 EDT
rpmlint says smokeping.src: E: unknown-key GPG#7666df64

Otherwise is ok on F8
Comment 2 Terje Røsten 2008-04-07 16:20:13 EDT
> rpmlint says smokeping.src: E: unknown-key GPG#7666df64

Yeah, my build scripts is adding a internal only gpg key.


 
Comment 3 Jason Tibbitts 2008-04-07 19:25:53 EDT
*** Bug 187326 has been marked as a duplicate of this bug. ***
Comment 4 Terje Røsten 2008-04-08 12:49:34 EDT
FYI: #441359 blocking testing in rawhide is now fixed.

Comment 5 Jason Tibbitts 2008-07-02 22:28:11 EDT
Would you consider updating to 2.4.1  and fixing up the URL and download URLs to
point to their current locations?
Comment 7 manuel wolfshant 2008-08-25 16:55:09 EDT
I am not sure how did you manage to build in koji, but for me it failed because the new rpm is picky (needs %Patch0 even where before %Patch was enough : http://koji.fedoraproject.org/koji/getfile?taskID=785303&name=build.log )

Now I am going to be picky, too: README.Fedora includes the following sentence "Fix you want to access from a remote host [...]". I can swear this is not proper English, even if all the words are in English :)

I'll try to give it a spin one of these days and come back with a review, unless someone beats me to it.
Comment 9 manuel wolfshant 2008-08-30 19:23:42 EDT
What's the reasoning behind using 
  %attr(2775, root, apache) %{_localstatedir}/lib/%{name}/images
and not
  %attr(755, apache, root) %{_localstatedir}/lib/%{name}/images
?

This will allow apache to write in the images directory (as required by the cgi) and avoid using any suid.


Look for a full review soon.
Comment 10 manuel wolfshant 2008-08-30 19:54:29 EDT
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM:E: unknown-key GPG#7666df64
--> ignoreable
binary RPM:
smokeping.noarch: W: non-standard-gid /var/lib/smokeping/images apache
-> needed, the cgi provided by smokeping must be able to create apache readable images
smokeping.noarch: E: non-standard-dir-perm /var/lib/smokeping/images 02775
--> needed, in order to have the provided CGI create images, but avoidable by changing the owner/group settings
smokeping.noarch: E: non-readable /etc/smokeping/smokeping_secrets 0640
--> needed, the file contains passwords
 [x] Package is not relocatable.
 [x] Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [!] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
--> see issue 1
 [!] License field in the package spec file matches the actual license.
     License type: see issue 1
 [x] If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     SHA1SUM of package: 055d65c7e3c49cd0d6e8f96242131fe69dc3110e  /tmp/smokeping-2.4.2.tar.gz
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [!] Permissions on files are set properly.
--> see issue 2
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: all archs suported by koji scratch build
 [x] Package should compile and build into binary rpms on all supported architectures.
     Tested on: all archs suported by koji scratch build
 [x] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.


=== Issues ===
1. Smokeping itself is GPLv2+ but it relies and includes several modules which either have no license specified (/usr/share/smokeping/lib/Smokeping/probes/FPing.pm, /usr/share/smokeping/lib/Smokeping/probes/EchoPingSmtp.pm and many many others) or claim to be under the artistic license (/usr/share/smokeping/lib/BER.pm, /usr/share/smokeping/lib/SNMP_Session.pm, /usr/share/smokeping/lib/SNMP_util.pm). Unfortunately, despite the fact the sources claim that the ""Artistic License" is included, the tar file does not really included it. AFAIK the original Artistic license is not acceptable in Fedora, so I must ask you to verify that all the included modules do have an acceptable license.
2. The permissions/ownership as proposed are functional and acceptable, but I see no reason to use 2775 for the folder where smokeping writes the images. I think that using apache.root as owner/group and 755 as modes will work just fine. Please correct me if I am wrong.
Comment 11 Yanko Kaneti 2008-08-30 20:16:00 EDT
The original tarball includes a private copy/version of files from at least perl-CGI-Session, perl-SNMP_Session, perl-JSON. And it seems the rpm produced with this spec does too.

This should not be allowed without a really good reason.
Comment 12 manuel wolfshant 2008-08-30 23:11:10 EDT
Good catch, Yanko! I've re-analyzed the perl modules that are included and the following ones are private copies of CPAN modules and should no be included:
- CGI::Session
- Digest:HMAC,Digest:HMAC_MD5, Digest::HMAC_SHA1
- JSON + submodules (note that the latest version available in CPAN is 2.12, while smokeping includes 1.14; according to it's documentation, 2.12 includes incompatibilities towards the older version)
- qooxdoo is included in the binary rpm, but qooxdoo/README.txt from the source files claims it is only needed for development, not for runtime

SNMP_Session and SNMP_Util seem to be own implementations, not the CPAN versions. I could not find their author, Simon Leinen, in http://search.cpan.org/search?m=author&q=+Simon+Leinen
Comment 13 manuel wolfshant 2008-08-31 19:46:52 EDT
The qooxdoo/README.txt seems to be misleading. SmokeTrace contains the following lines:
use Qooxdoo::JSONRPC;
Qooxdoo::JSONRPC::handle_request ($cgi, $session);

So it seems that
  /usr/share/smokeping/lib/Qooxdoo
  /usr/share/smokeping/lib/Qooxdoo/JSONRPC.pm
  /usr/share/smokeping/lib/Qooxdoo/Services
  /usr/share/smokeping/lib/Qooxdoo/Services/Tr.pm
should be made available, but I think that is should exist as a separate package. Fortunately it is licensed as LGPL so it is compatible with the GPLv2+ of smokeping.
Comment 14 manuel wolfshant 2008-08-31 20:20:10 EDT
Could you please add to the README.fedora file a couple of lines indicating how to configure the system to allow for smoketrace usage?
a) the traceroute binary must be suid (chmod u+s ), otherwise the cgi will display an errror message ("The specified type of tracerouting is allowed for superuser only") and bail out.
b) selinux might also complain with errors similar to:
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "net_raw" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l 5e473b24-c127-4c3f-903d-510ead101bec
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "connect" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l e9b14398-4979-43c9-8ea3-9ca46bae5034
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "rawip_send" to <Unknown> (node_t).      For complete SELinux messages. run sealert -l 1185b418-256b-4340-983c-e5a071804fe8
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "node_bind" to <Unknown> (inaddr_any_node_t).      For complete SELinux messages. run sealert -l 2c034994-cf52-4993-aa97-c6d28bba3562
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "setopt" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l 3ca926de-8227-4473-81c3-3d50dce61550
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing /bin/traceroute (httpd_t) "read" to <Unknown> (httpd_t).      For complete SELinux messages. run sealert -l 56c93fca-006d-40e6-88b6-72492f474042
Sep  1 03:11:52 wolfy setroubleshoot:      SELinux is preventing setroubleshootd (httpd_t) "rawip_recv" to <Unknown> (node_t).      For complete SELinux messages. run sealert -l ab9af5f0-874d-4521-9d2b-3a314af3f8dd

However the selinux messages suddenly disappeared, only that I am not yet sure what I did to fix...
Comment 15 Terje Røsten 2008-09-08 11:48:52 EDT
Ok, some issues here...

1) I will removed the CGI, JSON and Digest modules
2) I am tempted to drop smoketrace support, I don't see the big value over
   the very common traceroute command line tool. 
   I don't see this issue as blocker. Do you?
3) Will send a mail to Simon Leinen about the licenses on the SNMP* files
Comment 16 manuel wolfshant 2008-09-08 12:59:49 EDT
1) I am tempted to say "OK, but...". If those perl modules are not already available, they should be packaged and smokeping should be built against them. Perl is not my favorite subject but should you submit any of these as separate packages I'll try to help with the review.
2) I will not object, but it's a "nice to have".  The idea is that you get a traceroute issued via the web interface from a system to which the users might not have other forms of access. It's not so difficult to configure the system to allow it. Up to you to decide.
3) excellent
Comment 17 Terje Røsten 2008-09-08 17:28:28 EDT
3) SNMP_Session, SNMP_Util and BER is already in Fedora under a Artistic 2.0 license. spot saw the problem some time ago and asked for the license change.

Upstream is here:
 http://www.switch.ch/misc/leinen/snmp/perl/

Thanks to Simon for providing this information.

I need some time to rearrange the package and do some testing.
Comment 18 Andreas Thienemann 2008-09-08 17:44:59 EDT
This package needs a lot of work, specifically patching the perl code to be packageable in a sane way.

We did this for an in-house installation of smokeping in the past. If you're interested, I can see if we still have the spec file.
Comment 19 Terje Røsten 2008-09-09 04:20:20 EDT
Smokeping need Config::Grammar, submitted for review.
Comment 20 Terje Røsten 2008-09-15 14:56:42 EDT
New update fixing most issues:

o all external modules removed (however not Qooxdoo::JSONRPC, noted in README.fedora)

o README.fedora improved

o smoketrace support

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://koji.fedoraproject.org/koji/getfile?taskID=826553&name=smokeping-2.4.2-3.fc10.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=826553

Thanks so far guys, the package has improved a lot.
Comment 21 manuel wolfshant 2008-09-16 11:09:43 EDT
It looks better now, but I have found a few small issues. And I also have a question about which I am not sure.
a) perl has become an useless BR, because it is brought in by perl(Grammar);
b) glibc-common is definitely not needed as BR;
c) you use lots of macros in the {__xx} form, but "mv" is used as such. Nor a blocker neither incorrect, but not consistent from a stylish point of view.
d) in the lines
   %{__rm} -rf in lib/{CGI,Config,Digest,JSON}
   %{__rm} -rf in lib/{SNMP_Session,SNMP_util,BER,JSON}.pm

 I assume there is a confusion or a leftover from using find, as rm does not need or use the "in" argument. rm -fR in lib/*.. does not fail because of the -f flag, but you should remove the word "in" from these lines


Now the preamble of the question: as far as I can see, the newer rpm (2.4.2-3) removed the requires for
 perl(B)
 perl(CGI::Session::Driver)
 perl(CGI::Session::Driver::DBI)
 perl(CGI::Session::ErrorHandler)
 perl(Class::Struct)
 perl(DBD::Pg)
 perl(DB_File)
 perl(DBI)
 perl(Digest::HMAC)
 perl(Digest::SHA1)
 perl(File::Spec)
 perl(FreezeThaw)
 perl(overload)
 perl(Scalar::Util)
 perl(Test::More)

I do not have the infrastructure on which to test and my perl-fu is rather weak, so I have to rely to an answer from others: is all the previous functionality preserved ?
Comment 22 Terje Røsten 2008-09-16 14:57:25 EDT
> a) perl has become an useless BR, because it is brought in by perl(Grammar);

perl(Grammar) is req, not br? 
perl is not used in building anyway, removed.

> b) glibc-common is definitely not needed as BR;

needed for iconv (or do you mean glibc-common is always available?)

> c) you use lots of macros in the {__xx} form, but "mv" is used as such. Nor a
> blocker neither incorrect, but not consistent from a stylish point of view.

fixed.

> d) in the lines
>    %{__rm} -rf in lib/{CGI,Config,Digest,JSON}
>    %{__rm} -rf in lib/{SNMP_Session,SNMP_util,BER,JSON}.pm

cut-n-paste error, fixed.

> Now the preamble of the question: as far as I can see, the newer rpm (2.4.2-3)
> removed the requires for
>  perl(B)
>  perl(CGI::Session::Driver)
>  perl(CGI::Session::Driver::DBI)
>  perl(CGI::Session::ErrorHandler)
>  perl(Class::Struct)
>  perl(DBD::Pg)
>  perl(DB_File)
>  perl(DBI)
>  perl(Digest::HMAC)
>  perl(Digest::SHA1)
>  perl(File::Spec)
>  perl(FreezeThaw)
>  perl(overload)
>  perl(Scalar::Util)
>  perl(Test::More)
> 
> I do not have the infrastructure on which to test and my perl-fu is rather
> weak, so I have to rely to an answer from others: is all the previous
> functionality preserved ?

With all the external modules removed this is not surprising, these requirements
are now pushed into the modules smokeping depends on (I hope :-).

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://koji.fedoraproject.org/koji/getfile?taskID=828581&name=smokeping-2.4.2-4.fc10.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=828580
Comment 23 manuel wolfshant 2008-09-16 18:36:47 EDT
ref glibc-common: it is safe to assume it is always present in the buildroot. how many systems or build logs have you seen which work without THE core set of libs ?

except for the Qooxdoo problem (it is still a private lib and the guidelines forbit that), everything else seems fine to me. I think it's time to call for external help, because I am not sure how to proceed. Strictly following the rules would impose that the older Qooxdoo should be made available as a package and become a "Require" of smokeping
Comment 24 manuel wolfshant 2008-09-16 19:24:14 EDT
I think it's so easy to comply with the packaging rules that we really should do it.

The only required thing to do is to package JSONRPC.pm from http://downloads.sourceforge.net/qooxdoo/qooxdoo-0.7.3-backend.zip (linked from http://qooxdoo.org/download_0_7 ). Just name it qooxdoo-backend-0.7 or qooxdoo-backend-compat or something similar ( in case someone packages version 0.8 which is not directly API compatible), make sure it provides JSONRPC.pm and I think that is all.

Opinions, anyone ?
Comment 25 Terje Røsten 2008-10-05 16:54:11 EDT
Updated package:

- move Qooxdoo::JSONRPC to separate package, see #465710

spec: http://terjeros.fedorapeople.org/smokeping/smokeping.spec
srpm: http://terjeros.fedorapeople.org/smokeping/smokeping-2.4.2-5.fc9.src.rpm
koji: N/A, koji seems to be down.
Comment 26 Terje Røsten 2008-10-06 14:30:35 EDT
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=864417
Comment 29 manuel wolfshant 2008-10-20 15:43:07 EDT
I am sorry for the delay, Terje. I'll finish the review ASAP. Please bear with me, I have $WORK stuff to finish first
Comment 30 Terje Røsten 2008-10-20 16:19:07 EDT
No problemo, I can wait.
Comment 31 manuel wolfshant 2008-10-24 06:52:34 EDT
Excellent, everything seems OK, package is APPROVED


However, for a smoother experience, please add to the README.fedora file that SELinux will prevent by default smokeping from working, unless a couple of commands are used to change the context. My preliminary tests in rawhide have shown that the following two commands are needed at least:
   chcon -v -R --type=httpd_sys_script_exec_t /usr/share/smokeping/cgi/
   chcon -v -R --type=httpd_sys_content_t /var/lib/smokeping/
A few other rules are needed, because I still get some errors. audit2allow says the following policy is needed:

module smokeudpsocket 1.0;

require {
        type httpd_sys_script_t;
        class udp_socket connect;
}

#============= httpd_sys_script_t ==============
allow httpd_sys_script_t self:udp_socket connect;


As my config is very small and uses very few of smokeping's facilities, I am almost sure other rules are also needed.
Comment 32 Terje Røsten 2008-10-24 11:13:10 EDT
Thanks!

Will add SElinux info to README.



New Package CVS Request
=======================
Package Name: smokeping
Short Description: Latency Logging and Graphing System
Owners: terjeros
Branches: F-9
InitialCC:
Comment 33 manuel wolfshant 2008-10-24 11:24:24 EDT
For the record: I am using it for several months on F7 without any issues, so I guess (provided the deps are met), F-8 is also a branch which could make use of this package.
Comment 34 Terje Røsten 2008-10-24 11:35:08 EDT
Ok, adding F-8.

New Package CVS Request
=======================
Package Name: smokeping
Short Description: Latency Logging and Graphing System
Owners: terjeros
Branches: F-8 F-9
InitialCC:
Comment 35 Dennis Gilmore 2008-10-27 00:36:10 EDT
CVS Done
Comment 36 Terje Røsten 2008-10-27 16:50:03 EDT
Imported and built.
Comment 37 Jeff Sheltren 2011-12-01 16:32:45 EST
Package Change Request
======================
Package Name: smokeping
New Branches: el5 el6
Owners: micklweiss sheltren
Comment 38 Mick Weiss 2011-12-01 16:51:40 EST
*** Bug 758443 has been marked as a duplicate of this bug. ***
Comment 39 Jon Ciesla 2011-12-02 08:01:18 EST
Git done (by process-git-requests).

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