Bug 466762 - Review Request: ipmitool - Utility for IPMI control
Review Request: ipmitool - Utility for IPMI control
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Gwyn Ciesla
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-13 10:11 EDT by Jan Safranek
Modified: 2008-10-17 07:38 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-17 07:38:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jan Safranek 2008-10-13 10:11:13 EDT
Spec and SRPM URL: http://people.redhat.com/jsafrane/ipmitool/1/

Description: IPMItool is an utility for interfacing with local or remote devices that support Intelligent Platform Management Interface specification. IPMI is
an open standard for machine health, inventory, and remote power control.

IPMItool is shipped now in OpenIPMI-tools package. But the IPMItool itself does not have anything common with OpenIPMI and should be packaged separately. I'll drop it from OpenIPMI-tools when this package is approved.
Comment 1 Gwyn Ciesla 2008-10-13 10:57:42 EDT
Starting full review. . .
Comment 2 Gwyn Ciesla 2008-10-13 11:08:31 EDT
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.
Comment 3 Gwyn Ciesla 2008-10-13 11:30:44 EDT
BRs are fine.
Comment 4 Ville Skyttä 2008-10-13 14:47:36 EDT
> 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?
Comment 5 Gwyn Ciesla 2008-10-13 14:59:38 EDT
(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?
Comment 6 Jan Safranek 2008-10-14 07:58:07 EDT
(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.
Comment 7 Jan Safranek 2008-10-14 10:24:45 EDT
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/
Comment 8 Gwyn Ciesla 2008-10-14 10:55:33 EDT
[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?
Comment 9 Jan Safranek 2008-10-15 03:09:14 EDT
oops, I don't know what went wrong, but the srpm has old (=wrong) .spec file. I uploaded new .srpm.
Comment 10 Gwyn Ciesla 2008-10-15 09:35:22 EDT
Awesome.

APPROVED.
Comment 11 Jan Safranek 2008-10-16 06:25:48 EDT
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.
Comment 12 Kevin Fenzi 2008-10-16 13:05:35 EDT
cvs done.
Comment 13 Jan Safranek 2008-10-17 07:38:08 EDT
Thanks for review!

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