Bug 772284 - Review Request: libnl3 - convenience library for kernel netlink API
Summary: Review Request: libnl3 - convenience library for kernel netlink API
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dan Horák
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 689753 (view as bug list)
Depends On:
Blocks: 781458
TreeView+ depends on / blocked
 
Reported: 2012-01-06 17:06 UTC by Dan Williams
Modified: 2013-09-24 14:45 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-01-19 17:33:54 UTC
Type: ---
Embargoed:
dan: fedora-review+


Attachments (Terms of Use)

Description Dan Williams 2012-01-06 17:06:45 UTC
Spec URL: http://bigw.org/~dan/libnl3.spec

SRPM URL: http://bigw.org/~dan/libnl3-3.2.3-1.fc16.src.rpm

Description: libnl3 is intended to supplant the existing libnl package over time, but to ensure smooth updates we need to keep the old libnl package around for a while too since many things depend on it, and the API/ABI changed significantly between libnl1 and libnl3.

Since libnl3 is parallel installable with libnl1, I elected to create a new libnl3 package much like we have glib/glib2 etc.

NetworkManager 0.9.4 will depend on libnl3 for features such as bonding, bridging, and VLANs.

Comment 2 Thomas Graf 2012-01-11 13:11:58 UTC
What about /etc/libnl/?

We should be providing defaults for /etc/libnl/classid and /etc/libnl/pktloc

Comment 3 Jiri Pirko 2012-01-12 18:22:59 UTC
I updated spec and libnl version as well. The main difference is to put libnl3 files into /usr/ and to include -cli as separate package:

Spec diff URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec.diff
Spec URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec
SRPM URL: http://people.redhat.com/jpirko/libnl3/libnl3-3.2.5-1.fc16.src.rpm

