This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 226134 - Merge Review: mdadm
Merge Review: mdadm
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:39 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-28 20:34:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wolfy: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:39:37 EST
Fedora Merge Review: mdadm

http://cvs.fedora.redhat.com/viewcvs/devel/mdadm/
Initial Owner: dledford@redhat.com
Comment 1 manuel wolfshant 2007-04-11 07:38:08 EDT
Quick pre-review of release 2.6.1-2.fc7
* RPM name is OK
* Source mdadm-2.6.1.tgz is the same as upstream
* This is the latest version
* Builds fine in mock
* License GPL, is OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u}
-n)  (wiki: Packaging/Guidelines#BuildRoot)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: Packaging/Guidelines#parallelmake)
* Spec file: there is a "/sbin" in make install which should be replaced with
RPM macros (%_sbindir) (wiki: Packaging/Guidelines#macros)
* The BuildRoot must be cleaned at the beginning of %install
* make install (static ?) -- compiles stuff but does not use rpm_opt_flags; why
is it not compiled in %build ?
* Timestamps are not preserved for man pages, mdadm.conf-example and
init.d/mdmonitor (you should probably use install -p)
* Missing dependancies on chkconfig and service for %post / %preun / %postun
* mdadm service is enabled by default in /etc/rc.d/init.d/mdmonitor. Fedora
policy recommends services to be off by default and allow the admin to enable
them at will
* Most of the ANNOUNCE files could be dropped. The useful information is
available in the Changelog file
* In %Changelog, the line with 2.5.0-6 (which should have been 1.5.0-6) contains
an unescaped macro (%postun)
* Should require /usr/sbin/sendmail or maybe MTA, not smtpdaemon. See also
DSendmail="/usr/sbin/sendmail -t" in the Makefile
Comment 2 Doug Ledford 2007-04-11 11:30:28 EDT
BuildRoot fixed
Missing SMP flags fixed
/sbin != %{_sbindir}, and this package *really* needs to be in /sbin, not /usr/sbin
Cleaning of buildroot done
The make during build was due to building things out of order and the Makefile
was rebuilding mdadm because the object files were newer than the mdadm binary.
 Make mdadm the last thing to be built during %build solves the issue.
Timestamps for man pages and mdmonitor fixed, for all the others they are done
as part of the %doc macro, so if that gets them wrong then there's not much to
do (although viewing the output shows %doc is using -p, so I suspect their
timestamps are actually correct).
Dependencies done.
mdadm is a reasonable exception to the default off policy.  It exits immediately
if there is nothing for it to monitor, and if there is something for it to
monitor, it's responsible for notifying the admin of failure events in their
hardware that might compromise their raid subsystem integrity.
Announce files dropped.
Changelog fixed
As for sendmail, don't all the various MTA alternatives that provide smtpdaemon
also provide a sendmail compatibility link   via alternatives?
Comment 3 manuel wolfshant 2007-04-11 12:25:27 EDT
WRT smtpdaemon: please read this thread
https://www.redhat.com/archives/fedora-devel-list/2007-April/msg00377.html
The problem is not that all MTA alternatives that provide smtpdaemon also
provide a sendmail compatibility link but that mdadm really needs JUST that
compatibility link. We could say that by mere coincidence some packages which
provide /usr/sbin/sendmail also provide smtpdaemon. In my case, I have added
"Provides: smtpdaemon" to ssmtp ONLY to satisfy mdadm and mailx who do not
really need it [*]

/sbin: You are right, I apologize. I was thinking that %{_sbindir}= /sbin
Full review coming soon. First glance almost everything seems OK.


[*] Once this review is done I'll file bugs against all other versions of mdadm
(RHEL 3/4/5, FC5/6) to have Requires:smtpdaemon replaced.
Comment 4 manuel wolfshant 2007-04-12 12:12:58 EDT
rpmlint checks:

[wolfy@wolfy64 mdadm]$ rpmlint mdadm-2.6.1-3.fc7.src.rpm
W: mdadm summary-not-capitalized mdadm controls Linux md devices (software RAID
arrays)
-> ignorable
W: mdadm strange-permission mdmonitor.init 0755
-> including the file as 644 and setting it via install -p at 0755 would
probably make rpmlint happier. not a big deal anyway
W: mdadm unversioned-explicit-obsoletes mdctl
W: mdadm unversioned-explicit-obsoletes raidtools
-> As far as I have understood, these tools are gone for good, so this could be OK
  If very very sure that none of these will ever come back, it's OK, but most
probably a versioning measure should be put in place
W: mdadm mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 7)
-> cosmetic, please fix before uploading to CVS

[wolfy@wolfy64 mdadm]$ rpmlint mdadm-2.6.1-3.fc7.x86_64.rpm
W: mdadm summary-not-capitalized mdadm controls Linux md devices (software RAID
arrays)
->ignorable
E: mdadm obsolete-not-provided mdctl
E: mdadm obsolete-not-provided raidtools
-> see above
E: mdadm non-standard-dir-perm /var/run/mdadm 0700
-> security measure?
W: mdadm service-default-enabled /etc/rc.d/init.d/mdmonitor
-> ignorable per comment #2
W: mdadm incoherent-init-script-name mdmonitor
-> given my mdadm/mdmonitor change. ignorable

GOOD

- package meets naming guidelines
- package meets packaging guidelines [*]
- license (GPL ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream, sha1sum
c0f523853a9c816986c24c699fa80b2c02336f3d  mdadm/devel/mdadm-2.6.1.tgz
- no locales
- not relocatable
- owns all files/directories that it creates, does not take ownership of foreign
files/dirs
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for separate package for docs
- nothing in %doc affects runtime
- not a GUI, so no need for .desktop file 
- no .pc, .la files
- sciptlets are sane, including removal of previously existing but no longer
needed mdmpd
- final provides are sane:
[wolfy@wolfy64]$ rpm -q --provides mdadm
mdadm = 2.6.1-3.fc7
- final requires are mostly sane:
[wolfy@wolfy64]$ rpm -q --requires mdadm|sort -u
/bin/bash
/bin/sh
libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3.3)(64bit)
libc.so.6(GLIBC_2.3.4)(64bit)
libc.so.6(GLIBC_2.3)(64bit)
libc.so.6(GLIBC_2.4)(64bit)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)
/sbin/chkconfig
/sbin/service
smtpdaemon

