[Bug 218022] Review Request: clamsmtp - SMTP filter daemon for anti-virus checking using ClamAV

Fri Dec 1 16:12:00 UTC 2006

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

Summary: Review Request: clamsmtp - SMTP filter daemon for anti-virus checking using ClamAV

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

------- Additional Comments From wolfy at nobugconsulting.ro  2006-12-01 11:11 EST -------
Not an official review since I am just contributor. First sight observations:
- the package is named clamsmtp but almost everywhere in config and script files
it behaves as being named clamd.smtp. However it looks that this comes from
upstream.

- BuildRoot has a sane value, but not the one recommended at
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1

- rpmlint on the src.rpm gives two warnings
1)W: clamsmtp strange-permission clamsmtpd.init 0744
A file that you listed to include in your package has strange
permissions. Usually, a file should have 0644 permissions.
Since the file is a startup script, this should be ignored
2)W: clamsmtp setup-not-quiet
You should use -q to have a quiet extraction of the source tarball, as this
generate useless lines of log ( for buildbot, for example
setup -q should be used if possible 

- rpmlint on the binary gives a lot of errors about usage of non-standard
UID/GID. Since the daemon runs under its own freshly created user, all these can
be ignored.
W: clamsmtp conffile-without-noreplace-flag /etc/clamd.d/smtp.conf
This might be edited by the user, so I think that marking it as noreplace could
be useful.

E: clamsmtp non-standard-uid /var/lib/clamsmtp/tmp clamsmtp
E: clamsmtp non-standard-gid /var/lib/clamsmtp/tmp clamsmtp
E: clamsmtp non-standard-dir-perm /var/lib/clamsmtp/tmp 0750
This directory should not exist. At least not for pid, socket etc which should
not be placed here.

W: clamsmtp dangling-relative-symlink /usr/sbin/clamd.smtp clamd
Symlink to clamd, provided by the (required) clamav-server package

E: clamsmtp non-standard-uid /etc/clamsmtpd.conf clamsmtp
E: clamsmtp non-standard-gid /etc/clamsmtpd.conf clamsmtp
E: clamsmtp non-readable /etc/clamsmtpd.conf 0640
E: clamsmtp non-standard-uid /var/lib/clamsmtp clamsmtp
E: clamsmtp non-standard-gid /var/lib/clamsmtp clamsmtp
E: clamsmtp non-standard-dir-perm /var/lib/clamsmtp 0750
All these can be ignored due to daemon running as a new user which owns the above

E: clamsmtp incoherent-logrotate-file /etc/logrotate.d/clamd.smtp
logrotate file should be named clamsmtp

E: clamsmtp init-script-name-with-dot /etc/rc.d/init.d/clamd.smtp
recommended name is clamsmtp

E: clamsmtp no-status-entry /etc/rc.d/init.d/clamd.smtp
W: clamsmtp no-reload-entry /etc/rc.d/init.d/clamd.smtp
E: clamsmtp subsys-not-used /etc/rc.d/init.d/clamd.smtp
The init.d/clamd.smtp script just starts clamd.wrapper from the clamav package.
If possible, adding a couple of lines to verify the status would be nice, but I
guess that it can be ignored. How ever,I would say that a stop entry should exist

W: clamsmtp no-reload-entry /etc/rc.d/init.d/clamsmtpd
init.d/clamsmtpd claims it can do start, stop, restart, status, condrestart.
However only start, stop and restart are implemented

W: clamsmtp incoherent-subsys /etc/rc.d/init.d/clamsmtpd $prog
the package is named clamsmtp so the script should be called clamsmtp too, not
clamsmtpd

- the %install stage makes use of the non-recommended %makeinstall macro
(http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002)

- configuration files are created as here documents at install time. I have not
seen any comments about this in the wiki, but I think that using standard files
included as %source would be saner and less error prone

- a bunch of files are created in non-standard places
(/var/lib/clamsmtp/clamd.smtp.log instead of /var/log/clamsmtp;
/var/lib/clamsmtp/clamd.smtp.pid instead of /var/run/clamsmtp; I would also
recommend something in the line of /var/run/clamav/clamd.smtp rather then
/var/lib/clamsmtp/clamd.smtp.sock)

Now, the review list:
-OK:  named according to packaging guidelines
-OK:  program licensed as GPL, original tar.gz includes the license, license
filed in spec matches the actual license
-OK: spec is written in AE but includes configurations files and startup scripts
as here documents which make it hard to follow
-OK: source matchs upstream (04da6aab94934641fcf9e7a7598346fb
clamsmtp-1.8.tar.gz for both files)
-OK: builds succesfully in mock/i386
-OK: no build dependency
-OK: no locale
-OK: no shared libraries, so no need for calling ldconfig
-OK: not relocatable
-OK: the package owns the files/directories it creates but does not respect FHS
(see above my comments about this)
-OK: does not include duplicates
-OK: permissions are very sane
-OK: includes a %clean section
-OK: consistent usage of macros
-OK: all content is permissable
-OK: no large documents, so no need for separate -doc package
-OK: %doc really includes just docs, runtime functionality not affected by them
-OK: No header/static/.la/.pc files
-OK: Not a GUI so no need for .desktop
-OK: owns only its files
-OK: scriptlets are sane; the daemons are added bit not started by default,
conditionally restarted at upgrade, 
-SHOULD: works as advertised (at least does not segfault..)

Bottom line: 
- I think all those here documents should be placed in independent SOURCE files
- the startup scripts NEEDWORK (either implementing the missing advertised
functions or removing those functions from the help line)
- %makeinstall should be avoided
- maybe something can be done about the inconsistent name usage ? clamsmtp
(package name) is almost not used at all...

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.