Successfully built in koji (http://koji.fedoraproject.org/koji/taskinfo?taskID=3642248)

I successfully built and run my libteam library against this.

Comment 4 Dan Horák 2012-01-12 18:35:24 UTC
Dan, please let me know if you will accept Jiri's updates to the spec or what version should I review. In any case the changelog should be truncated for libnl3.

Comment 5 Dan Williams 2012-01-12 21:18:38 UTC
(In reply to comment #3)
> I updated spec and libnl version as well. The main difference is to put libnl3
> files into /usr/ and to include -cli as separate package:
> 
> Spec diff URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec.diff
> Spec URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec
> SRPM URL: http://people.redhat.com/jpirko/libnl3/libnl3-3.2.5-1.fc16.src.rpm
> 
> Successfully built in koji
> (http://koji.fedoraproject.org/koji/taskinfo?taskID=3642248)
> 
> I successfully built and run my libteam library against this.

We've put the libs into /lib to make sure we can still boot the system when /usr is network mounted; NetworkManager for example requires libnl and if it were in /usr we wouldn't be able to boot a network-mounted-/usr system.

I wasn't quite sure what to do about the cli tools so thanks for handling that.  Can we keep the libraries in /lib or /lib64 for now to match the packaging of libnl 1.x?

Comment 6 Dan Williams 2012-01-12 21:19:00 UTC
*** Bug 689753 has been marked as a duplicate of this bug. ***

Comment 7 Jiri Pirko 2012-01-12 23:08:03 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > I updated spec and libnl version as well. The main difference is to put libnl3
> > files into /usr/ and to include -cli as separate package:
> > 
> > Spec diff URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec.diff
> > Spec URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec
> > SRPM URL: http://people.redhat.com/jpirko/libnl3/libnl3-3.2.5-1.fc16.src.rpm
> > 
> > Successfully built in koji
> > (http://koji.fedoraproject.org/koji/taskinfo?taskID=3642248)
> > 
> > I successfully built and run my libteam library against this.
> 
> We've put the libs into /lib to make sure we can still boot the system when
> /usr is network mounted; NetworkManager for example requires libnl and if it
> were in /usr we wouldn't be able to boot a network-mounted-/usr system.

Well I was told that eventually all libs will end up to be in /usr/lib. So that was the reason why I changed libnl3 files to be there. Dan Horak told me that as well. Anyway, if you have strong feeling about them being in /lib, please move them there.

> 
> I wasn't quite sure what to do about the cli tools so thanks for handling that.

Well in debian for example they fragment this even more. They have separate package for nf, route, cli, utils as well. But I think that the partitioning I proposed is good enough.

>  Can we keep the libraries in /lib or /lib64 for now to match the packaging of
> libnl 1.x?

Comment 8 Dan Williams 2012-01-13 01:10:49 UTC
I'll defer to Bill here... Bill, do we care about network-mounted-/usr much any more?  If we don't, then I'm fine with /usr/lib.

We originally moved glib2 and libnl and NetworkManager to /bin and /lib to support that case way back before F7 I think.

Comment 9 Jiri Pirko 2012-01-13 09:49:12 UTC
http://fedoraproject.org/wiki/Features/UsrMove

Comment 10 Dan Williams 2012-01-13 22:02:05 UTC
(In reply to comment #9)
> http://fedoraproject.org/wiki/Features/UsrMove

Ah, no problem with /usr in that case.  (In reply to comment #4)
> Dan, please let me know if you will accept Jiri's updates to the spec or what
> version should I review. In any case the changelog should be truncated for
> libnl3.

Jiri's updated version looks good to me.  When importing I'll truncate the changelog.

Comment 11 Dan Williams 2012-01-16 16:18:13 UTC
Dan, anything else we should fix up from Jiri's specfile?  Or are we good to go?  (in which case, fedora-review+ greatly appreciated :)

Comment 12 Dan Horák 2012-01-17 09:59:40 UTC
formal review is here, see the notes explaining OK* and BAD statuses below:

OK	source files match upstream:
	    f7994bf67452e2c11fe55ce7f4caa18a4f23e37d  libnl-3.2.5.tar.gz
OK	package meets naming and versioning guidelines.
OK	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	license field matches the actual license.
OK	license is open source-compatible (LGPLv2). License text included in package.
OK	latest version is being packaged.
OK	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	package builds in mock (Rawhide/i386).
OK	debuginfo package looks complete.
OK*	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	shared libraries are added to the regular linker search paths with correct scriptlets
BAD	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
OK	correct scriptlets present.
OK	code, not content.
BAD	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	headers in devel subpackage
OK	pkgconfig files in devel subpackage.
OK	no libtool .la droppings.
OK	not a GUI app.

- you could probably use --disable-static instead of --enable-static=no, but it's more cosmetic issue
- rpmlint complains a bit
libnl3.src: W: spelling-error Summary(en_US) netlink -> net link, net-link, nestling
libnl3.src: W: spelling-error %description -l en_US netlink -> net link, net-link, nestling
libnl3.i686: W: spelling-error Summary(en_US) netlink -> net link, net-link, nestling
libnl3.i686: W: spelling-error %description -l en_US netlink -> net link, net-link, nestling
libnl3-cli.i686: W: spelling-error Summary(en_US) utils -> utile, utilizes, utilize
libnl3-cli.i686: W: spelling-error %description -l en_US utils -> utile, utilizes, utilize
    => can be ignored

libnl3.src:117: W: macro-in-%changelog %{_lib}
libnl3.src:117: W: macro-in-%changelog %{_libdir}
libnl3.src:173: W: macro-in-%changelog %{_libdir}
    => will be resolved by dropping the old changelog

libnl3.i686: E: incorrect-fsf-address /usr/share/doc/libnl3-3.2.5/COPYING
libnl3-cli.i686: E: incorrect-fsf-address /usr/share/doc/libnl3-cli-3.2.5/COPYING
    => upstream should fix this

libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/cgroup.so cgroup.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/basic.so basic.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/htb.so htb.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/bfifo.so bfifo.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/pfifo.so pfifo.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/blackhole.so blackhole.so.0.0.0
    => doesn't seem as a bug, but are those files libraries or dlopen()-ed plugins? Shouldn't they be linked with -avoid-version libtool flag?

libnl3.i686: W: shared-lib-calls-exit /usr/lib/libnl-route-3.so.199.5.1 exit
libnl3-cli.i686: W: shared-lib-calls-exit /usr/lib/libnl-cli-3.so.199.5.1 exit
    => needs a comment

libnl3-cli.i686: W: summary-not-capitalized C libnl3 command line interface utils
    => could be reworded to "Command line interface utils for libnl3"

libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-qdisc-add.8.gz 2: warning: macro `LO' not defined
libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-classid-lookup.8.gz 2: warning: macro `LO' not defined
libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-pktloc-lookup.8.gz 2: warning: macro `LO' not defined
    => upstream issue

libnl3-cli.i686: W: no-manual-page-for-binary nl-class-list
libnl3-cli.i686: W: no-manual-page-for-binary nl-class-delete
libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-add
libnl3-cli.i686: W: no-manual-page-for-binary nl-class-add
libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-list
libnl3-cli.i686: W: no-manual-page-for-binary nl-link-list
libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-delete
    => would be nice to have

- the directories under %{_libdir} (libnl, libnl/cli/...) are not owned by the package although they should
- the documentation makes 99% of the devel subpackage and it should be packaged separately

Comment 13 Thomas Graf 2012-01-17 10:23:08 UTC
(In reply to comment #12)

 
> libnl3.i686: E: incorrect-fsf-address /usr/share/doc/libnl3-3.2.5/COPYING
> libnl3-cli.i686: E: incorrect-fsf-address
> /usr/share/doc/libnl3-cli-3.2.5/COPYING
>     => upstream should fix this

Will do

> 
> libnl3-devel.i686: W: dangling-relative-symlink
> /usr/lib/libnl/cli/cls/cgroup.so cgroup.so.0.0.0
> libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/basic.so
> basic.so.0.0.0
> libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/htb.so
> htb.so.0.0.0
> libnl3-devel.i686: W: dangling-relative-symlink
> /usr/lib/libnl/cli/qdisc/bfifo.so bfifo.so.0.0.0
> libnl3-devel.i686: W: dangling-relative-symlink
> /usr/lib/libnl/cli/qdisc/pfifo.so pfifo.so.0.0.0
> libnl3-devel.i686: W: dangling-relative-symlink
> /usr/lib/libnl/cli/qdisc/blackhole.so blackhole.so.0.0.0
>     => doesn't seem as a bug, but are those files libraries or dlopen()-ed
> plugins? Shouldn't they be linked with -avoid-version libtool flag?

Those are dlopen() plugins. They are currently compiled like this:

cli_qdisc_htb_la_LDFLAGS = -module -version-info 0:0:0

I will change it to -avoid-version. Thanks!

> libnl3.i686: W: shared-lib-calls-exit /usr/lib/libnl-route-3.so.199.5.1
> exit

This I do not understand. git grep exit also does not show anything unusual.

> libnl3-cli.i686: W: shared-lib-calls-exit /usr/lib/libnl-cli-3.so.199.5.1
> exit
>     => needs a comment

The CLI libraries do issue exit(), f.e. when parsing the argument string requests printing the help text.

> libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-qdisc-add.8.gz
> 2: warning: macro `LO' not defined
> libnl3-cli.i686: W: manual-page-warning
> /usr/share/man/man8/nl-classid-lookup.8.gz 2: warning: macro `LO' not defined
> libnl3-cli.i686: W: manual-page-warning
> /usr/share/man/man8/nl-pktloc-lookup.8.gz 2: warning: macro `LO' not defined
>     => upstream issue

Will fix those.

> libnl3-cli.i686: W: no-manual-page-for-binary nl-class-list
> libnl3-cli.i686: W: no-manual-page-for-binary nl-class-delete
> libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-add
> libnl3-cli.i686: W: no-manual-page-for-binary nl-class-add
> libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-list
> libnl3-cli.i686: W: no-manual-page-for-binary nl-link-list
> libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-delete
>     => would be nice to have

Volunteers needed :-)

Comment 14 Dan Horák 2012-01-17 11:15:22 UTC
(In reply to comment #13)
> (In reply to comment #12)
> 
> > libnl3-devel.i686: W: dangling-relative-symlink
> > /usr/lib/libnl/cli/cls/cgroup.so cgroup.so.0.0.0
> > libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/basic.so
> > basic.so.0.0.0
> > libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/htb.so
> > htb.so.0.0.0
> > libnl3-devel.i686: W: dangling-relative-symlink
> > /usr/lib/libnl/cli/qdisc/bfifo.so bfifo.so.0.0.0
> > libnl3-devel.i686: W: dangling-relative-symlink
> > /usr/lib/libnl/cli/qdisc/pfifo.so pfifo.so.0.0.0
> > libnl3-devel.i686: W: dangling-relative-symlink
> > /usr/lib/libnl/cli/qdisc/blackhole.so blackhole.so.0.0.0
> >     => doesn't seem as a bug, but are those files libraries or dlopen()-ed
> > plugins? Shouldn't they be linked with -avoid-version libtool flag?
> 
> Those are dlopen() plugins. They are currently compiled like this:
> 
> cli_qdisc_htb_la_LDFLAGS = -module -version-info 0:0:0
> 
> I will change it to -avoid-version. Thanks!

will also need some changes in the packaging, like filtering the Provides
 
> > libnl3.i686: W: shared-lib-calls-exit /usr/lib/libnl-route-3.so.199.5.1
> > exit
> 
> This I do not understand. git grep exit also does not show anything unusual.

will try to find out what's rpmlint doing here
 
> > libnl3-cli.i686: W: shared-lib-calls-exit /usr/lib/libnl-cli-3.so.199.5.1
> > exit
> >     => needs a comment
> 
> The CLI libraries do issue exit(), f.e. when parsing the argument string
> requests printing the help text.

OK

> > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-list
> > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-delete
> > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-add
> > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-add
> > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-list
> > libnl3-cli.i686: W: no-manual-page-for-binary nl-link-list
> > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-delete
> >     => would be nice to have
> 
> Volunteers needed :-)

it's not a blocker, so it can wait for the right volunteer :-)

Comment 15 Jiri Pirko 2012-01-17 13:11:51 UTC
Tomas issued 3.2.6 which fixes many of warnings. Working on re-spec that.

Comment 17 Dan Horák 2012-01-17 14:55:57 UTC
All issues look to be resolved or explained, package is APPROVED.

Only change the Group for the cli sub-package to Applications/System before uploading to dist-git.

Comment 18 Jiri Pirko 2012-01-17 15:49:41 UTC
(In reply to comment #17)
> Only change the Group for the cli sub-package to Applications/System before
> uploading to dist-git.

lib is there as well (I link that in libteam). As I discussed with Dan Horak on irc, this change is not desirable then. So please leave that as it is. Thanks.

Comment 19 Dan Williams 2012-01-19 16:22:21 UTC
New Package SCM Request
=======================
Package Name: libnl3
Short Description: Convenience library for kernel netlink sockets
Owners: dcbw jirka
Branches: f15 f16
InitialCC:

Comment 20 Gwyn Ciesla 2012-01-19 16:29:05 UTC
Git done (by process-git-requests).

Comment 21 Dan Williams 2012-01-19 17:33:54 UTC
Imported to git and built.  Thanks everyone!

Comment 22 Paul Wouters 2013-09-23 19:36:34 UTC
Package Change Request
======================
Package Name: libnl3
New Branches: el6
Owners: pwouters
InitialCC:

Comment 23 Gwyn Ciesla 2013-09-23 20:06:35 UTC
Any comments from primary maintainers?

Comment 24 Paul Wouters 2013-09-24 14:45:16 UTC
sorry. I didn't spot it was already in rhel itself.....


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