Bug 241654 - Review Request: ipcalculator - A utility for computing broadcast, network, mask, and host ranges
Review Request: ipcalculator - A utility for computing broadcast, network, ma...
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Marek Mahut
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2007-05-29 07:58 EDT by Jakub Hrozek
Modified: 2008-02-12 07:15 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-02-12 07:15:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mmahut: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Jakub Hrozek 2007-05-29 07:58:55 EDT
Spec URL: http://www.volny.cz/jhrozek/ipcalc/ipcalc.spec
SRPM URL: http://www.volny.cz/jhrozek/ipcalc/ipcalc-0.41-1.fc7.src.rpm
ipcalc takes an IP address and netmask and calculates the resulting
broadcast, network, Cisco wildcard mask, and host range. By giving a second
netmask, you can design subnets and supernets. It is also intended to be
a teaching tool and presents the subnetting results as easy-to-understand
binary values.
Comment 1 manuel wolfshant 2007-05-29 08:13:56 EDT
Fedora already includes an ipcalc utility (in initscripts). Trying to install 
another application with the very same name (and location, /bin/ipcalc) will
lead to confusions and conflicts.
Comment 2 Jakub Hrozek 2007-05-29 08:38:32 EDT
Ouch, you are right, I was not aware of the initscripts ipcalc..

One minor thing, this ipcalc installs its binary into /usr/bin/ipcalc, 
not /bin/ipcalc, so these two would not slap on one another, but one would 
have to invoke the binary with full path, which is quite ugly..

Would renaming of the package/binary be allowed? If not, I'm OK with 
withdrawing this package.
Comment 3 manuel wolfshant 2007-05-29 08:40:22 EDT
Sure, renaming is fine
Comment 5 Philippe Valembois 2007-05-29 15:34:25 EDT
Package doesn't follow guidelines : 
Comment 6 Jakub Hrozek 2007-06-02 06:34:58 EDT
* Sat Jun 1 2007 Jakub Hrozek <jhrozek@redhat.com> - 0.41-3
-move the cgi script outside /var/www

Comment 7 Patrice Dumas 2007-06-02 07:07:38 EDT
The sed substitution may also be along:

sed -i 's:/usr/local/bin/ipcalc/:%{_bindir}/ipcalculator:' \

Regarding the package name, this deviation is accepted in the 
guidelines since it adds a best judgement clause. And in that 
case indeed there could be confusion with the other ipcalc.

You should own the

You should also patch the program to use ipcalculator instead of 
ipcalc in help message. Unless I am wrong it could also be achieved

sed -e 's/ipcalc /%{name} /'

I know it is a small package, but you may want to split the cgi 
part and the main script anyway since they correspond with very 
different uses in my opinion. Also, especially in case you have 
a -cgi subpackage, you may consider shipping a config file for 
apache, as doc or in /etc/httpd/conf.d such that the cgi works
more easily out of the box. Having a config file for httpd already
installed has advantages (works out of the box) and inconvenients 
(may create security issues if it isn't required to change something 
manually by the local admin). All that is optional and left to your
best judgement.
Comment 8 manuel wolfshant 2007-06-02 17:37:28 EDT
Something similar to the below lines should do for
/etc/httpd/conf.d/ipcalculator.conf (beware: not tested)

ScriptAlias /ipcalculator/ /usr/share/ipcalculator/
<Directory /usr/share/ipcalculator/>
        DirectoryIndex ipcalculator.cgi
        Options ExecCGI
        order deny,allow
        deny from all
        allow from

Comment 9 Jakub Hrozek 2007-06-02 18:10:09 EDT
Patrice, Manuel,
Thanks for the valuable suggestions. I'll fix the issues pointed out in 
comment #7, also the split seems like a good idea, thanks for proposing the 
httpd config file.

But it might take a few days as I'm currently swamped with other things (a.k.a 
it's exam time on universities :) ), please don't take it as I'm abandoning 
the review and/or ignoring your suggestions.
Comment 10 Jakub Hrozek 2007-06-16 15:48:53 EDT
* Sat Jun 16 2007 Jakub Hrozek <jhrozek@redhat.com> - 0.41-4
- split the CGI wrapper into its own -cgi subpackage
- nicer sed substitution in the CGI module
- own the /usr/share/ipcalculator directory
- add a config file for httpd (written by Manuel Wolfshant)
- mention ipcalculator, not ipcalc in usage and help messages


