Bug 700571 - Review Request: spindown - Daemon that can spindown idle disks
Summary: Review Request: spindown - Daemon that can spindown idle disks
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-04-28 17:14 UTC by Martin Cermak
Modified: 2011-06-09 00:06 UTC (History)
3 users (show)

Fixed In Version: spindown-0.4.0-3.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-25 02:49:56 UTC
hdegoede: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Martin Cermak 2011-04-28 17:14:34 UTC
Spec URL: http://www.physics.muni.cz/~cermak/spindown/spindown.spec
SRPM URL: http://www.physics.muni.cz/~cermak/spindown/spindown-0.4.0-1.fc14.src.rpm
Patch0 URL: http://www.physics.muni.cz/~cermak/spindown/spindown-0.4.0-initscript.patch
Patch1 URL: http://www.physics.muni.cz/~cermak/spindown/spindown-0.4.0-Makefile.patch

Description: 

Hi! I had a look at the Package maintainers wishlist [1] and decided to get the spindown daemon packaged for Fedora. I would appreciate a review.

+ rpmlint spindown.spec
spindown.spec: W: invalid-url Source0: http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not Found
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
+ rpmlint spindown-0.4.0-1.fc14.src.rpm
spindown.src: W: name-repeated-in-summary C spindown
spindown.src: W: spelling-error %description -l en_US sg -> SG, chg, cig
spindown.src: W: spelling-error %description -l en_US swappable -> trappable, mappable, unflappable
spindown.src: W: spelling-error %description -l en_US hda -> had, hd, ha
spindown.src: W: spelling-error %description -l en_US sdb -> sd, sb, db
spindown.src: W: invalid-url Source0: http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 6 warnings.
+ rpmlint spindown-0.4.0-1.fc14.x86_64.rpm
spindown.x86_64: W: name-repeated-in-summary C spindown
spindown.x86_64: W: spelling-error %description -l en_US sg -> SG, chg, cig
spindown.x86_64: W: spelling-error %description -l en_US swappable -> trappable, mappable, unflappable
spindown.x86_64: W: spelling-error %description -l en_US hda -> had, hd, ha
spindown.x86_64: W: spelling-error %description -l en_US sdb -> sd, sb, db
spindown.x86_64: W: no-documentation
spindown.x86_64: W: no-manual-page-for-binary spindownd
1 packages and 0 specfiles checked; 0 errors, 7 warnings.

-------------------------------
[1] http://fedoraproject.org/wiki/PackageMaintainers/WishList

Comment 1 Martin Cermak 2011-04-29 07:19:55 UTC
> spindown.spec: W: invalid-url Source0:
> http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: 
> Not Found

Just note this is a valid url, but google blocks certain clients probably.

Also the spelling warnings should be okay I suppose.

Comment 2 Hans de Goede 2011-04-29 12:21:41 UTC
I'll review this (and your other submission), and assuming all goes well sponsor you eventually, removing FE_NEEDSPONSOR blocker.

Comment 3 Hans de Goede 2011-04-29 12:46:50 UTC
Full review done:

Good:
- rpmlint checks return:
spindown.src: W: name-repeated-in-summary C spindown
spindown.src: W: spelling-error %description -l en_US sg -> chg, gs, sf
spindown.src: W: spelling-error %description -l en_US swappable -> trappable
spindown.src: W: spelling-error %description -l en_US hda -> had, ha, Ida
spindown.src: W: spelling-error %description -l en_US sdb -> db, sob, sub
spindown.src: W: invalid-url Source0: http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not Found
spindown.x86_64: W: name-repeated-in-summary C spindown
spindown.x86_64: W: spelling-error %description -l en_US sg -> chg, gs, sf
spindown.x86_64: W: spelling-error %description -l en_US swappable -> trappable
spindown.x86_64: W: spelling-error %description -l en_US hda -> had, ha, Ida
spindown.x86_64: W: spelling-error %description -l en_US sdb -> db, sob, sub
spindown.x86_64: W: no-documentation
spindown.x86_64: W: no-manual-page-for-binary spindownd
 These can all be ignored
- package meets naming guidelines
- package meets packaging guidelines
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- no need for .desktop file

