Bug 486961 - Review Request: libservicelog - Servicelog Database and Library
Review Request: libservicelog - Servicelog Database and Library
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ondrej Vasik
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-23 08:47 EST by Roman Rakus
Modified: 2014-01-12 19:08 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-11 10:55:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ovasik: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Roman Rakus 2009-02-23 08:47:11 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
Description: 
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.
Comment 1 Ondrej Vasik 2009-02-25 10:11:19 EST
First check:
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...
Comment 2 Bill Nottingham 2009-02-25 17:28:47 EST
Also, what's the separate user used for - there's nothing obvious in this package that explains why it needs to be this way.
Comment 3 Ondrej Vasik 2009-03-03 10:24:06 EST
Additionally groupdel in postun would mean deleting that group even on update (as postun is run after post).

Other checks:
+ 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.
e3280d47a16a29beef586d5b6068a562  libservicelog-1.0.1.tar.gz
+ 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
Comment 4 Roman Rakus 2009-03-03 11:16:35 EST
https://fedoraproject.org/w/uploads/a/ac/Libservicelog.spec
spec file updated.
Asked upstream about group service. Waiting to answer...
Comment 5 Ondrej Vasik 2009-03-03 11:33:36 EST
I recommend to also add - otherwise pre scriptlet will fail for no shadow-utils or 
for existing service group:
Requires(pre): shadow-utils

%pre
getent group news >/dev/null || /usr/sbin/groupadd service
Comment 6 Ondrej Vasik 2009-03-03 11:34:46 EST
Sorry, I meant:
%post
getent group service >/dev/null || /usr/sbin/groupadd service 


cut & paste is evil ;)
Comment 7 Roman Rakus 2009-03-03 12:15:30 EST
updated
Comment 8 Ondrej Vasik 2009-03-04 06:52:43 EST
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).
APPROVED...
Comment 9 Roman Rakus 2009-03-04 06:59:16 EST
New Package CVS Request
=======================
Package Name: libservicelog
Short Description: Servicelog Database and Library
Owners: rrakus
Branches:
InitialCC:
Comment 10 Bill Nottingham 2009-03-04 10:33:26 EST
Shoudln't this be using a defined system uid/gid, rather than blindly calling groupadd?
Comment 11 Ondrej Vasik 2009-03-04 10:43:08 EST
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...
Comment 12 Kevin Fenzi 2009-03-05 15:32:26 EST
I don't see the "Requires(pre): shadow-utils". Did you add that?
Comment 13 Ondrej Vasik 2009-03-06 03:22:09 EST
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.
Comment 14 Kevin Fenzi 2009-03-07 12:16:32 EST
ok. 

cvs done.
Comment 15 Roman Rakus 2009-03-11 10:37:43 EDT
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)
Comment 16 Roman Rakus 2009-03-11 10:55:56 EDT
libservicelog-1.0.1-1.fc11 just built...
Trust I can close this bug...

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