Bug 830992 - Review Request: libseccomp - Enhanced seccomp library
Summary: Review Request: libseccomp - Enhanced seccomp library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Cole Robinson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 731148 817987
TreeView+ depends on / blocked
 
Reported: 2012-06-11 20:59 UTC by Paul Moore
Modified: 2013-10-19 14:42 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-06-26 20:52:59 UTC
Type: ---
Embargoed:
crobinso: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Paul Moore 2012-06-11 20:59:46 UTC
Spec URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp.spec
SRPM URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-0.fc18.src.rpm

Description:
The libseccomp library provides an easy to use interface to the Linux Kernel's syscall filtering mechanism, seccomp.  The libseccomp API allows an application to specify which syscalls, and optionally which syscall arguments, the application is allowed to execute, all of which are enforced by the Linux Kernel.

Fedora Account System Username: pcmoore

Comment 1 Paul Moore 2012-06-11 21:01:04 UTC
This package is part of the QEMU syscall filtering feature which is currently proposed for Fedora 18:

https://fedoraproject.org/wiki/Features/Syscall_Filters

Comment 2 Paul Moore 2012-06-11 21:03:58 UTC
The rpmlint is shown below; the spelling errors are not actual spelling errors (seccomp, syscall) and the URL (http://libseccomp.sourceforge.net) does exist.

libseccomp.x86_64: W: spelling-error Summary(en_US) seccomp -> sec comp, sec-comp, compose
libseccomp.x86_64: W: spelling-error %description -l en_US syscall -> scallop
libseccomp.x86_64: W: spelling-error %description -l en_US seccomp -> sec comp, sec-comp, compose
libseccomp.x86_64: W: spelling-error %description -l en_US syscalls -> miscalls
libseccomp.x86_64: W: invalid-url URL: http://libseccomp.sourceforge.net HTTP Error 403: Forbidden
libseccomp-debuginfo.x86_64: W: invalid-url URL: http://libseccomp.sourceforge.net HTTP Error 403: Forbidden
libseccomp-devel.x86_64: W: spelling-error %description -l en_US syscall -> scallop
libseccomp-devel.x86_64: W: spelling-error %description -l en_US seccomp -> sec comp, sec-comp, compose
libseccomp-devel.x86_64: W: spelling-error %description -l en_US syscalls -> miscalls
3 packages and 0 specfiles checked; 0 errors, 9 warnings.

Comment 3 Paul Moore 2012-06-11 21:58:00 UTC
Koji scratch build for Rawhide/F18:

http://koji.fedoraproject.org/koji/taskinfo?taskID=4152796

Comment 4 Richard W.M. Jones 2012-06-12 08:06:57 UTC
This isn't a review, just some comments.

I find spec files much easier to read if the fields
are lined up, like this example:

http://oirase.annexia.org/reviews/whenjobs/whenjobs.spec

Modern spec files *don't* need:

* BuildRoot
* %clean
* rm -rf buildroot
* defattr

All of the above can be removed.

Is the license LGPLv2 or LGPLv2+?

Instead of ./configure + options, use %configure macro.

Instead of CFLAGS=..., use 'make %{_smp_mflags}'

Comment 5 Ralf Corsepius 2012-06-12 09:20:43 UTC
Building should be verbose:

Non-verbose build logs like this are not-helpful when building in batch-jobs (such as in mock):
...
INFO: building in directory src/ ...
 CC api.o
 CC db.o
 CC arch.o
 CC arch-i386.o
...

Comment 6 Paul Moore 2012-06-12 14:27:36 UTC
(In reply to comment #4)
> This isn't a review, just some comments.

Regardless, thanks for the feedback.

> I find spec files much easier to read if the fields
> are lined up, like this example:
> 
> http://oirase.annexia.org/reviews/whenjobs/whenjobs.spec

Noted for future requests, as this is a style issue I'll leave it as-is right now pending the favorite style of the reviewer.

> Modern spec files *don't* need:
> 
> * BuildRoot
> * %clean
> * rm -rf buildroot
> * defattr
> 
> All of the above can be removed.

Done.

> Is the license LGPLv2 or LGPLv2+?

LGPLv2. LGPLv2.1 to be specific, but from what I read on the Fedora wiki regarding packaging the LGPLv2 tag applies to both LGPLv2 and LGPLv2.1.

> Instead of ./configure + options, use %configure macro.

While the libseccomp configure script mimics a few of the autoconf options, it only supports a select few.  I think we are better off specifying those few options by hand then risking a change in the %configure macro causing the configure/build to fail for this package.

However, if I'm looking at it the wrong way please let me know.

> Instead of CFLAGS=..., use 'make %{_smp_mflags}'

Noted, but unfortunately the package doesn't build correctly with simultaneous make jobs, e.g. "-j<N>" with "<N>" greater than one.  We'll work on this upstream but until we have it sorted we can't use "%{_smp_mflags}"; I tried previously and the results were not good.

Comment 7 Paul Moore 2012-06-12 14:29:02 UTC
(In reply to comment #5)
> Building should be verbose:

Is this a requirement for acceptance?  I ask because to make this change with the current release would either require changes upstream or a patch carried with the RPM.

Comment 8 Paul Moore 2012-06-12 14:30:55 UTC
I've updated the specfile and SRPM to incorporate the feedback from Richard W.M. Jones and I've submitted a new scratch build; links to all are below:

Spec URL:
http://people.redhat.com/~pmoore/review/libseccomp/libseccomp.spec

SRPM URL:
http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-0.fc18.src.rpm

Scratch Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4155147

Comment 9 Richard W.M. Jones 2012-06-12 14:44:23 UTC
(In reply to comment #6)
> LGPLv2. LGPLv2.1 to be specific, but from what I read on the Fedora wiki
> regarding packaging the LGPLv2 tag applies to both LGPLv2 and LGPLv2.1.

What I mean is, is it LGPLv2(.1) "only", or "any later version"?

(In reply to comment #7)
> > Building should be verbose:
> 
> Is this a requirement for acceptance?  I ask because to make this change
> with the current release would either require changes upstream or a patch
> carried with the RPM.

Whether or not it's a requirement, Ralf is right that it
makes debugging a whole lot easier.  When all you have to go on
is a Koji logfile, you'll be grateful that you enabled as much
verbosity as possible.

Comment 10 Paul Moore 2012-06-12 14:51:29 UTC
(In reply to comment #9)
> What I mean is, is it LGPLv2(.1) "only", or "any later version"?

LGPLv2.1 "only".
 
> (In reply to comment #7)
> > > Building should be verbose:
> > 
> > Is this a requirement for acceptance?  I ask because to make this change
> > with the current release would either require changes upstream or a patch
> > carried with the RPM.
> 
> Whether or not it's a requirement, Ralf is right that it
> makes debugging a whole lot easier.  When all you have to go on
> is a Koji logfile, you'll be grateful that you enabled as much
> verbosity as possible.

I've made it note of it for future upstream development efforts, but I just wanted to know if it was a requirement which would block the acceptance of the package.

Comment 11 Ralf Corsepius 2012-06-12 15:21:05 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > > Building should be verbose:
> > 
> > Is this a requirement for acceptance?
Off-head, I don't know if it's formally mandated. However without it, it's effectively impossible to review a package without major efforts, because the logs don't tell whether a package receives all CFLAGS/CPPFLAGS etc. correctly.

> >  I ask because to make this change
> > with the current release would either require changes upstream or a patch
> > carried with the RPM.
Are you upstream? 

Openly said, I'd recommend upstream to switch to a more standardized build/make framework (be it autotools, cmake or what ever), because hand-written makefiles/makefile-fragments, like the ones being using by this package tend to become non-understood and unmaintainable in longer terms.

Comment 12 Paul Moore 2012-06-12 15:31:36 UTC
(In reply to comment #11)
> Off-head, I don't know if it's formally mandated. However without it, it's
> effectively impossible to review a package without major efforts, because
> the logs don't tell whether a package receives all CFLAGS/CPPFLAGS etc.
> correctly.

As mentioned in comment 10, we'll work on a verbose build for a future upstream release but it would be very helpful to get this package added to Rawhide now (as long as it meets all of the established requirements) so that we can make progress in other areas which depend on this package simultaneously.

> Are you upstream? 

I've contributed the bulk of the code and cut the initial release.
 
> Openly said, I'd recommend upstream to switch to a more standardized
> build/make framework (be it autotools, cmake or what ever), because
> hand-written makefiles/makefile-fragments, like the ones being using by this
> package tend to become non-understood and unmaintainable in longer terms.

I understand the concern but I am not currently a fan of the build frameworks that I've seen thus far and I'm unlikely to convert to them in the near term unless their use is a requirement for acceptance into Fedora.  Looking towards the future, I'll need to spend more time understanding the build frameworks before I can give a definite "yes" or "no" regarding their inclusion in libseccomp.

Comment 13 Richard W.M. Jones 2012-06-12 17:08:03 UTC
Although autotools sucks greatly, it's a known quantity amongst
developers and for that reason generally better than hand-hacked
Makefiles.

Here is a minimal autotools setup you can take inspiration from:
http://git.annexia.org/?p=virt-hostinfo.git;a=tree;h=858cd733f328e2ffc961a28bbd4c362061bd2e6f;hb=56a3b79fbdcb80ef68f0311ca1d6af8b0789bff1

Comment 14 Paul Moore 2012-06-12 17:28:54 UTC
Thanks for the example, I'll reconsider autotools for a future release.

Comment 15 Paul Moore 2012-06-13 18:29:07 UTC
I've updated the libseccomp package for version 0.1.0 to include a patch which enables a verbose build as requested.  Links to the patch, specfile, SRPM and scratch build as below:

Spec URL:
http://people.redhat.com/~pmoore/review/libseccomp/libseccomp.spec

Patch URL:
http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-build_verbose.patch

SRPM URL:
http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-0.fc18.src.rpm

Scratch Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4159695

Scratch Build Log (x86_64):
http://koji.fedoraproject.org/koji/getfile?taskID=4159696&name=build.log

Comment 16 Paul Moore 2012-06-15 21:38:39 UTC
Just as a point of reference, libseccomp packages are now included in Debian/Ubuntu and Gentoo.

Debian/Ubuntu:
http://packages.debian.org/sid/libseccomp0

Gentoo:
http://packages.gentoo.org/package/sys-libs/libseccomp?arches=prefix

Comment 18 Cole Robinson 2012-06-25 21:24:19 UTC
I'll take the case!

Comment 19 Cole Robinson 2012-06-25 21:37:13 UTC
Firstly, I agree with recommendations up thread to convert to autotools, the 'known quantity' aspect is hugely beneficial if other packagers ever need to step in a make build or specfile tweaks. But not a blocker here.

fedora-review reported issues:

Issues:
[!]: MUST Package requires pkgconfig, if .pc files are present. (EPEL5)
     Note: Only applicable for EL-5

Not relevant since this requires a sufficiently new kernel that RHEL5 will never have.

See: http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#EL5
[!]: MUST Rpmlint output is silent.

rpmlint libseccomp-0.1.0-0.fc18.src.rpm

libseccomp.src: W: spelling-error Summary(en_US) seccomp -> sec comp, sec-comp, compose
libseccomp.src: W: spelling-error %description -l en_US syscall -> scallop
libseccomp.src: W: spelling-error %description -l en_US seccomp -> sec comp, sec-comp, compose
libseccomp.src: W: spelling-error %description -l en_US syscalls -> miscalls

Not relevant (these errors were also repeated for each sub package, output snipped).

libseccomp.src: W: invalid-url URL: http://libseccomp.sourceforge.net HTTP Error 403: Forbidden
1 packages and 0 specfiles checked; 0 errors, 5 warnings.

Certainly works for me, so not sure what rpmlint is hitting here.

Manual inspection of the spec and upstream sources didn't spot any other problems. Looks good to me!

Comment 20 Cole Robinson 2012-06-25 21:45:36 UTC
When the package is imported, please list me as a co-maintainer, and the virtmaint as watchbugzilla + watchcommits.

Also, Tomas Mraz agreed offline to sponsor Paul.

Comment 21 Tomas Mraz 2012-06-25 22:20:11 UTC
Paul, you're sponsored now.

Comment 22 Paul Moore 2012-06-26 14:48:05 UTC
Great, thank you to everyone for your comments/help/sponsorship!

Comment 23 Paul Moore 2012-06-26 19:26:35 UTC
New Package SCM Request
=======================
Package Name: libseccomp
Short Description: Enhanced seccomp library
Owners: pcmoore
Branches: f18
InitialCC:

Comment 24 Gwyn Ciesla 2012-06-26 19:33:13 UTC
Git done (by process-git-requests).

Don't request f18 yet, ==devel.

Comment 25 Paul Moore 2012-06-26 19:43:57 UTC
Great, thank you.

Cole, unfortunately I made a mistake and didn't list you as a co-maintainer (comment #20).  If you know how to fix this let me know.  Same thing applies to virtmaint and watchbugzilla + watchcommits.

Comment 26 Paul Moore 2012-06-26 20:52:59 UTC
According to the Fedora Wiki instructions I'm now closing out this BZ as "NEXTRELEASE" based on a successful Rawhide build.

* Rawhide Build
http://koji.fedoraproject.org/koji/buildinfo?buildID=327750


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