[Bug 495579] Review Request: ifstatus - Command Line real time interface graphs using ncurses

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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





--- Comment #7 from Robert Scheck <redhat-bugzilla at linuxnetz.de>  2009-04-13 19:21:15 EDT ---
1. Personally I would prefer the group "Applications/Internet", as tcpdump, 
   tcpick and iftop are very closed to the functionality (especially iftop)
   and they've the group "Applications/Internet" (see comment #6)

2. Building your package in Rawhide fails for several reasons:
   a) Upstream has written not really C++ compliant code and lacks several
      #include lines (see comment #3, #4)
   b) Upstream has a non autotools generated makefile which lacks some needed
      functionality. I added the missing functionality and corrected a few
      things from your patch more etc. (see comment #5)
   c) Please write upstream an e-mail and explain, that these patches (or any
      better) have to make it upstream and should be included into the next
      release of ifstatus. Please set me on Cc: at that e-mail, thank you.

3. "Requires: ncurses" is not needed, because "Requires: libc.so.6()(64bit) 
   libc.so.6(GLIBC_2.2.5)(64bit) libgcc_s.so.1()(64bit) 
   libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libncurses.so.5()(64bit) 
   libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit) libtinfo.so.5()(64bit) rtld(GNU_HASH)"
   from "rpm -qp --requires ifstatus-1.1.0-2.fc10.x86_64.rpm" already lists
   "libncurses.so.5", which makes a runtime dependency to ncurses, that will
   be resolved e.g. by yum (see comment #6)

4. "make CFLAGS=%{?_smp_mflags}" looks strange - CFLAGS and %_smp_mflags are
   two completely different things. CFLAGS are the compiler flags passed to
   the makefile while %_smp_mflags are SMP flags expanded into "-j2" for e.g.
   parallel builds during %build.

   As per my patch from comment #5 the $RPM_OPT_FLAGS are already honored to
   CFLAGS, just using "make %{?_smp_mflags}" is enough (see comment #6)

5. "make install DESTDIR=%{buildroot}%{_bindir}/%{name}" would have worked
   with the original patch, but I think, it's better to less touch upstream
   work and (sorry) my patch is hopefully near to get accepted by upstream.
   Thus replaced by a hopefully better construct (see comment #6)

6. As far as I can see, ifstatus requires either root permissions or the +s
   option (suid bit) to work. For security reasons, it's better, if only root
   can execute the binary in case of a security issue at ifstatus (this is
   how you've packaged it now - fine). And as it doesn't make sense to have
   root-only executable binaries in %{_bindir}, I'm hereby putting ifstatus 
   into %{_sbindir} to satisfy that (see comment #6)

Any objections to my suggestions and explanations so far? Rest looks fine so
far to me, but the formal/official review is still missing. I will do that,
once we've clarified all my points above... ;-)

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.