[*] There is one static compiled binary but it is needed so this should be
considered an exception to normal policy of a separate -static package.


SHOULD
- builds in mock/devel/x86_64
- works as advertised

Recommended fixes:
a) timestamp is not preserved for /usr/share/doc/mdadm-2.6.1/mdadm.conf-example:
-rw-r--r--    1 root    root             2180 Apr 11 19:16 
one can see that this is the build time, not the file time in the tar (
-rw-r--r-- 1 wolfy wolfy  2194 Jun 20  2006)
The difference comes from mdadm-1.5.0-email.patch; I suggest "touch -r" using
one of the other doc files as reference

b) %defattr(-,root,root,-) instead of %defattr(-,root,root)


Only remaining issue is the "smtpdaemon" Requires which definitely should be
replaced either by the virtual provide MTA or (better yet) by "/usr/sbin/sendmail"
Comment 5 manuel wolfshant 2007-04-12 12:21:19 EDT
sorry for typo, "given my mdadm/mdmonitor change" should have been "given by ... "
Comment 6 Doug Ledford 2007-04-16 20:18:36 EDT
W: mdadm unversioned-explicit-obsoletes mdctl
W: mdadm unversioned-explicit-obsoletes raidtools
-> As far as I have understood, these tools are gone for good, so this could be OK
  If very very sure that none of these will ever come back, it's OK, but most
probably a versioning measure should be put in place

These are most definitely gone forever.  The ioctls that the raidtools use are
being phased out of the kernel entirely, and mdctl was basically the first
version of mdadm and the name was changed later.  However, just in the last
month I had to do a new update to raidtools for a bug fix for a commercial
package that depended on raidtools, so I'm leary of hard coding the values since
they *might* change on something like an older RHEL but we still want them to be
obsoleted on upgrade to later RHEL or Fedora.

Tab/spaces fixed.

Changed summary and description so they start capitalized.

In regards to /var/run/mdadm (I didn't make the change and don't fully know why
it was necessary), here's the changelog:

* Fri Jul 30 2004 Dan Walsh <dwalsh@redhat.com> 1.5.0-11
- Create a directory /var/run/mdadm to contain mdadm.pid
- This cleans up SELinux problem

I'm guessing that the permissions on the dir are to avoid possible cross context
contamination since mdadm has it's own context in SELinux policy.

As for the mdadm.conf-example timestamp, you're right that it's because of the
email patch.  But, that then raises the question of whether or not the timestamp
should be preserved since the file no longer matches the original tarball file.
 The patch was originally done because the default install of mdadm copied the
mdadm.conf-example to /etc and we wanted a working MAILADDR.  However, now a
days, Anaconda custom writes the mdadm.conf on install, so we don't even package
an /etc/mdadm.conf file in the rpm.  As a result, it's safe to drop the email
patch entirely, so we will no longer be modifying the file, and the timestamp
should come out correct now.

[*] There is one static compiled binary but it is needed so this should be
considered an exception to normal policy of a separate -static package.

Actually, there are two: mdadm.static and mdassemble.static (we don't even
include the shared version of mdassemble).  However, these are both intended for
use by mkinitrd to create initrd images and therefore *need* to be static, so I
do think they both warrant the exception status.

I changed the permission on the repo copy of the init script.  It was already
being installed with -m 0755 in the %install, the odd permissions were just a relic.

Changed Requires: smptdaemon to /usr/sbin/sendmail

New build currently in process.  2.6.1-4.fc7 is the tag.
Comment 7 manuel wolfshant 2007-04-17 03:57:57 EDT
All problems fixed, explanations are logical. Package is APPROVED.
Thank you, Doug, for changing the Provides.

Comment 8 Doug Ledford 2007-04-17 09:10:14 EDT
Thank you for reviewing.  I'll go ahead and build this same package for FC5/FC6.
Comment 9 manuel wolfshant 2007-05-28 20:16:29 EDT
Doug, could you please close this ticket when you have 30 secs?
Comment 10 Doug Ledford 2007-05-28 20:34:38 EDT
Sure, didn't know I was supposed to ;-)

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