Bug 1396232

Summary:	Review Request: libkeepalive - Enable TCP keepalive in dynamic binaries
Product:	[Fedora] Fedora	Reporter:	Phil Sutter <psutter>
Component:	Package Review	Assignee:	Igor Gnatenko <ignatenko>
Status:	CLOSED NEXTRELEASE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	package-review, psutter
Target Milestone:	---	Flags:	ignatenko: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2016-12-19 22:05:06 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Phil Sutter 2016-11-17 18:27:55 UTC

Spec URL: https://psutter.fedorapeople.org/libkeepalive.spec
SRPM URL: https://psutter.fedorapeople.org/libkeepalive-0.3-1.fc25.src.rpm
Description: libkeepalive is a library that enables tcp keepalive features in glibc based binary dynamic executables, without any change in the original program.
Fedora Account System Username: psutter

Originally I got a request for inclusion into RHEL (see private bug 1356103 for details) but I think including this into Fedora first and then cloning from there is the preferred method.

The library is actually quite simple: It enables TCP keepalives in binaries by wrapping the socket() syscall and setting SO_KEEPALIVE sockopt before returning the new socket. It is activated by setting LD_PRELOAD to it's path before calling the binary to alter, so by default does not have any effect on other packages.

Comment 1 Igor Gnatenko 2016-11-18 18:06:10 UTC

* CFLAGS/LDFLAGS are not used
  export CFLAGS="%{optflags}"
  export LDFLAGS="%{__global_ldflags}"
  but it still requires some patching of Makefile `=` -> `?=`
* Missing BuildRequires: gcc
* You should try to version library (though I'm not sure how much it matters here, probably doesn't at all)
* echo is not needed in %build
* instead of make you should use %make_build (e.g. %make_build -C src)
* there are some tests, so try to build / run them
* instead of doing mkdir, you can pass `-D` to install

Apart from that, looks good ;)

Comment 2 Phil Sutter 2016-11-23 19:25:32 UTC

Hi,

(In reply to Igor Gnatenko from comment #1)
> * CFLAGS/LDFLAGS are not used
>   export CFLAGS="%{optflags}"
>   export LDFLAGS="%{__global_ldflags}"
>   but it still requires some patching of Makefile `=` -> `?=`
> * Missing BuildRequires: gcc
> * You should try to version library (though I'm not sure how much it matters
> here, probably doesn't at all)
> * echo is not needed in %build
> * instead of make you should use %make_build (e.g. %make_build -C src)
> * there are some tests, so try to build / run them
> * instead of doing mkdir, you can pass `-D` to install
> 
> Apart from that, looks good ;)

Thanks for the elaborate review! I'll try to get things sorted ASAP then upload fresh SRPM and spec file.

Cheers, Phil

Comment 3 Phil Sutter 2016-11-25 12:42:15 UTC

Hi again,

(In reply to Phil Sutter from comment #2)
> (In reply to Igor Gnatenko from comment #1)
> > * CFLAGS/LDFLAGS are not used
> >   export CFLAGS="%{optflags}"
> >   export LDFLAGS="%{__global_ldflags}"
> >   but it still requires some patching of Makefile `=` -> `?=`
> > * Missing BuildRequires: gcc
> > * You should try to version library (though I'm not sure how much it matters
> > here, probably doesn't at all)

I didn't do this. To my knowledge, versioning shared objects makes sense only if programs are expected to link against them so it's possible to distinguish between minor and major upgrades. In this case, the library is meant to be injected into the program using LD_PRELOAD, so no versioning is required.

> > * echo is not needed in %build
> > * instead of make you should use %make_build (e.g. %make_build -C src)
> > * there are some tests, so try to build / run them
> > * instead of doing mkdir, you can pass `-D` to install
> > 
> > Apart from that, looks good ;)
> 
> Thanks for the elaborate review! I'll try to get things sorted ASAP then
> upload fresh SRPM and spec file.

All fixed apart from the missing versioning, updated spec file and SRPM available here:

Spec URL: https://psutter.fedorapeople.org/libkeepalive.spec
SRPM URL: https://psutter.fedorapeople.org/libkeepalive-0.3-2.fc25.src.rpm

Thanks, Phil

Comment 4 Igor Gnatenko 2016-11-25 13:01:03 UTC

* BuildRequires:	git -> BuildRequires:	git-core
git itself requires a lot of perl which you don't really need to apply patches.
* %autosetup -S git
probably you just want %autosetup -p1 to not pull git into BuildRequires
* %doc LICENSE README
LICENSE should be %license, READNE should be %doc
* Also put comment where you sent patches to upstream (or why you didn't sent)...

Apart from that, looks good.

Comment 5 Phil Sutter 2016-12-05 15:27:18 UTC

Hi Igor,

(In reply to Igor Gnatenko from comment #4)
> * BuildRequires:	git -> BuildRequires:	git-core
> git itself requires a lot of perl which you don't really need to apply
> patches.
> * %autosetup -S git
> probably you just want %autosetup -p1 to not pull git into BuildRequires

Ah yes, that is much easier. When playing around with autosetup options, I got errors during patch applying, but probably that was just due to missing '-p1' option.

> * %doc LICENSE README
> LICENSE should be %license, READNE should be %doc
> * Also put comment where you sent patches to upstream (or why you didn't
> sent)...

OK, DONE! Here are updated URLs for review:

Spec URL: https://psutter.fedorapeople.org/libkeepalive.spec
SRPM URL: https://psutter.fedorapeople.org/libkeepalive-0.3-3.fc25.src.rpm

Comment 6 Igor Gnatenko 2016-12-12 23:00:44 UTC

any plans to request package in PkgDB? ;)

Comment 7 Phil Sutter 2016-12-18 17:13:31 UTC

(In reply to Igor Gnatenko from comment #6)
> any plans to request package in PkgDB? ;)

Doh! I just caught up on that, thanks for the heads-up!

Cheers, Phil

Comment 8 Gwyn Ciesla 2016-12-19 13:53:38 UTC

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libkeepalive