Bug 523553 - Review Request: mutrace - Mutex Tracer
Review Request: mutrace - Mutex Tracer
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-15 18:39 EDT by Lennart Poettering
Modified: 2009-10-14 14:21 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-14 14:21:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
michel: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patches mutrace.c to include signal.c on non-x86 archs (314 bytes, patch)
2009-10-06 23:19 EDT, Michel Alexandre Salim
no flags Details | Diff

  None (edit)
Description Lennart Poettering 2009-09-15 18:39:42 EDT
Spec URL: http://0pointer.de/public/mutrace.spec
SRPM URL: http://0pointer.de/public/mutrace-0.1-1.fc12.src.rpm
Description: Mutex Tracer

mutracer is a mutex tracer/profiler. The package should be easy to review.

See this for more information:

http://0pointer.de/blog/projects/mutrace.html

If you run this through rpmlint you'll get invalid-soname and shared-lib-calls-exit warnings. Both are safe to be ignored.

The former is expected since this is not a library applications are supposed to link to, it's intended only for LD_PRELOAD. This is similar to libpulsedsp.so.

The latter too is expected since we actually hook into the different variations of exit() to print our statistics summary. After printing that we call the real implementations of those calls.
Comment 1 Martin Gieseking 2009-09-18 10:28:45 EDT
As far as I can see, there's not much to correct. In the %files section, you should change the %defattr and %attr settings like this:

%files
%defattr(-,root,root,-)
%doc README LGPL
%{_bindir}/mutrace
%attr(0644,root,root) %{_libdir}/libmutrace.so
Comment 2 Susi Lehtola 2009-09-18 10:37:56 EDT
Umh, shared libraries should have 755 attributes.
Comment 3 Martin Gieseking 2009-09-18 11:13:16 EDT
(In reply to comment #2)
> Umh, shared libraries should have 755 attributes.  

Yes, I also wanted to write that. But as the library is intended to be used with LD_PRELOAD only, it's not absolutely necessary. At least, LD_PRELOAD also works with libraries having 0644 permissions.
Comment 4 Mamoru TASAKA 2009-09-18 11:38:46 EDT
Well, LD_PRELOAD'ed library can have 0644 permission.

The point is that: on Fedora every ELF binaries must have executionable
permission when %install ends, otherwide find-debuginfo.sh won't work
correctly. So at the stage %install ends, libmutrace.so cannot be 0644
permission. 
We can change the permission by rpm's %attr explicitly after find-debuginfo.sh
strips binaries like this, however we usually just say "shared libraries should
have 755 attributes" because it is much simpler.

By the way I think it is better to move libmutrace.so to under %_libexecdir/%name
or so because this does not seem to be system-wide library.
Comment 5 Lennart Poettering 2009-09-18 11:47:22 EDT
(In reply to comment #4)

> By the way I think it is better to move libmutrace.so to under
> %_libexecdir/%name
> or so because this does not seem to be system-wide library.  

We cannot do this due to the madness that is multilib. We need to make sure that ld.so uses /usr/lib/libmutrace.so for 32bit and /usr/lib/libmutrace.so for 64bit processes. If we moved that .so into a subdir then we had to hardcode its path in $LD_PRELOAD which would break ld.so's automatic selection of the module to load. So, no can do. Sorry.
Comment 6 Lennart Poettering 2009-09-18 11:48:29 EDT
(In reply to comment #5)

> We cannot do this due to the madness that is multilib. We need to make sure
> that ld.so uses /usr/lib/libmutrace.so for 32bit and /usr/lib/libmutrace.so for

Oops, that second path should have been /usr/lib64/libmutrace.so of course. Sorry for the confusion.

> 64bit processes. If we moved that .so into a subdir then we had to hardcode its
> path in $LD_PRELOAD which would break ld.so's automatic selection of the module
> to load. So, no can do. Sorry.
Comment 7 Lennart Poettering 2009-09-18 11:58:06 EDT
.spec file is now fixed according to the comments:

Spec URL: http://0pointer.de/public/mutrace.spec
SRPM URL: http://0pointer.de/public/mutrace-0.1-2.fc12.src.rpm
Comment 8 Lennart Poettering 2009-09-23 19:30:07 EDT
Ping. I'd be thanful if someone could do the second review before the freeze! Thanks!
Comment 9 Michel Alexandre Salim 2009-09-24 18:25:03 EDT
Presumably you mean the first review, since nobody appears to be officially reviewing this?

Will be done in a few hours -- got to run right now.
Comment 10 Michel Alexandre Salim 2009-09-24 19:03:38 EDT
ReviewTemplate

MUST

?  rpmlint
  source clean. binary not produced due to errors.
OK package name
OK spec file name
OK package guideline-compliant
? license complies with guidelines
  complies with guidelines, yes. However, I recall a discussion in -devel about
  the licensing of Fedora Infrastructure projects.These are supposed to be
  LGPLv2+ for libraries and GPLv2+ for applications. Yours most likely is not
  bound by these guidelines, and should the infrastructure team decide to use
  mutrace, they can (the combination becomes GPLv3+), so this is up to you.
OK license field accurate
OK license file not deleted
OK spec in US English
OK spec legible
FIX source matches upstream
$ sha1sum mutrace-0.1.tar.gz ../SOURCES/mutrace-0.1.tar.gz 
dcb16f9a80262cb608f641ee5c960b15def6ad83  mutrace-0.1.tar.gz
6017fc40158663eeffec4c70ba69cf04cde11ef2  ../SOURCES/mutrace-0.1.tar.gz

Looks like the source in the SRPM is old:
-rw-rw-r--. 1 michel michel 339432 2009-09-22 00:09 mutrace-0.1.tar.gz
-rw-rw-r--. 1 michel michel 328641 2009-09-15 18:20 ../SOURCES/mutrace-0.1.tar.gz


OK builds under >= 1 archs, others excluded
  rpmbuild -bb attempted on x86_64 (see also "no dupes in %files")
FIX build dependencies complete
  missing binutils-devel
OK library -> ldconfig
OK own all directories
FIX no dupes in %files
  no dupes, but some unpackaged files: %{_libdir}/libmatrace.so and
  libmutrace-backtrace-symbols.so
OK permission
OK %clean RPM_BUILD_ROOT
OK macros used consistently
OK Package contains code
NA large docs => -doc
NA doc not runtime dependent
OK clean buildroot before install
OK filenames UTF-8

SHOULD
FIX if license text missing, ask upstream to include it
  corresponding GPL license text not included
FIX package build in mock on all architectures
  not using mock yet, BR problem
OK package functioned as described
FIX require package not files
  require util-linux-ng rather than /usr/bin/getopt ?
Comment 11 Michel Alexandre Salim 2009-10-01 12:41:44 EDT
Lennart? Would be nice to see this packaged soon-ish, especially since building for F-12 now require rel-eng to manually tag a package.
Comment 12 Lennart Poettering 2009-10-06 15:31:45 EDT
Yepp, the .spec file was actually belonging to an older tarball. Since I didn't really do a proper release of mutarce yet I just updated the tarball, but didn't update the spec file.
Comment 13 Lennart Poettering 2009-10-06 15:49:44 EDT
(In reply to comment #10)

> ? license complies with guidelines
>   complies with guidelines, yes. However, I recall a discussion in -devel about
>   the licensing of Fedora Infrastructure projects.These are supposed to be
>   LGPLv2+ for libraries and GPLv2+ for applications. Yours most likely is not
>   bound by these guidelines, and should the infrastructure team decide to use
>   mutrace, they can (the combination becomes GPLv3+), so this is up to you.

I don't see how mutrace should ever be used for Fedora infrastructure.

> OK license field accurate
> OK license file not deleted
> OK spec in US English
> OK spec legible
> FIX source matches upstream
> $ sha1sum mutrace-0.1.tar.gz ../SOURCES/mutrace-0.1.tar.gz 
> dcb16f9a80262cb608f641ee5c960b15def6ad83  mutrace-0.1.tar.gz
> 6017fc40158663eeffec4c70ba69cf04cde11ef2  ../SOURCES/mutrace-0.1.tar.gz

Fixed now.

> FIX build dependencies complete
>   missing binutils-devel

Fixed.

> FIX no dupes in %files
>   no dupes, but some unpackaged files: %{_libdir}/libmatrace.so and
>   libmutrace-backtrace-symbols.so

Fixed.

> SHOULD
> FIX if license text missing, ask upstream to include it
>   corresponding GPL license text not included

It is included.

> FIX package build in mock on all architectures
>   not using mock yet, BR problem

Uh?

> OK package functioned as described
> FIX require package not files
>   require util-linux-ng rather than /usr/bin/getopt ?  

/usr/bin/getopt is pretty generic and changed packages a couple of times in the past, which is is why I decided to stick with a path dependency instead of package dependency.

Spec file and srpm nowupdated:

Spec URL: http://0pointer.de/public/mutrace.spec
SRPM URL: http://0pointer.de/public/mutrace-0.1-3.fc12.src.rpm
Comment 14 Lennart Poettering 2009-10-06 15:54:46 EDT
Just a small side note: this newer snapshot of mutrace now includes a copy of libacktrace-symbols which is licensed GPLv2+. I have hence updated the spec file license tag accordingly.

Oh, and I now did the proper release of mutrace 0.1, so the confusion between tarballs and srpms shouldn't happen anymore.
Comment 15 Michel Alexandre Salim 2009-10-06 23:18:39 EDT
(In reply to comment #13)
> (In reply to comment #10)
> 
> > ? license complies with guidelines
> I don't see how mutrace should ever be used for Fedora infrastructure.
Fair enough.

> > FIX source matches upstream
> > $ sha1sum mutrace-0.1.tar.gz ../SOURCES/mutrace-0.1.tar.gz 
> > dcb16f9a80262cb608f641ee5c960b15def6ad83  mutrace-0.1.tar.gz
> > 6017fc40158663eeffec4c70ba69cf04cde11ef2  ../SOURCES/mutrace-0.1.tar.gz
> 
> Fixed now.
> 
Verified
$ sha1sum mutrace-0.1.tar.gz ../SOURCES/mutrace-0.1.tar.gz 
ea972f8666933fb297b9791187858ccca3fecff7  mutrace-0.1.tar.gz
ea972f8666933fb297b9791187858ccca3fecff7  ../SOURCES/mutrace-0.1.tar.gz

> > FIX build dependencies complete
> >   missing binutils-devel
> 
> Fixed.
OK
> 
> > FIX no dupes in %files
> >   no dupes, but some unpackaged files: %{_libdir}/libmatrace.so and
> >   libmutrace-backtrace-symbols.so
> 
> Fixed.
OK
> 
> > SHOULD
> > FIX if license text missing, ask upstream to include it
> >   corresponding GPL license text not included
> 
> It is included.
Em, I don't see it. I see these license files:
GPL  ==> GPL v2 (needed)
LGPL ==> LGPL v3 (needed)

per the LGPL file,

"""
  This version of the GNU Lesser General Public License incorporates
the terms and conditions of version 3 of the GNU General Public
License, supplemented by the additional permissions listed below.
"""

Per the FSF's instructions:
http://www.fsf.org/licensing/licenses/gpl-howto.html

"""
You should also include a copy of the license itself somewhere in the distribution of your program. All programs, whether they are released under the GPL or LGPL, should include the text version of the GPL. In GNU programs the license is usually in a file called COPYING.
"""

since you use LGPLv3+, you must bundle the GPLv3 license text as well.


> > FIX package build in mock on all architectures
> >   not using mock yet, BR problem
> 
> Uh?
I didn't build the package in mock, since the build dependencies were not complete (and there were unpackaged files), and I figured I'd have to test the fixed package anyway.

There are some failures on PPC, both with F-11 and F-12:

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

In mutrace.c, you define DEBUG_TRAP on non-x86 architectures as raise(SIGTRAP), but you are not #ifdef-including signal.h

> > OK package functioned as described
> > FIX require package not files
> >   require util-linux-ng rather than /usr/bin/getopt ?  
> 
> /usr/bin/getopt is pretty generic and changed packages a couple of times in the
> past, which is is why I decided to stick with a path dependency instead of
> package dependency.
Fair enough

I'm attaching the patch, which allows building on non-x86 architectures; incorporate that and add the missing GPLv3 text and this package should be ready to go.
Comment 16 Michel Alexandre Salim 2009-10-06 23:19:40 EDT
Created attachment 363922 [details]
Patches mutrace.c to include signal.c on non-x86 archs
Comment 17 Lennart Poettering 2009-10-07 14:01:56 EDT
Thanks. Merged your patch (thanks for prepping that!), and added the GPL3 license text.

Spec URL: http://0pointer.de/public/mutrace.spec
SRPM URL: http://0pointer.de/public/mutrace-0.2-1.fc12.src.rpm
Comment 18 Michel Alexandre Salim 2009-10-09 14:42:51 EDT
Koji build (F-12):
http://koji.fedoraproject.org/koji/taskinfo?taskID=1738488

$ rpmlint SRPMS/mutrace-0.2-1.fc12.src.rpm ~/Download/mutrace*0.2*
mutrace.x86_64: E: invalid-soname /usr/lib64/libmatrace.so libmatrace.so
mutrace.x86_64: W: shared-lib-calls-exit /usr/lib64/libmatrace.so exit
mutrace.x86_64: W: shared-lib-calls-exit /usr/lib64/libmatrace.so _exit
mutrace.x86_64: E: invalid-soname /usr/lib64/libmutrace.so libmutrace.so
mutrace.x86_64: W: shared-lib-calls-exit /usr/lib64/libmutrace.so exit
mutrace.x86_64: W: shared-lib-calls-exit /usr/lib64/libmutrace.so _exit
mutrace.x86_64: E: invalid-soname /usr/lib64/libmutrace-backtrace-symbols.so libmutrace-backtrace-symbols.so
mutrace.x86_64: W: shared-lib-calls-exit /usr/lib64/libmutrace-backtrace-symbols.so exit@GLIBC_2.2.5
mutrace.x86_64: W: shared-lib-calls-exit /usr/lib64/libmutrace-backtrace-symbols.so _exit@GLIBC_2.2.5
3 packages and 0 specfiles checked; 3 errors, 6 warnings.

The invalid-soname is OK, really, as the libraries are not supposed to be linked to by applications. The shared-lib-calls-exit warnings are just warnings, and presumably there's a good reason for using exit.

That took care of all the previous problems -- APPROVED.
Comment 19 Lennart Poettering 2009-10-12 18:16:14 EDT
New Package CVS Request
=======================
Package Name:  mutrace
Short Description: Mutex Tracer
Owners: lennart
Branches: F-12 devel
InitialCC:
Comment 20 Kevin Fenzi 2009-10-13 12:25:35 EDT
cvs done.
Comment 21 Lennart Poettering 2009-10-14 14:21:50 EDT
Commited, built and asked for a tag for f12.

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