One question - is it necessary to have some docs in the -cgi subpackage even
though it requires the ipcalculator package which contains the necessary docs?
(rpmlint gripes about that)
Comment 11 Patrice Dumas 2007-06-16 19:48:59 EDT
(In reply to comment #10)

> One question - is it necessary to have some docs in the -cgi subpackage even
> though it requires the ipcalculator package which contains the necessary docs?
> (rpmlint gripes about that)

You don't have to worry about that rpmlint warning. In general rpmlint
errors and warnings may be ignored when they are pointless, as it is
the case here.
Comment 12 Patrice Dumas 2007-11-28 13:47:59 EST
I cannot access the srpm... I can't remember why I didn't
finished that review...
Comment 13 Jakub Hrozek 2007-11-28 14:02:29 EST
srpm re-uploaded.
Thanks for your time taking a look at my package!
Comment 14 Patrice Dumas 2007-11-28 17:13:15 EST
The perl(CGI) requires seems to be only for -cgi, and autodetected.

Similarly the perl requires is autodetected.

Shouldn't the cgi subpackage depend on httpd?

GPL isn't a valid license anymore, it is GPL+, GPLv2, GPLv2+...

The source archive timestamp isn't kept:
-rw-rw-r-- 1 dumas dumas 21599 jui 27  2006 ipcalc-0.41.tar.gz
-rw-rw-r-- 1 dumas dumas 21599 mai 29  2007 ../SOURCES/ipcalc-0.41.tar.gz

You can use wget -N, spectool -g and the corresponding curl option.

There are issues with the package as I tested. First the images
are considered as scripts with the conf file Manuel proposed. I
propose the following:

Alias /ipcalculator/ /usr/share/ipcalculator/
<Directory /usr/share/ipcalculator/>
    DirectoryIndex ipcalculator.cgi
    Options ExecCGI
    AddHandler cgi-script cgi
    order deny,allow
    deny from all
    allow from

The images should be owned by the -cgi subpackage.

There is a missing image files, bg.gif.  ipcalc.gif should be 
renamed ipcal03.gif.

<a href="ipcalc.png">Screenshot</a> (ipcalc works also at the prompt)<br>
should be changed to
<a href="http://jodies.de/ipcalc.png">Screenshot</a> (ipcalc works also at the
Similar with
<a href="ipcalc-archive">Archive</a><br>
<a href="ipcalc-faq/win32.html">How to run this under windows</a><br>

use %{version} in 
Source0:        http://jodies.de/ipcalc-archive/ipcalc-%{version}.tar.gz
Comment 15 Mamoru TASAKA 2007-12-15 23:59:34 EST
Any progress on here?
Comment 16 Jakub Hrozek 2007-12-16 09:40:55 EST
Thanks for poking me, Mamoru! I totally forgot about this one..


The bg.gif image is still not included, as I'd like to use the one at the
upstream page, so I queried the author about including it. 
Comment 17 Patrice Dumas 2007-12-16 13:17:21 EST
The sed is wrong:
sed -i 's:/usr/local/bin/ipcalc/:%{_bindir}/ipcalculator:' ipcalc.cgi
should be
sed -i 's:/usr/local/bin/ipcalc:%{_bindir}/ipcalculator:' ipcalc.cgi
Comment 18 Jakub Hrozek 2007-12-26 15:00:55 EST

* Wed Dec 26 2007 Jakub Hrozek <jhrozek@redhat.com> - 0.41-6
- fix a typo in the sed substitution
- add the background image from the project page
Comment 19 Marek Mahut 2008-02-10 13:21:04 EST

All ok for me. 

Comment 20 Patrice Dumas 2008-02-10 13:47:16 EST
Looks good to me too.
Comment 21 Jakub Hrozek 2008-02-11 16:56:01 EST
New Package CVS Request
Package Name: ipcalculator
Short Description: A utility for computing broadcast, network, mask, and host ranges
Owners: jhrozek
Branches: F-7 F-8
InitialCC: none
Cvsextras Commits: yes
Comment 22 Kevin Fenzi 2008-02-11 21:35:41 EST
cvs done.
Comment 23 Jakub Hrozek 2008-02-12 07:15:19 EST
Built for rawhide:

Patrice, Manuel, Marek - thanks for the work done on this review!

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