Bug 830992

Summary: Review Request: libseccomp - Enhanced seccomp library
Product: [Fedora] Fedora Reporter: Paul Moore <pmoore>
Component: Package ReviewAssignee: Cole Robinson <crobinso>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: crobinso, notting, package-review, rc040203, rjones, tmraz
Target Milestone: ---Flags: crobinso: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-06-26 16:52:59 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 731148, 817987    

Description Paul Moore 2012-06-11 16:59:46 EDT
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 17:01:04 EDT
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 17:03:58 EDT
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 17:58:00 EDT
Koji scratch build for Rawhide/F18:

http://koji.fedoraproject.org/koji/taskinfo?taskID=4152796
Comment 4 Richard W.M. Jones 2012-06-12 04:06:57 EDT
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 05:20:43 EDT
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 10:27:36 EDT
(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 10:29:02 EDT
(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 10:30:55 EDT
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 10:44:23 EDT
(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 10:51:29 EDT
(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 11:21:05 EDT
(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 11:31:36 EDT
(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 13:08:03 EDT
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 13:28:54 EDT
Thanks for the example, I'll reconsider autotools for a future release.
Comment 15 Paul Moore 2012-06-13 14:29:07 EDT
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 17:38:39 EDT
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 17:24:19 EDT
I'll take the case!
Comment 19 Cole Robinson 2012-06-25 17:37:13 EDT
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 17:45:36 EDT
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 18:20:11 EDT
Paul, you're sponsored now.
Comment 22 Paul Moore 2012-06-26 10:48:05 EDT
Great, thank you to everyone for your comments/help/sponsorship!
Comment 23 Paul Moore 2012-06-26 15:26:35 EDT
New Package SCM Request
=======================
Package Name: libseccomp
Short Description: Enhanced seccomp library
Owners: pcmoore
Branches: f18
InitialCC:
Comment 24 Jon Ciesla 2012-06-26 15:33:13 EDT
Git done (by process-git-requests).

Don't request f18 yet, ==devel.
Comment 25 Paul Moore 2012-06-26 15:43:57 EDT
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 16:52:59 EDT
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