Bug 353931
Summary: | Review Request: xguest - kiosk user setup program | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Daniel Walsh <dwalsh> |
Component: | Package Review | Assignee: | Tomas Mraz <tmraz> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, matt_domsch, notting |
Target Milestone: | --- | Flags: | tmraz:
fedora-review+
tcallawa: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-09-23 03:30:47 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Daniel Walsh
2007-10-26 12:13:52 UTC
SRPM URL: http://people.fedoraproject.org/~dwalsh/xguest/xguest-1.0.2-1.fc8.src.rpm First off I would like to get this review as fast as possible, to get inclusion in Fedora 8. Maybe I am too late already. The spec file does a couple of things that cause rpmlint to give errors. These are discussed at: https://bugzilla.redhat.com/show_bug.cgi?id=353321 This package could be used in conjunction with Fast User Switching to allow users to share the machines with others with little risk, as well as in kiosk environments. good to add license file as %doc whats use of calling "exit 0" at end of each section? Added and removed exit 0's SRPM URL: http://people.fedoraproject.org/~dwalsh/xguest/xguest-1.0.3-1.fc8.src.rpm Maybe I am just picky, but I am very much against creating files in %post. I feel that %source1 sepermit.conf, with the appropriate content, copied in %post to the right place would be cleaner and easier to manage/verify via rpm -qV There are no files being created in post. sepermit.conf is owned by pam and xguest is just appending "xguest" to the file. So what is the state of this? How do we move forward? rpmlint -v ../SRPMS/xguest-1.0.5-2.fc8.src.rpm xguest.src: I: checking rpmlint -v ../RPMS/noarch/xguest-1.0.5-2.fc9.noarch.rpm xguest.noarch: I: checking xguest.noarch: E: use-tmp-in-%post xguest.noarch: E: use-of-home-in-%post These two aren't actually a use of it but adding the namespace configuration to namespace.conf. As soon as pam_namespace will support config files in namespace.d this should be changed. Perhaps you should save a backup of the existing namespace.conf so it will be possible to restore it after the future change. Of course the backup should be owned by the package as %ghost. xguest.noarch: E: preun-without-chkconfig /etc/rc.d/init.d/xguest That's a real error and it should be fixed. chkconfig --del should be run for xguest. xguest.noarch: W: service-default-enabled /etc/rc.d/init.d/xguest xguest.noarch: E: no-status-entry /etc/rc.d/init.d/xguest xguest.noarch: W: no-reload-entry /etc/rc.d/init.d/xguest I think these are OK. Status and reload entries do not make much sense as xguest is not a daemon but script containing just some bind mounts. Whether it should be enabled by default or not is debatable but as the %post script creates the xguest user account I think that enabling the polyinstantiation to work for him without further admins actions (except reboot or start of the script for the first time) is fine. xguest.noarch: W: uncompressed-zip /etc/desktop-profiles/xguest.zip That's OK as it is only 177kB anyway. Comment about the wording of Summary and Description: It should be describing what the package does so IMO it should be more like: Summary: Creates xguest user as a locked down user %description Installing this package sets up the xguest user to be used as a temporary account to switch to or as a kiosk user account. The account is disabled unless SELinux is in enforcing mode. The user is only allowed to log in via gdm. The home and temporary directories of the user will be polyinstantiated and mounted on tmpfs. More notes: The URL: http://people.fedoraproject.org/~dwalsh/xguest/%{name}-%{version}.tar.bz2 points to nonexistent file. There is no %defattr(...) on the beginning of %files. SRPM URL: http://people.fedoraproject.org/~dwalsh/xguest/xguest-1.0.6-1.fc9.src.rpm Has been updated with all your suggested fixes. rpmlint -v ../SRPMS/xguest-1.0.6-1.fc8.src.rpm xguest.src: I: checking rpmlint -v ../RPMS/noarch/xguest-1.0.6-1.fc8.noarch.rpm xguest.noarch: I: checking xguest.noarch: E: use-tmp-in-%post xguest.noarch: E: use-of-home-in-%post xguest.noarch: W: service-default-enabled /etc/rc.d/init.d/xguest xguest.noarch: W: no-reload-entry /etc/rc.d/init.d/xguest xguest.noarch: W: uncompressed-zip /etc/desktop-profiles/xguest.zip All these warnings and errors are OK as I wrote in comment #7. The suggested fixes were applied. Package is ACCEPTed. One more suggestion - when the namespace.conf file is modified I'd suggest to add a comment line before and after the xguest configuration so it will be easy to remove it as soon as the configuration is moved to an extra file in the future namespace.d. And in the %preun you can already remove these lines between the comment lines. SRPM URL: http://people.fedoraproject.org/~dwalsh/xguest/xguest-1.0.6-2.fc9.src.rpm Has been updated with all your suggested fixes. Spec file now deletes the namespace.conf entry on removal. Dan, the CVS process is here: http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure Please append a New Package CVS Request to this ticket and flip the flag back to ?. New Package CVS Request ======================= Package Name: dwalsh Short Description: SELinux Kiosk User account setup program Owners: dwalsh Branches: F-8 Rawhide InitialCC: sgrubb Cvsextras Commits: Yes New Package CVS Request ======================= Package Name: dwalsh Short Description: SELinux Kiosk User account setup program Owners: dwalsh Branches: F-8 Rawhide InitialCC: sgrubb Cvsextras Commits: No Umm, Dan? Is the package name really dwalsh? I almost committed that to cvs. :) Also, you don't need to list Rawhide as a branch, you always get that one. cvs is done. Package Name: xguest package is in the repos now, closing. |