Needs work:
- rpmlint checks return:
spindown-debuginfo.x86_64: E: debuginfo-without-sources
 That is not good, this is likely caused by RPM_OPT_FLAGS not being used, see
 below.
 
-license should be GPLv3+

-the requires for the post / preun scripts are wrong, and the output
 of the service command should be silenced, please use the template requires
 + scripts from here:
 http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets

-RPM_OPT_FLAGS are not used during compilation. You call make with:
 OPT="$RPM_OPT_FLAGS" but that does not seem to do the trick, the output during
 building shows:
 g++ -O1 -c src/main.cpp

-You're not using %{?_smp_mflags} when calling make from %build, if you've tried
 so and it does not work, please add a comment stating so

-src/ininiparser3.0b contains an embedded copy if the iniparser library,
 luckily the exact same version is already packaged for Fedora, so fixing this
 is easy, just rm -rf src/ininiparser3.0b in %prep, fixes the includes to use
 the headers from the iniparser-devel package and add -liniparser to the
 linking command in the Makefile

-the initscript:
 - tries to start shutdownd when no /etc/shutdown.conf is present
 - is missing the usual printing of [ OK] / [FAILED]
 - you may want to consider writing a Fedora specific initscript, using
   the template from:
   http://fedoraproject.org/wiki/Packaging/SysVInitScript
   That should be easy to adapt to shutdownd

- You're not packaing any docs, you MUST package the COPYING file as %doc,
  you should also package the CHANGELOG README TODO and spindown.conf.example
  files as %doc

Comment 4 Martin Cermak 2011-05-05 14:02:55 UTC
I belive all issues are fixed now in spindown-0.4.0-2:

Spec URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown.spec
SRPM URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-2.fc14.src.rpm
Patch0 URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-initscript.patch
Patch1 URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-Makefile.patch
Patch2 URL: http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-iniparser.patch

All changes are described in the changelog. Because the project is not under version control yet, I'm creating patches against pure sources. For this reason patches changed (for example [1] differs from [2]). Also I'm omitting the licence change announcement on fedora-devel-list in this case. Here are rpmlint results:

+ rpmlint spindown.spec
spindown.spec: W: invalid-url Source0: http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not Found
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

+ rpmlint spindown-0.4.0-2.fc14.src.rpm
spindown.src: W: spelling-error %description -l en_US sg -> SG, chg, cig
spindown.src: W: spelling-error %description -l en_US hdparm -> hardpan, harmed, Parma
spindown.src: W: spelling-error %description -l en_US swappable -> trappable, mappable, unflappable
spindown.src: W: spelling-error %description -l en_US hda -> had, hd, ha
spindown.src: W: spelling-error %description -l en_US sdb -> sd, sb, db
spindown.src: W: invalid-url Source0: http://spindown.googlecode.com/files/spindown-0.4.0.tar.gz HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 6 warnings.

+ rpmlint spindown-debuginfo-0.4.0-2.fc14.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

+ rpmlint spindown-0.4.0-2.fc14.x86_64.rpm
spindown.x86_64: W: spelling-error %description -l en_US sg -> SG, chg, cig
spindown.x86_64: W: spelling-error %description -l en_US hdparm -> hardpan, harmed, Parma
spindown.x86_64: W: spelling-error %description -l en_US swappable -> trappable, mappable, unflappable
spindown.x86_64: W: spelling-error %description -l en_US hda -> had, hd, ha
spindown.x86_64: W: spelling-error %description -l en_US sdb -> sd, sb, db
spindown.x86_64: W: no-manual-page-for-binary spindownd
1 packages and 0 specfiles checked; 0 errors, 6 warnings.

The package does not explicitly require sg3_utils because custom power management binaries (like sg_start or hdparm) can be used. 

I also performed sanity testing of the package on a machine with multiple disks.

-------------------
[1] http://www.physics.muni.cz/~cermak/spindown-0.4.0-2/spindown-0.4.0-Makefile.patch
[2] http://www.physics.muni.cz/~cermak/spindown/spindown-0.4.0-Makefile.patch

Comment 5 Hans de Goede 2011-05-06 09:22:13 UTC
Much better now, almost there.

Needs work:
- You're installing the example config file as /etc/spindown.conf now,
  no need to include at is as %doc then
