Bug 409511 - Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver
Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-03 17:04 EST by Roland Dreier
Modified: 2008-02-03 15:48 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-03 15:48:57 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Roland Dreier 2007-12-03 17:04:05 EST
Spec URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4.spec
SRPM URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4-1.0-0.1.rc1.fc8.src.rpm
Description: 
libmlx4 provides a device-specific userspace driver for Mellanox
ConnectX HCAs for use with the libibverbs library.

Note that libibverbs is already part of Fedora, as is the libmthca userspace driver package.  I packaged libmlx4 almost identically to the existing libmthca package.

[This package is of 1.0-rc1, but the 1.0 final release is expected within two weeks; I am entering this bug now to start the review process, but the package I actually upload will be of the final 1.0 release]
Comment 1 Roland Dreier 2007-12-09 14:15:06 EST
The real 1.0 release of libmlx4 has come out, and I updated my package:
Spec URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4.spec
SRPM URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4-1.0-1.fc8.src.rpm
Comment 2 Jason Tibbitts 2008-01-27 01:38:33 EST
I guess all of the other reviewers are stared away by esoteric things like
hardware we couldn't hope to afford, but I am fearless.  Or am I insane?  I keep
forgetting.

Builds OK for me on rawhide; rpmlint says:

  libmlx4.x86_64: W: conffile-without-noreplace-flag 
   /etc/libibverbs.d/mlx4.driver
If it's a config, you probably don't want an rpm update wiping out end-user
customization, so you should use %config(noreplace).  The difference is whether
rpm creates a .rpmnew file instead of moving the old version to .rpmsave.

  libmlx4-devel-static.x86_64: W: no-documentation
Not a problem.

Generally, the package containing the static library should be named "-static".
 However, if this would leave the -devel package empty, you can put the library
in the -devel package and have it provide -static.  See the "Packaging Static
Libraries" section of http://fedoraproject.org/wiki/Packaging/Guidelines.  I
know this package is a little odd (unversioned so, no headers to compile
against) but I think the static library bits in the guidelines still cover this
situation well enough.

Since you install a shared library, you need to call ldconfig:
  %post -p /sbin/ldconfig
  %postun -p /sbin/ldconfig

Checklist:
* source files match upstream:
   ced3d0d5ac965e5d9c1230ecb98a6fb644906b6cdf25c117fabbdce0e0be2974  
   libmlx4-1.0.tar.gz
X package does not follow the naming guidelines for static library packages.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly
* debuginfo package looks complete.
X rpmlint has a valid complaint.
* Ignoring the -static package issue, final provides and requires are sane:
  libmlx4-1.0-1.fc9.x86_64.rpm
   config(libmlx4) = 1.0-1.fc9
   libmlx4-rdmav2.so()(64bit)
   libmlx4 = 1.0-1.fc9
  =
   config(libmlx4) = 1.0-1.fc9
   libibverbs.so.1()(64bit)
   libibverbs.so.1(IBVERBS_1.0)(64bit)
   libibverbs.so.1(IBVERBS_1.1)(64bit)

  libmlx4-devel-static-1.0-1.fc9.x86_64.rpm
   libmlx4-devel-static = 1.0-1.fc9
  =
   libmlx4 = 1.0-1.fc9

* %check is not present; no test suite upstream.  I haven't a clue how to test 
   this, and I don't have the hardware anyway.
X a shared library is installed, but ldconfig is not run.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
X no scriptlets present (but there should be)
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
X static libraries should be in the -devel package.
* no libtool .la files.
Comment 3 Roland Dreier 2008-01-27 17:47:16 EST
Thanks for the review!  A few comments/questions:

 > libmlx4.x86_64: W: conffile-without-noreplace-flag 
 >    /etc/libibverbs.d/mlx4.driver
 > If it's a config, you probably don't want an rpm update wiping out end-user
 > customization, so you should use %config(noreplace).  The difference is whether
 > rpm creates a .rpmnew file instead of moving the old version to .rpmsave.

the mlx4.driver file is not exactly a config file in the sense that there is
no useful change that a user can make right now; all it contains is a single
line that tells the libibverbs library to look for an "mlx4" plugin.  However
if I don't mark it as a conffile then rpmlint complains about non-config files
in /etc.  On the other hand, if some future version does introduce new info
in the mlx4.driver file, I think the best thing for a user would be to update
the file, so I left out the "noreplace" marking.

