Re: patch 2/2 debuginfod server etc.

[Mark Wielaard <mark@klomp.org>]

Hi,

On Mon, 2019-10-28 at 15:07 -0400, Frank Ch. Eigler wrote:
> Add the server to the debuginfod/ subdirectory.  This is a highly
> multithreaded c++11 program (still buildable on rhel7's gcc 4.8,
> which is only partly c++11 compliant).  Includes an initial suite
> of tests, man pages, and a sample systemd service.

Some feedback on the first part, the config directory additions.

> diff --git a/config/ChangeLog b/config/ChangeLog
> index b641d0d5b74e..73643f919406 100644
> --- a/config/ChangeLog
> +++ b/config/ChangeLog
> @@ -1,3 +1,10 @@
> +2019-10-28  Frank Ch. Eigler  <fche@redhat.com>
> +
> +	* eu.am (AM_CXXFLAGS): Clone & amend AM_CFLAGS for c++11 code.
> +	* debuginfod.service, debuginfod.sysconfig: New files: systemd.
> +	* Makefile.am: Install them.
> +	* elfutils.spec.in: Add debuginfod and debuginfod-client subrpms.
> +
>  2019-08-29  Mark Wielaard  <mark@klomp.org>
>  
>  	* elfutils.spec.in (%description devel): Remove libebl text.
> diff --git a/config/Makefile.am b/config/Makefile.am
> index 6425718efc54..7c75f59880d3 100644
> --- a/config/Makefile.am
> +++ b/config/Makefile.am
> @@ -28,8 +28,8 @@
>  ## the GNU Lesser General Public License along with this program.  If
>  ## not, see <http://www.gnu.org/licenses/>.
>  ##
> -EXTRA_DIST = elfutils.spec.in known-dwarf.awk 10-default-yama-scope.conf
> -	     libelf.pc.in libdw.pc.in
> +EXTRA_DIST = elfutils.spec.in known-dwarf.awk 10-default-yama-scope.conf \
> +	libelf.pc.in libdw.pc.in libdebuginfod.pc.in debuginfod.service debuginfod.sysconfig
>  
>  pkgconfigdir = $(libdir)/pkgconfig
>  pkgconfig_DATA = libelf.pc libdw.pc libdebuginfod.pc

OK.

> diff --git a/config/debuginfod.service b/config/debuginfod.service
> new file mode 100644
> index 000000000000..15a8808baf0c
> --- /dev/null
> +++ b/config/debuginfod.service
> @@ -0,0 +1,15 @@
> +[Unit]
> +Description=elfutils debuginfo-over-http server
> +Documentation=http://elfutils.org/
> +After=network.target
> +
> +[Service]
> +EnvironmentFile=/etc/sysconfig/debuginfod
> +User=debuginfod
> +Group=debuginfod
> +#CacheDirectory=debuginfod
> +ExecStart=/usr/bin/debuginfod -d /var/cache/debuginfod/debuginfod.sqlite -p $DEBUGINFOD_PORT $DEBUGINFOD_VERBOSE $DEBUGINFOD_PATHS
> +TimeoutStopSec=10

Why is CacheDirectory commented out, I cannot find it anywhere else in
the code.

> +[Install]
> +WantedBy=multi-user.target
> diff --git a/config/debuginfod.sysconfig b/config/debuginfod.sysconfig
> new file mode 100644
> index 000000000000..bee42a4f443a
> --- /dev/null
> +++ b/config/debuginfod.sysconfig
> @@ -0,0 +1,9 @@
> +#
> +DEBUGINFOD_PORT="8002"
> +#DEBUGINFOD_VERBOSE="-v"
> +DEBUGINFOD_PATHS="/usr/lib/debug /usr/bin /usr/sbin /usr/lib /usr/lib64 /usr/local"

Should this also include /usr/libexec ?
Isn't /usr/local too broad? Should it also include /opt and/or /srv?

> +# upstream debuginfods
> +#DEBUGINFOD_URLS="http://secondhost:8002 http://thirdhost:8002"
> +#DEBUGINFOD_TIMEOUT="5"
> +#DEBUGINFOD_CACHE_DIR=""
> diff --git a/config/elfutils.spec.in b/config/elfutils.spec.in
> index 6771d13ba740..694f8fde2f48 100644
> --- a/config/elfutils.spec.in
> +++ b/config/elfutils.spec.in
> @@ -23,10 +23,22 @@ BuildRequires: flex >= 2.5.4a
>  BuildRequires: bzip2
>  BuildRequires: m4
>  BuildRequires: gettext
> -BuildRequires: zlib-devel
> +BuildRequires: pkgconfig(zlib)
> +%if 0%{?rhel} == 7
>  BuildRequires: bzip2-devel
> -BuildRequires: xz-devel
> +%else
> +BuildRequires: pkgconfig(bzip2)
> +%endif
> +BuildRequires: pkgconfig(liblzma)

