[Bug 409511] New: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.




https://bugzilla.redhat.com/show_bug.cgi?id=409511

           Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand
                    HCA Userspace Driver
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: nobody(a)fedoraproject.org
        ReportedBy: rolandd(a)cisco.com
         QAContact: extras-qa(a)fedoraproject.org
                CC: [e-mail],notting(a)redhat.com


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....
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]

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511





------- Additional Comments From rolandd(a)cisco.com  2007-12-09 14:15 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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511


tibbs(a)math.uh.edu changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|nobody(a)fedoraproject.org    |tibbs(a)math.uh.edu
             Status|NEW                         |ASSIGNED
               Flag|                            |fedora-review?




------- Additional Comments From tibbs(a)math.uh.edu  2008-01-27 01:38 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.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511





------- Additional Comments From rolandd(a)cisco.com  2008-01-27 17:47 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.


-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511





------- Additional Comments From tibbs(a)math.uh.edu  2008-01-27 18:56 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?

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511





------- Additional Comments From rolandd(a)cisco.com  2008-01-27 23:26 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).

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511





------- Additional Comments From rolandd(a)cisco.com  2008-01-28 16:36 EST -------
I uploaded the updated package:

* Sun Jan 27 2008 Roland Dreier <rdreier(a)cisco.com&gt; - 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.


-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511


tibbs(a)math.uh.edu changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+




------- Additional Comments From tibbs(a)math.uh.edu  2008-01-31 00:22 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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511


rolandd(a)cisco.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?




------- Additional Comments From rolandd(a)cisco.com  2008-01-31 11:46 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(a)cisco.com
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes


-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511


kevin(a)tummy.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-cvs?                 |fedora-cvs+




------- Additional Comments From kevin(a)tummy.com  2008-01-31 12:50 EST -------
cvs done.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug 409511] Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libmlx4 - Mellanox ConnectX InfiniBand HCA Userspace Driver


https://bugzilla.redhat.com/show_bug.cgi?id=409511


rolandd(a)cisco.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |NEXTRELEASE




------- Additional Comments From rolandd(a)cisco.com  2008-02-03 15:48 EST -------
Successfully built.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.