Re: [PATCH] Fix 'make coverage' when used with lcov version 2.0+
Hi - > Well, even if it happened unintentionally, it was ignored just the same > as if it happened on purpose. I hope the technical reasons for that > are now fixed, and won't cause any problems next time. You have been in the open source community long enough to know that emails sometimes just get lost. It might happen again, and one one should not take too much offence. > Please note that elfutils is a community project, and the code in > question was contributed a few years ago by myself, see commit > 2a16a0fc7e353f8fcfc27a57710e008840297847. Yes, however we both know that this does not mean that a maintainer is required to affirmatively consult you (or anyone) on changes years after its contribution. > Frank, please reconsider your approach. Your comments are [...] I disagree. - FChE
Re: [PATCH] Fix 'make coverage' when used with lcov version 2.0+
Hi Frank, On Thu, May 09, 2024 at 07:03:47PM -0400, Frank Ch. Eigler wrote: > Hi - > > > > > What's the purpose of sending proposed patches to the mailing list > > > > if reviews are silently ignored? > > > > > > Please be collegial and don't exaggerate. > > > > The fact is that the review was silently ignored, which is, from my point > > of view, an extraordinary event, > > The review *singular*. And you can't know whether it was ignored, or > never received or missed or considered. Well, even if it happened unintentionally, it was ignored just the same as if it happened on purpose. I hope the technical reasons for that are now fixed, and won't cause any problems next time. > > [...] and request that the issue pointed out in the review to be > > properly addressed. > > May I suggest asking for that in a less accusatory and exaggerated > fashion next time. Note also that being "properly addressed" is up to > the discretion of the maintainers - one of whom is Aaron. Please note that elfutils is a community project, and the code in question was contributed a few years ago by myself, see commit 2a16a0fc7e353f8fcfc27a57710e008840297847. I'm glad the code duplication issue is now resolved. Frank, please reconsider your approach. Your comments are not just unhelpful and unproductive, they can actually discourage contributors which is the last thing one wants in a free software project. -- ldv
Re: [COMMITTED] Makefile.am: Avoid code duplication
On Thu, May 09, 2024 at 07:24:26PM -0400, Aaron Merey wrote: > On Thu, May 9, 2024 at 6:59 PM Dmitry V. Levin wrote: > > On Thu, May 09, 2024 at 06:08:05PM -0400, Frank Ch. Eigler wrote: > > > On Fri, May 10, 2024 at 12:53:39AM +0300, Dmitry V. Levin wrote: > > > > > Pushed as commit ca8ad4648197 > > > > > > > > What's the purpose of sending proposed patches to the mailing list > > > > if reviews are silently ignored? > > > > > > Please be collegial and don't exaggerate. > > > > The fact is that the review was silently ignored, which is, from my point > > of view, an extraordinary event, and I hereby raise the question why it > > was ignored, and request that the issue pointed out in the review to be > > properly addressed. > > Apologies Dmitry, your review was mistakenly caught in my mail client's > spam filter and I did not see it until now. > > I agree that we can avoid some code duplication here. I'm going to push > the following patch as obvious: > > --- > Makefile.am | 33 ++--- > 1 file changed, 6 insertions(+), 27 deletions(-) > > diff --git a/Makefile.am b/Makefile.am > index a1f0b0c3..69af63ac 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -78,24 +78,16 @@ coverage: $(COVERAGE_OUTPUT_INDEX_HTML) > @echo 'file://$(abs_builddir)/$(COVERAGE_OUTPUT_INDEX_HTML)' > > if LCOV_OLD > -$(COVERAGE_OUTPUT_INDEX_HTML): $(COVERAGE_OUTPUT_FILE) > - LC_ALL=C $(GENHTML) \ > - --legend \ > - --show-details \ > - --rc=genhtml_branch_coverage=1 \ > - --title='$(COVERAGE_TITLE)' \ > - --prefix='$(abspath $(abs_srcdir))' \ > - --prefix='$(realpath $(abs_srcdir))' \ > - --prefix='$(abspath $(abs_builddir)/..)' \ > - --prefix='$(realpath $(abs_builddir)/..)' \ > - --output-directory='$(COVERAGE_OUTPUT_DIRECTORY)' \ > - $< > +ignore_errors = > else > +ignore_errors = --ignore-errors empty,negative > +endif > + > $(COVERAGE_OUTPUT_INDEX_HTML): $(COVERAGE_OUTPUT_FILE) > LC_ALL=C $(GENHTML) \ > --legend \ > --show-details \ > - --ignore-errors empty,negative \ > + $(ignore_errors) \ > --rc=genhtml_branch_coverage=1 \ > --title='$(COVERAGE_TITLE)' \ > --prefix='$(abspath $(abs_srcdir))' \ > @@ -104,30 +96,17 @@ $(COVERAGE_OUTPUT_INDEX_HTML): $(COVERAGE_OUTPUT_FILE) > --prefix='$(realpath $(abs_builddir)/..)' \ > --output-directory='$(COVERAGE_OUTPUT_DIRECTORY)' \ > $< > -endif > > -if LCOV_OLD > $(COVERAGE_OUTPUT_FILE): > $(LCOV) \ > --capture \ > --no-external \ > --no-checksum \ > + $(ignore_errors) \ > --rc=lcov_branch_coverage=1 \ > --gcov-tool='$(GCOV)' \ > --output-file='$@' \ > $(LCOV_DIRS_ARGS) > -else > -$(COVERAGE_OUTPUT_FILE): > - $(LCOV) \ > - --capture \ > - --no-external \ > - --no-checksum \ > - --ignore-errors empty,negative \ > - --rc=lcov_branch_coverage=1 \ > - --gcov-tool='$(GCOV)' \ > - --output-file='$@' \ > - $(LCOV_DIRS_ARGS) > -endif > endif > > # Tell version 3.79 and up of GNU make to not build goals in this That's much better, thanks! -- ldv
[COMMITTED] Makefile.am: Avoid code duplication
On Thu, May 9, 2024 at 6:59 PM Dmitry V. Levin wrote: > > On Thu, May 09, 2024 at 06:08:05PM -0400, Frank Ch. Eigler wrote: > > Hi - > > > > On Fri, May 10, 2024 at 12:53:39AM +0300, Dmitry V. Levin wrote: > > > > Pushed as commit ca8ad4648197 > > > > > > What's the purpose of sending proposed patches to the mailing list > > > if reviews are silently ignored? > > > > Please be collegial and don't exaggerate. > > The fact is that the review was silently ignored, which is, from my point > of view, an extraordinary event, and I hereby raise the question why it > was ignored, and request that the issue pointed out in the review to be > properly addressed. Apologies Dmitry, your review was mistakenly caught in my mail client's spam filter and I did not see it until now. I agree that we can avoid some code duplication here. I'm going to push the following patch as obvious: --- Makefile.am | 33 ++--- 1 file changed, 6 insertions(+), 27 deletions(-) diff --git a/Makefile.am b/Makefile.am index a1f0b0c3..69af63ac 100644 --- a/Makefile.am +++ b/Makefile.am @@ -78,24 +78,16 @@ coverage: $(COVERAGE_OUTPUT_INDEX_HTML) @echo 'file://$(abs_builddir)/$(COVERAGE_OUTPUT_INDEX_HTML)' if LCOV_OLD -$(COVERAGE_OUTPUT_INDEX_HTML): $(COVERAGE_OUTPUT_FILE) - LC_ALL=C $(GENHTML) \ - --legend \ - --show-details \ - --rc=genhtml_branch_coverage=1 \ - --title='$(COVERAGE_TITLE)' \ - --prefix='$(abspath $(abs_srcdir))' \ - --prefix='$(realpath $(abs_srcdir))' \ - --prefix='$(abspath $(abs_builddir)/..)' \ - --prefix='$(realpath $(abs_builddir)/..)' \ - --output-directory='$(COVERAGE_OUTPUT_DIRECTORY)' \ - $< +ignore_errors = else +ignore_errors = --ignore-errors empty,negative +endif + $(COVERAGE_OUTPUT_INDEX_HTML): $(COVERAGE_OUTPUT_FILE) LC_ALL=C $(GENHTML) \ --legend \ --show-details \ - --ignore-errors empty,negative \ + $(ignore_errors) \ --rc=genhtml_branch_coverage=1 \ --title='$(COVERAGE_TITLE)' \ --prefix='$(abspath $(abs_srcdir))' \ @@ -104,30 +96,17 @@ $(COVERAGE_OUTPUT_INDEX_HTML): $(COVERAGE_OUTPUT_FILE) --prefix='$(realpath $(abs_builddir)/..)' \ --output-directory='$(COVERAGE_OUTPUT_DIRECTORY)' \ $< -endif -if LCOV_OLD $(COVERAGE_OUTPUT_FILE): $(LCOV) \ --capture \ --no-external \ --no-checksum \ + $(ignore_errors) \ --rc=lcov_branch_coverage=1 \ --gcov-tool='$(GCOV)' \ --output-file='$@' \ $(LCOV_DIRS_ARGS) -else -$(COVERAGE_OUTPUT_FILE): - $(LCOV) \ - --capture \ - --no-external \ - --no-checksum \ - --ignore-errors empty,negative \ - --rc=lcov_branch_coverage=1 \ - --gcov-tool='$(GCOV)' \ - --output-file='$@' \ - $(LCOV_DIRS_ARGS) -endif endif # Tell version 3.79 and up of GNU make to not build goals in this -- 2.43.0
Re: [PATCH] Fix 'make coverage' when used with lcov version 2.0+
Hi - > > > What's the purpose of sending proposed patches to the mailing list > > > if reviews are silently ignored? > > > > Please be collegial and don't exaggerate. > > The fact is that the review was silently ignored, which is, from my point > of view, an extraordinary event, The review *singular*. And you can't know whether it was ignored, or never received or missed or considered. > [...] and request that the issue pointed out in the review to be > properly addressed. May I suggest asking for that in a less accusatory and exaggerated fashion next time. Note also that being "properly addressed" is up to the discretion of the maintainers - one of whom is Aaron. - FChE
Re: [PATCH] Fix 'make coverage' when used with lcov version 2.0+
On Thu, May 09, 2024 at 06:08:05PM -0400, Frank Ch. Eigler wrote: > Hi - > > On Fri, May 10, 2024 at 12:53:39AM +0300, Dmitry V. Levin wrote: > > > Pushed as commit ca8ad4648197 > > > > What's the purpose of sending proposed patches to the mailing list > > if reviews are silently ignored? > > Please be collegial and don't exaggerate. The fact is that the review was silently ignored, which is, from my point of view, an extraordinary event, and I hereby raise the question why it was ignored, and request that the issue pointed out in the review to be properly addressed. Thanks, -- ldv
Re: [PATCH] Fix 'make coverage' when used with lcov version 2.0+
Hi - On Fri, May 10, 2024 at 12:53:39AM +0300, Dmitry V. Levin wrote: > > Pushed as commit ca8ad4648197 > > What's the purpose of sending proposed patches to the mailing list > if reviews are silently ignored? Please be collegial and don't exaggerate. - FChE
Re: [PATCH] Fix 'make coverage' when used with lcov version 2.0+
On Wed, May 08, 2024 at 12:28:04PM -0400, Aaron Merey wrote: > On Mon, May 6, 2024 at 4:45 PM Aaron Merey wrote: > > > > Starting with version 2.0, various lcov warnings now trigger an error > > exit. This results in 'make coverage' terminating before completion. > > > > Fix this by invoking lcov and genhtml with --ignore-errors to prevent > > the error exit when version 2.0+ is in use. > > > > Manually tested by running elfutils-htdocs/update-coverage.sh with > > lcov 1.14 and 2.0. > > > > Signed-off-by: Aaron Merey > > --- > > Makefile.am | 30 +- > > configure.ac | 4 > > 2 files changed, 33 insertions(+), 1 deletion(-) > > Pushed as commit ca8ad4648197 What's the purpose of sending proposed patches to the mailing list if reviews are silently ignored? -- ldv
Re: [rfc] [patch] PR28204: debuginfod ima signature verification
Hi Frank,
I've pointed out a couple nits below, but otherwise the patch LGTM.
I've also attached a diff for handling DEBUGINFOD_IMA_CERT_PATH in
profile.fish.in that should apply on top of this patch.
I know there's already been a lot of discussion re. ima:permissive and
I'm weighing in rather late, but FWIW I do support including it.
Currently individual ELF sections cannot be downloaded when
ima:enforcing is active. With ima:permissive we could support proper
section queries while also being able to perform some amount of
ima verification.
On Tue, Apr 16, 2024 at 6:15 PM Frank Ch. Eigler wrote:
>
> Hi -
>
> The following is the candidate patch for the basic functionality.
> It's been corrected for whitespace & error codes, given more complete
> docs and commit message. See also the users/fche/try-bz2824f branch.
>
>
> debuginfod: PR28204 - RPM IMA per-file signature verification
>
> Recent versions of Fedora/RHEL include per-file cryptographic
> signatures in RPMs, not just an overall RPM signature. This work
> extends debuginfod client & server to extract, transfer, and verify
> those signatures. These allow clients to assure users that the
> downloaded files have not been corrupted since their original
> packaging. Downloads that fail the test are rejected.
>
> Clients may select a desired level of enforcement for sets of URLs in
> the DEBUGINFOD_URLS by inserting special markers ahead of them:
>
> ima:ignore pay no attention to absence or presence of signatures
> ima:enforcingrequire every file to be correctly signed
>
> The default is ima:ignore mode. In ima:enforcing mode, section
> queries are forced to be entire-file downloads, as it is not
> possible to crypto-verify just sections.
>
> IMA signatures are verified against a set of signing certificates.
> These are normally published by distributions. The environment
> variable $DEBUGINFOD_IMA_CERT_PATH contains a colon-separated path for
> finding DER or PEM formatted certificates / public keys. These
> certificates are assumed trusted. The profile.d scripts transcribe
> /etc/debuginfod/*.certdir files into that variable.
>
> As for implementation:
>
> * configure.ac: Add --enable-default-ima-cert-path=PATH parameter.
> Check for libimaevm (using headers only).
>
> * config/Makefile.am: Install defaults into /etc files.
> * config/profile.{csh,sh}.in: Process defaults into env variables.
> * config/elfutils.spec.in: Add more buildrequires.
>
> * debuginfod/debuginfod.cxx (handle_buildid_r_match): Added extraction of
> the
> per-file IMA signature for the queried file and store in http header.
> (find_globbed_koji_filepath): New function.
> (parse_opt): New flag --koji-sigcache.
> * debuginfod/debuginfod-client.c (debuginfod_query_server): Added policy
> for
> validating IMA signatures
> (debuginfod_validate_imasig): New function, with friends.
> * debuginfod/debuginfod.h.in: Added DEBUGINFOD_IMA_CERT_PATH_ENV_VAR.
> * debuginfod/Makefile.am: Add linker flags for rpm and crypto.
>
> * doc/debuginfod-client-config.7: Document DEBUGINFOD_IMA_CERT_PATH,
> update DEBUGINFOD_URLS.
> * doc/debuginfod.8: Document --koji-sigcache.
> * doc/debuginfod-find.1, doc/debuginfod_find_debuginfo.3: Update SECURITY.
>
> * tests/run-debuginfod-ima-verification.sh: New test.
> * tests/debuginfod-ima: Some new files for the tests.
> * tests/Makefile.am: run/distribute them.
>
> Signed-off-by: Ryan Goldberg
> Signed-off-by: Frank Ch. Eigler
>
> diff --git a/config/Makefile.am b/config/Makefile.am
> index ae14e625b726..5a28e66d4408 100644
> --- a/config/Makefile.am
> +++ b/config/Makefile.am
> @@ -46,12 +46,16 @@ pkgconfig_DATA += libdebuginfod.pc
> if [ -n "@DEBUGINFOD_URLS@" ]; then \
> echo "@DEBUGINFOD_URLS@" >
> $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.urls; \
> fi
> + if [ -n "@DEBUGINFOD_IMA_CERT_PATH@" ]; then \
> + echo "@DEBUGINFOD_IMA_CERT_PATH@" >
> $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.certpath; \
> + fi
>
> uninstall-local:
> rm -f $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.sh
> rm -f $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.csh
> rm -f $(DESTDIR)$(datadir)/fish/vendor_conf.d/debuginfod.fish
> rm -f $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.urls
> + rm -f $(DESTDIR)$(sysconfdir)/debuginfod/elfutils.certpath
> -rmdir $(DESTDIR)$(sysconfdir)/debuginfod
> endif
>
> diff --git a/config/elfutils.spec.in b/config/elfutils.spec.in
> index 4d802a25ad5f..460729972420 100644
> --- a/config/elfutils.spec.in
> +++ b/config/elfutils.spec.in
> @@ -43,6 +43,12 @@ BuildRequires: curl
> # For run-debuginfod-response-headers.sh test case
> BuildRequires: socat
>
> +# For debuginfod rpm IMA verification
> +BuildRequires: rpm-devel
>