So now we only need the -devel package for bzip2 because it might not
have a pkgconfig file. Good.

I am slightly confused about xz-devel vs liblzma.
Could you remind me again what the relation was?
I see we do check for liblzma with pkgconfig in configure.ac.
So I assume the change is correct. Just confused about the naming.

>  BuildRequires: gcc-c++
> +BuildRequires: pkgconfig(libmicrohttpd) >= 0.9.33
> +BuildRequires: pkgconfig(libcurl) >= 7.29.0
> +BuildRequires: pkgconfig(sqlite3) >= 3.7.17
> +BuildRequires: pkgconfig(libarchive) >= 3.1.2
> +%if 0%{?rhel} >= 8 || 0%{?fedora} >= 20
> +Recommends: elfutils-debuginfod-client
> +%endif
> +

Should we add %else Requires: elfutils-debuginfod-client?
Also I think it would be more correct to move the Recommends/Requires up with the other Requires.
(Side note, in fedora we actually have a separate libs subpackage, in fedora it should be moved there.)
 
>  %define _gnu %{nil}
>  %define _programprefix eu-
> @@ -116,18 +128,45 @@ interprocess services, communication and introspection
>  (like synchronisation, signaling, debugging, tracing and
>  profiling) of processes.
>  
> +%package debuginfod-client
> +Summary: Libraries and command-line frontend for HTTP ELF/DWARF file server addressed by build-id.
> +License: GPLv2+

That should probably be:
GPLv3+ and (GPLv2+ or LGPLv3+)
^ for the binary, ^ for the library

> +%package debuginfod
> +Summary: HTTP ELF/DWARF file server addressed by build-id.
> +License: GPLv2+

GPLv3+

> +BuildRequires: systemd
> +Requires(post):   systemd
> +Requires(preun):  systemd
> +Requires(postun): systemd
> +Requires: shadow-utils
> +Requires: /usr/bin/rpm2cpio

Should that be Requires(pre): shadow-utils?
Because you only need it it the pre-script?

> +%description debuginfod-client
> +The elfutils-debuginfod-client package contains shared libraries
> +dynamically loaded from -ldw, which use a debuginfod service
> +to look up debuginfo and associated data. Also includes a
> +command-line frontend.
> +
> +%description debuginfod
> +The elfutils-debuginfod package contains the debuginfod binary
> +and control files for a service that can provide ELF/DWARF
> +files to remote clients, based on build-id identification.
> +The ELF/DWARF file searching functions in libdwfl can query
> +such servers to download those files on demand.
> +
>  %prep
>  %setup -q
>  
>  %build
> -%configure --program-prefix=%{_programprefix}
> +%configure --program-prefix=%{_programprefix} --enable-debuginfod
>  make
>  
>  %install
>  rm -rf ${RPM_BUILD_ROOT}
>  mkdir -p ${RPM_BUILD_ROOT}%{_prefix}
>  
> -%makeinstall
> +%make_install

O. Why? What?
Probably fine, just different.

>  chmod +x ${RPM_BUILD_ROOT}%{_prefix}/%{_lib}/lib*.so*
>  
> @@ -140,6 +179,11 @@ chmod +x ${RPM_BUILD_ROOT}%{_prefix}/%{_lib}/lib*.so*
>  
>  install -Dm0644 config/10-default-yama-scope.conf ${RPM_BUILD_ROOT}%{_sysctldir}/10-default-yama-scope.conf
>  
> +install -Dm0644 config/debuginfod.service ${RPM_BUILD_ROOT}%{_unitdir}/debuginfod.service
> +install -Dm0644 config/debuginfod.sysconfig ${RPM_BUILD_ROOT}%{_sysconfdir}/sysconfig/debuginfod
> +mkdir -p ${RPM_BUILD_ROOT}%{_localstatedir}/cache/debuginfod
> +touch ${RPM_BUILD_ROOT}%{_localstatedir}/cache/debuginfod/debuginfod.sqlite
> +
>  %check
>  make check

OK.
 
