Bug 466762
Summary: | Review Request: ipmitool - Utility for IPMI control | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jan Safranek <jsafrane> |
Component: | Package Review | Assignee: | Gwyn Ciesla <gwync> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, gwync, notting, ville.skytta |
Target Milestone: | --- | Flags: | gwync:
fedora-review+
kevin: 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-10-17 11:38:08 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
Jan Safranek
2008-10-13 14:11:13 UTC
Starting full review. . . rpmlint on srpm clean On rpms: ipmitool.i386: W: service-default-enabled /etc/rc.d/init.d/ipmievd The service is enabled by default after "chkconfig --add"; for security reasons, most services should not be. Use "-" as the default runlevel in the init script's "chkconfig:" line and/or remove the "Default-Start:" LSB keyword to fix this if appropriate for this service. Is this really necessary? ipmitool.i386: W: incoherent-init-script-name ipmievd The init script name should be the same as the package name in lower case, or one with 'd' appended if it invokes a process by that name. Fixable, or does this coorespond with what's in openipmi-tools? I see no upstream future in the patch, but you might want to comment on that in the spec, just the same. Running a mock build to test BuildRequires, but otherwise, other than the above, it looks great on full review. BRs are fine. > service-default-enabled /etc/rc.d/init.d/ipmievd I agree that it should not be. Disabling its autostart is part of the patch submitted in bug 466343: -# chkconfig: 345 99 00 +# chkconfig: - 99 00 > incoherent-init-script-name ipmievd > Fixable, or does this coorespond with what's in openipmi-tools? IMHO, there's nothing to fix here - the init script specifically starts ipmievd, not ipmitool... rpmlint is being a bit silly here. "Obsoletes: OpenIPMI-tools < 2.0.14-3" missing (and possibly "Provides: OpenIPMI-tools = 2.0.14-3" if you think it's necessary, see http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Renaming.2Freplacing_existing_packages) From bug 466343: "BuildRequires: freeipmi-devel" should be added, or freeipmi support explicitly disabled for reproducible builds. From bug 466343: configure with --disable-dependency-tracking for slight build speedup and cleaner build logs? Source0 URL does not match recommended sf.net one: http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net Wouldn't configuring with --enable-file-security be desirable? "Requires: OpenIPMI" missing? (In reply to comment #4) > > service-default-enabled /etc/rc.d/init.d/ipmievd > > I agree that it should not be. Disabling its autostart is part of the patch > submitted in bug 466343: > > -# chkconfig: 345 99 00 > +# chkconfig: - 99 00 > > > incoherent-init-script-name ipmievd > > Fixable, or does this coorespond with what's in openipmi-tools? > > IMHO, there's nothing to fix here - the init script specifically starts > ipmievd, not ipmitool... rpmlint is being a bit silly here. Agreed. > > "Obsoletes: OpenIPMI-tools < 2.0.14-3" missing (and possibly "Provides: > OpenIPMI-tools = 2.0.14-3" if you think it's necessary, see > http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Renaming.2Freplacing_existing_packages) > > From bug 466343: "BuildRequires: freeipmi-devel" should be added, or freeipmi > support explicitly disabled for reproducible builds. > > From bug 466343: configure with --disable-dependency-tracking for slight build > speedup and cleaner build logs? Works for me. > Source0 URL does not match recommended sf.net one: > http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net The DNS resolves the same way, but there's no harm fixing it. > Wouldn't configuring with --enable-file-security be desirable? Possbily, not sure of the ramifications. Jan? > "Requires: OpenIPMI" missing? Probably, yes, I think I misunderstood the relationship here. Not needed to build, but it would require at runtime, no? (In reply to comment #5) > (In reply to comment #4) > > > service-default-enabled /etc/rc.d/init.d/ipmievd > > > > I agree that it should not be. Disabling its autostart is part of the patch > > submitted in bug 466343: > > > > -# chkconfig: 345 99 00 > > +# chkconfig: - 99 00 > > > > > incoherent-init-script-name ipmievd > > > Fixable, or does this coorespond with what's in openipmi-tools? > > > > IMHO, there's nothing to fix here - the init script specifically starts > > ipmievd, not ipmitool... rpmlint is being a bit silly here. > > Agreed. Rpmlit checks also Default-Start, I'll remove the runlevels from there. > > "Obsoletes: OpenIPMI-tools < 2.0.14-3" missing (and possibly "Provides: > > OpenIPMI-tools = 2.0.14-3" if you think it's necessary, see > > http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Renaming.2Freplacing_existing_packages) Obsoletes: added, Provides: sounds silly to me. > > From bug 466343: "BuildRequires: freeipmi-devel" should be added, or freeipmi > > support explicitly disabled for reproducible builds. Here I am not sure if we want dependency on FreeIPMI... It was not there before and most users won't need it. I'll check it together with pknirsch and we'll decide. > > From bug 466343: configure with --disable-dependency-tracking for slight build > > speedup and cleaner build logs? > > Works for me. Done. > > Source0 URL does not match recommended sf.net one: > > http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net > > The DNS resolves the same way, but there's no harm fixing it. Fixed. > > Wouldn't configuring with --enable-file-security be desirable? > > Possbily, not sure of the ramifications. Jan? It just adds additional checks when opening files (like owner, inode, link count etc.). It does not harm nor introduce any performance impact. I'll add it there. > > "Requires: OpenIPMI" missing? > > Probably, yes, I think I misunderstood the relationship here. Not needed to > build, but it would require at runtime, no? AFAIK, ipmitools talks to OpenIPMI driver via /dev/ipmi0. It does not link with OpenIPMI libraries. One may use ipmitool when talking to remote host over IP, completely without need of OpenIMPI. I'll check the freeipmi-devel issue and post new .spec. Enabling FreeIPMI support would IPMItool possibility to talk to local devices, that are supported by the FreeIPMI driver. But FreeIPMI package itself already provides IPMItool-like utility and there is no need to use IPMItool, that's why I decided to disable it. In addition to above, I added 'Provides: OpenIPMI-tools = 2.0.14-3' to make rpmlint happy. You can find new .spec and .srpm at http://people.redhat.com/jsafrane/ipmitool/2/ [limb@fawkes fedora]$ diff ipmitool.spec ~/rpmbuild/SPECS/ipmitool.spec
11c11
< BuildRequires: openssl-devel readline-devel ncurses-devel
---
> BuildRequires: openssl-devel readline-devel ncurses-devel freeipmi-devel
Which is the one I should be using?
oops, I don't know what went wrong, but the srpm has old (=wrong) .spec file. I uploaded new .srpm. Awesome. APPROVED. New Package CVS Request ======================= Package Name: ipmitool Short Description: Utility for IPMI control Owners: jsafrane Branches: F-10 InitialCC: I hope I can still catch F-10. cvs done. Thanks for review! |