Red Hat Bugzilla – Bug 486961
Review Request: libservicelog - Servicelog Database and Library
Last modified: 2014-01-12 19:08:21 EST
Spec URL: https://fedoraproject.org/w/uploads/a/ac/Libservicelog.spec
SRPM URL: https://fedoraproject.org/w/uploads/0/03/Libservicelog-1.0.1-1.fc11.src.rpm
The libservicelgo package contains a library to create and maintain a
database for storing events related to system service. This database
allows for the logging of serviceable and informational events, and for
the logging of service procedures that have been performed upon the system.
I'm a bit worried about the simple groupadd in post and simple groupdel in postun ... service is quite common name for group and I could imagine that group being used for "service" guys on some machines... by simple removal of libservicelog package, you will delete that group without warning - imho too dangerous... I remember case of Amanda (as user) and troubles which were caused by amanda package...
Also, what's the separate user used for - there's nothing obvious in this package that explains why it needs to be this way.
Additionally groupdel in postun would mean deleting that group even on update (as postun is run after post).
+ package builds in mock (development i386).
+ rpmlint is silent for SRPM
+ rpmlint is not silent for RPM.
- mostly could be ignored:
- .so is in lib<package>
- servicelog.db file strange permissions are recommended in usptream
installation procedure, so I guess required
- dangerous groupdel in postun mentioned earlier
+ source files match upstream.
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in package.
+ %doc files present.
+ BuildRequires are proper.
+ defattr usage is correct.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code.
+ no static libraries.
- .pc file present (in devel subpackage).
- I guess it is necessary for that package
- Requires: pkgconfig is missing for devel subpackage
+ no .la files.
+ no translations are available.
- Does NOT owns the directories it creates.
/var/lib/servicelog has to be owned
+ no duplicates in %files.
+ file permissions are appropriate.
+ Not a GUI app.
Summary of things to fix:
1) groupdel in post (consider complete removal)
2) var/lib/servicelog has to be owned
3) Requires: pkgconfig is missing for devel subpackage
spec file updated.
Asked upstream about group service. Waiting to answer...
I recommend to also add - otherwise pre scriptlet will fail for no shadow-utils or
for existing service group:
getent group news >/dev/null || /usr/sbin/groupadd service
Sorry, I meant:
getent group service >/dev/null || /usr/sbin/groupadd service
cut & paste is evil ;)
Ok, looks sane for me now (except those pending things [strange mode of servicelog.db and group service existence] now in clarification process with upstream - however those things are not review blockers).
New Package CVS Request
Package Name: libservicelog
Short Description: Servicelog Database and Library
Shoudln't this be using a defined system uid/gid, rather than blindly calling groupadd?
If defined system gid, then some of the existing or created via https://fedoraproject.org/wiki/PackageUserRegistry and https://fedoraproject.org/wiki/PackageUserCreation - as there are no free slots under 100 in setup uidgid reservation file. Need for service group is currently questioned on upstream mailing list, so it is possible that it will be removed completely. But IMHO not a review blocker anyway...
I don't see the "Requires(pre): shadow-utils". Did you add that?
Good point - I mentioned that off bugzilla too and AFAIK Roman changed this locally - so I guess he only forgot to update spec file on fedoraproject upload.
Service group will be able to write into database. Every needed program will be in this group.
Next question is database file permissions. I will push them as they are now (with executable bit)
libservicelog-1.0.1-1.fc11 just built...
Trust I can close this bug...