> @@ -190,6 +234,7 @@ rm -rf ${RPM_BUILD_ROOT}
>  %dir %{_includedir}/elfutils
>  %{_includedir}/elfutils/elf-knowledge.h
>  %{_includedir}/elfutils/known-dwarf.h
> +%{_includedir}/elfutils/debuginfod.h
>  #%{_includedir}/elfutils/libasm.h
>  %{_includedir}/elfutils/libdw.h
>  %{_includedir}/elfutils/libdwfl.h
> @@ -197,6 +242,7 @@ rm -rf ${RPM_BUILD_ROOT}
>  %{_includedir}/elfutils/version.h
>  #%{_libdir}/libasm.so
>  %{_libdir}/libdw.so
> +%{_libdir}/libdebuginfod.so
>  %{_libdir}/pkgconfig/libdw.pc

Since the debuginfod client stuff is actually independent from the
other devel libraries I think these should not go here, but...

>  %files devel-static
> @@ -225,6 +271,43 @@ rm -rf ${RPM_BUILD_ROOT}
>  %files default-yama-scope
>  %{_sysctldir}/10-default-yama-scope.conf
>  
> +
> +%files debuginfod-client
> +%defattr(-,root,root)
> +%{_libdir}/libdebuginfod.so.*
> +%{_libdir}/libdebuginfod-%{version}.so
> +%{_libdir}/pkgconfig/libdebuginfod.pc
> +%{_bindir}/debuginfod-find
> +%{_mandir}/man1/debuginfod-find.1*
> +%{_mandir}/man3/debuginfod_find_debuginfo.3*
> +%{_mandir}/man3/debuginfod_find_executable.3*
> +%{_mandir}/man3/debuginfod_find_source.3*

...here, but I think we should also split this into debuginfod-client
with just the shared library and binary plus manpage. And debuginfod-
client-devel with the header file, pkgconfig and other man-pages.

> +%files debuginfod
> +%defattr(-,root,root)
> +%{_bindir}/debuginfod
> +%config(noreplace) %verify(not md5 size mtime) %{_sysconfdir}/sysconfig/debuginfod
> +%{_unitdir}/debuginfod.service
> +%{_sysconfdir}/sysconfig/debuginfod
> +%{_mandir}/man8/debuginfod.8*
> +
> +%dir %attr(0700,debuginfod,debuginfod) %{_localstatedir}/cache/debuginfod
> +%verify(not md5 size mtime) %attr(0600,debuginfod,debuginfod) %{_localstatedir}/cache/debuginfod/debuginfod.sqlite
> +
> +%pre debuginfod
> +getent group debuginfod >/dev/null || groupadd -r debuginfod
> +getent passwd debuginfod >/dev/null || \
> +    useradd -r -g debuginfod -d /var/cache/debuginfod -s /sbin/nologin \
> +            -c "elfutils debuginfo server" debuginfod
> +exit 0

OK, I see this follows 
https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_dynamic_allocation

> +%post debuginfod
> +%systemd_post debuginfod.service
> +
> +%postun debuginfod
> +%systemd_postun_with_restart debuginfod.service
> +
>  %changelog
>  * Tue Aug 13 2019 Mark Wielaard <mark@klomp.org> 0.177-1
>  - elfclassify: New tool to analyze ELF objects.
> diff --git a/config/eu.am b/config/eu.am
> index 82acda3ab2cd..6c3c444f143a 100644
> --- a/config/eu.am
> +++ b/config/eu.am
> @@ -79,6 +79,16 @@ AM_CFLAGS = -std=gnu99 -Wall -Wshadow -Wformat=2 \
>  	    $(if $($(*F)_no_Wpacked_not_aligned),-Wno-packed-not-aligned,) \
>  	    $($(*F)_CFLAGS)
>  
> +AM_CXXFLAGS = -std=c++11 -Wall -Wshadow \
> +	   -Wtrampolines \
> +	   $(LOGICAL_OP_WARNING) $(DUPLICATED_COND_WARNING) \
> +	   $(NULL_DEREFERENCE_WARNING) $(IMPLICIT_FALLTHROUGH_WARNING) \
> +	   $(if $($(*F)_no_Werror),,-Werror) \
> +	   $(if $($(*F)_no_Wunused),,-Wunused -Wextra) \
> +	   $(if $($(*F)_no_Wstack_usage),,$(STACK_USAGE_WARNING)) \
> +	   $(if $($(*F)_no_Wpacked_not_aligned),-Wno-packed-not-aligned,) \
> +	   $($(*F)_CXXFLAGS)
> +
>  COMPILE.os = $(filter-out -fprofile-arcs -ftest-coverage, $(COMPILE))
>  
>  DEFS.os = -DPIC -DSHARED

Looks good.
Maybe there are some C++ specific warnings we could enable?
Not a priority of course.

Thanks,

Mark