- Thanks for doing the new initscript, but I'm afraid it needs some work:
  Must Fix:
  - The LSB HEADER is entirely empty, fill in the Short Description and
    Description and remove all the other lines.
  - statuspipe="/tmp/spindown.status"
    Is vulnerable to symlinks attacks, please make this /var/run/spindown.status
    instead
  - the $prog in "status -p /var/run/spindownd.pid $prog" must be $exec
  - start() is missing a "return $retval" at the end

  Should fix:
  - Given that you've effectively written a new script, please add it as
    Source1: (without an url) and then in $setup do:
    cp -p %{SOURCE1} .
    Rather then making it a hard to read patch. This will also make
    making future changes to it a lot easier.
  - Drop the unused RETVAL variable
  - Please use a pidfile variable (like lockfile and statuspipe) rather then
    writing out /var/run/spindownd.pid in full everywhere
  - This part:
    echo -n $"Starting $prog: "
    if [ $UID -ne 0 ] ; then   
        failure
        echo -e "\nUser has insufficient privilege."
        exit 4
    fi
    if [ ! -x $exec ]; then
        failure
        echo -e "\nDaemon binary not executable."
        exit 5
    fi
    if [ ! -f $config ]; then
        failure
        echo -e "\nConfig file missing."
        exit 6
    fi

    Deviates from how most Fedora init scripts do it, please change this to:

    [ $UID -eq 0 ] || exit 4
    [ -x $exec ] || exit 5
    [ -f $config ] || exit 6
    echo -n $"Starting $prog: "

  - You can likely (and if it works should) write this bit:

    $exec -d -f $statuspipe -c $config -p /var/run/spindownd.pid;
    retval=$?
    [ "$retval" -eq 0 ] && success || failure
    echo

    As:

    daemon $exec -d -f $statuspipe -c $config -p /var/run/spindownd.pid;
    retval=$?
    echo

  - stop() can be simplified to just:
    stop() {
        echo -n $"Stopping $prog: "
        killproc -p /var/run/spindownd.pid $exec
        retval=$?
        echo
        [ $retval -eq 0 ] && rm -f $lockfile
        return $retval
    }

  - judging from the old initscript reload should be:
    reload() {
        killproc -p /var/run/spindownd.pid $exec -HUP
    }

    And force_reload should then call reload rather then restart

Comment 6 Martin Cermak 2011-05-06 12:27:30 UTC
Created spindown-0.4.0-3.fc14 addressing the aforementioned issues, see [1]. 
I hope it'll be fine now. Please review.

-------------------
[1] http://www.physics.muni.cz/~cermak/spindown-0.4.0-3/

Comment 7 Hans de Goede 2011-05-06 13:17:50 UTC
(In reply to comment #6)
> Created spindown-0.4.0-3.fc14 addressing the aforementioned issues, see [1]. 
> I hope it'll be fine now. Please review.
> 
> -------------------
> [1] http://www.physics.muni.cz/~cermak/spindown-0.4.0-3/

Hi,

Thanks, looks good now: approved.

Regards,

Hans

Comment 8 Martin Cermak 2011-05-07 10:43:36 UTC
New Package SCM Request
=======================
Package Name: spindown
Short Description: Daemon that can spin idle disks down
Owners: mcermak
Branches: f14 f15
InitialCC: jwrdegoede

Comment 9 Jason Tibbitts 2011-05-10 15:40:45 UTC
Git done (by process-git-requests).

Comment 10 Fedora Update System 2011-05-13 09:27:23 UTC
spindown-0.4.0-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/spindown-0.4.0-3.fc15

Comment 11 Fedora Update System 2011-05-13 09:31:29 UTC
spindown-0.4.0-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/spindown-0.4.0-3.fc14

Comment 12 Fedora Update System 2011-05-13 23:16:17 UTC
spindown-0.4.0-3.fc14 has been pushed to the Fedora 14 testing repository.

Comment 13 Fedora Update System 2011-05-25 02:49:51 UTC
spindown-0.4.0-3.fc14 has been pushed to the Fedora 14 stable repository.

Comment 14 Fedora Update System 2011-06-09 00:06:28 UTC
spindown-0.4.0-3.fc15 has been pushed to the Fedora 15 stable repository.


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