I don't have a strong opinion about the right thing here; although I did think
about this and make a semi-informed choice, just let me know if you want me to
change how the mlx4.driver file is handled, and I'll update my package.

 > Generally, the package containing the static library should be named "-static".
 >  However, if this would leave the -devel package empty, you can put the library
 > in the -devel package and have it provide -static.

Make sense, I will make the change and add the provides tag.

 > Since you install a shared library, you need to call ldconfig:
 >   %post -p /sbin/ldconfig
 >   %postun -p /sbin/ldconfig

The same issue came up when my libmthca package was reviewed (bug 169744).
The package does contain a .so file, but it is not something that is linked
to applications; rather, libibverbs uses dlopen() to load this plugin.  So
I don't think running ldconfig is necessary or desirable for this package.

I will prepare a package with the -devel-static -> -devel change and post it.
Let me know if my explanations for the other points make sense.
Comment 4 Jason Tibbitts 2008-01-27 18:56:55 EST
Note that the non-conffile-in-etc complaint is not absolute; there are
configuration things in /etc that aren't user configurables, like the files in
/etc/ld.so.conf.d.  This seems to be in the same vein, and from your description
it would be appropriate for that file to not be marked as %config.  The real
problem comes when files are marked %config but not %config(noreplace) as that's
explicitly saying that we expect the user to modify the file but we'll also
happly nuke their changes on package upgrades and force them to hunt down the
.rpmsave file and pull their changes back in.  Hence plain %config should be
used only very rarely.

About .so files used as plugins: If nothing ever links to them, they probably
should not live in the standard library directory.  Is it possible for these
files to live elsewhere?
Comment 5 Roland Dreier 2008-01-27 23:26:20 EST
 > Note that the non-conffile-in-etc complaint is not absolute

OK, I will just remove the %config marking.

 > About .so files used as plugins: If nothing ever links to them, they probably
 > should not live in the standard library directory.  Is it possible for these
 > files to live elsewhere?

It's not that easy for the .so plugins to be out of the standard library path.
libibverbs uses dlopen() without a full path to open the plugin.  This means
that the standard library path is used to find the .so.  This has the advantage
that we don't have to reinvent all the library path configuration stuff; for
example the user can use LD_LIBRARY_PATH to point to a different version of a
plugin if desired, or optimized builds can be put in /lib/tls/i686/sse2, etc.

I guess the main point is that the upstream libibverbs will only search the
standard library path, and the Fedora package keeps this behavior.  So there's
not really any way to change this without redoing the Fedora libibverbs and
libmthca package, diverging from upstream, and causing pain for users who want
to install unmodified upstream plugins (there are several others not yet
packaged for Fedora, eg libcxgb3, libipathverbs, libehca).
Comment 6 Roland Dreier 2008-01-28 16:36:54 EST
I uploaded the updated package:

* Sun Jan 27 2008 Roland Dreier <rdreier@cisco.com> - 1.0-2 
- Spec file cleanups, based on Fedora review: don't mark 
  libmlx4.driver as a config file, since it is not user modifiable, 
  and change the name of the -devel-static package to plain -devel, 
  since it would be empty without the static library. 

Spec URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4.spec
SRPM URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4-1.0-2.fc8.src.rpm

The only outstanding issue is the plugin .so as discussed above, but I don't
think there is a better way to handle this.
Comment 7 Jason Tibbitts 2008-01-31 00:22:47 EST
This picked up an additional rpmlint complaint, as expected:
  libmlx4.x86_64: W: non-conffile-in-etc /etc/libibverbs.d/mlx4.driver
but that's OK.

About the .so file, it is somewhat suboptimal, but this wouldn't be the first
package in the distro to do this and you do have a reasonable point about being
able to have specifically optimized versions without duplicating all of that
logic from the loader.  So I guess there's not much to do.

Outside of that, everything looks good to me.

APPROVED
Comment 8 Roland Dreier 2008-01-31 11:46:49 EST
New Package CVS Request
=======================
Package Name: libmlx4
Short Description: device-specific userspace driver for Mellanox ConnectX HCAs
for use with the libibverbs library
Owners: rolandd@cisco.com
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes
Comment 9 Kevin Fenzi 2008-01-31 12:50:57 EST
cvs done.
Comment 10 Roland Dreier 2008-02-03 15:48:57 EST
Successfully built.

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