* Re: [PATCH 1/2] KMI Whitelists: Add functionality to make whitelists additive
2020-01-01 0:00 [PATCH 1/2] KMI Whitelists: Add functionality to make whitelists additive Matthias Maennich via libabigail
@ 2020-01-01 0:00 ` Matthias Maennich via libabigail
2020-01-01 0:00 ` Dodji Seketeli
2020-01-01 0:00 ` [PATCH 2/2] KMI Whitelists: Drop old whitelist extraction methods Matthias Maennich via libabigail
2 siblings, 0 replies; 6+ messages in thread
From: Matthias Maennich via libabigail @ 2020-01-01 0:00 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team
On Tue, Jan 14, 2020 at 05:34:49PM +0000, Matthias Maennich wrote:
>If multiple KMI whitelists are specified, either by passing
>--kmi-whitelist several times or by having multiple whitelist sections
>in the whitelist files, the generated suppressions are created as an
>intersection of symbols. That is rather unusual, as whitelisting should
>rather work additive. That means that the symbols (or expressions
>thereof) defined across several sections or files shall be considered a
>union of symbols. This patch combines the whitelist parsing to create
>exactly one function_suppression and one variable suppression. A test
>case has been added to ensure the functionality is working.
>
>Please note, migrating the existing code to this new functionality is
>done in a separate commit.
>
> * include/abg-tools-utils.h
> (gen_suppr_spec_from_kernel_abi_whitelists): New function.
> * src/abg-tools-utils.cc
> (gen_suppr_spec_from_kernel_abi_whitelists): Likewise.
> * tests/.gitignore: Ignore new test executable.
> * tests/Makefile.am: Add new test executable.
> * tests/data/test-kmi-whitelist/whitelist-with-another-single-entry:
> New test input file.
> * tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry:
> Likewise.
> * tests/data/test-kmi-whitelist/whitelist-with-single-entry:
> Likewise.
> * tests/data/test-kmi-whitelist/whitelist-with-two-sections:
> Likewise.
> * tests/data/Makefile.am: Add above test material.
> * tests/test-kmi-whitelist.cc: Add new test executable.
>
>Signed-off-by: Matthias Maennich <maennich@google.com>
>---
> include/abg-tools-utils.h | 4 +
> src/abg-tools-utils.cc | 113 ++++++++++++
> tests/.gitignore | 1 +
> tests/Makefile.am | 4 +
> tests/data/Makefile.am | 7 +-
> .../whitelist-with-another-single-entry | 2 +
> .../whitelist-with-duplicate-entry | 3 +
> .../whitelist-with-single-entry | 2 +
> .../whitelist-with-two-sections | 5 +
> tests/test-kmi-whitelist.cc | 162 ++++++++++++++++++
> 10 files changed, 302 insertions(+), 1 deletion(-)
> create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-another-single-entry
> create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry
> create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-single-entry
> create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-two-sections
> create mode 100644 tests/test-kmi-whitelist.cc
>
>diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
>index a153af689740..ea44e4dd36aa 100644
>--- a/include/abg-tools-utils.h
>+++ b/include/abg-tools-utils.h
>@@ -90,6 +90,10 @@ bool
> gen_suppr_spec_from_kernel_abi_whitelist(const string& abi_whitelist_path,
> suppr::suppressions_type& s);
>
>+suppr::suppressions_type
>+gen_suppr_spec_from_kernel_abi_whitelists(
>+ const std::vector<string>& abi_whitelist_paths);
>+
> bool
> get_vmlinux_path_from_kernel_dist(const string& from,
> string& vmlinux_path);
>diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
>index 0495905253ed..dcc302546beb 100644
>--- a/src/abg-tools-utils.cc
>+++ b/src/abg-tools-utils.cc
>@@ -38,8 +38,10 @@
> #include <sys/time.h>
> #include <dirent.h>
> #include <time.h>
>+#include <algorithm>
> #include <cstdlib>
> #include <cstring>
>+#include <iterator>
> #include <ctype.h>
> #include <errno.h>
> #include <libgen.h>
>@@ -1870,6 +1872,117 @@ gen_suppr_spec_from_headers(const string& headers_root_dir)
> return result;
> }
>
>+/// Generate a suppression specification from kernel abi whitelist
>+/// files.
>+///
>+/// A kernel ABI whitelist file is an INI file that usually has only
>+/// one section. The name of the section is a string that ends up
>+/// with the sub-string "whitelist". For instance
>+/// RHEL7_x86_64_whitelist.
>+///
>+/// Then the content of the section is a set of function or variable
>+/// names, one name per line. Each function or variable name is the
>+/// name of a function or a variable whose changes are to be keept.
>+///
>+/// A whitelist file can have multiple sections (adhering to the naming
>+/// conventions and multiple files can be passed. The suppression that
>+/// is created takes all whitelist sections from all files into account.
>+/// Symbols (or expression of such) are deduplicated in the final
>+/// suppression expression.
>+///
>+/// This function reads the white lists and generates a
>+/// function_suppression_sptr and variable_suppression_sptr and returns
>+/// a vector containing those.
>+///
>+/// @param abi_whitelist_paths a vector of KMI whitelist paths
>+///
>+/// @return a vector or suppressions
>+suppressions_type
>+gen_suppr_spec_from_kernel_abi_whitelists(
>+ const std::vector<std::string>& abi_whitelist_paths)
>+{
>+
>+ std::vector<std::string> whitelisted_names;
>+ for (std::vector<std::string>::const_iterator path_iter
>+ = abi_whitelist_paths.begin(),
>+ path_end = abi_whitelist_paths.end();
>+ path_iter != path_end; ++path_iter)
>+ {
>+
>+ abigail::ini::config whitelist;
>+ if (!read_config(*path_iter, whitelist))
>+ continue;
>+
>+ const ini::config::sections_type& whitelist_sections
>+ = whitelist.get_sections();
>+
>+ for (ini::config::sections_type::const_iterator section_iter
>+ = whitelist_sections.begin(),
>+ section_end = whitelist_sections.end();
>+ section_iter != section_end; ++section_iter)
>+ {
>+ std::string section_name = (*section_iter)->get_name();
>+ if (!string_ends_with(section_name, "whitelist"))
>+ continue;
>+ for (ini::config::properties_type::const_iterator prop_iter
>+ = (*section_iter)->get_properties().begin(),
>+ prop_end = (*section_iter)->get_properties().end();
>+ prop_iter != prop_end; ++prop_iter)
>+ {
>+ if (const simple_property_sptr& prop
>+ = is_simple_property(*prop_iter))
>+ if (prop->has_empty_value())
>+ {
>+ const std::string& name = prop->get_name();
>+ if (!name.empty())
>+ whitelisted_names.push_back(name);
>+ }
>+ }
>+ }
>+ }
>+
>+ suppressions_type result;
>+ if (!whitelisted_names.empty())
>+ {
>+ // Drop duplicates
>+ whitelisted_names.erase(
>+ std::unique(whitelisted_names.begin(), whitelisted_names.end()),
>+ whitelisted_names.end());
>+
>+ // Build the regular expression to suppress non whitelisted symbols.
>+ std::stringstream regex_ss;
>+ regex_ss << "^";
>+ std::copy(whitelisted_names.begin(), whitelisted_names.end(),
>+ std::ostream_iterator<std::string>(regex_ss, "$|^"));
>+ regex_ss.seekp(0, std::ios::end);
>+ const std::string& regex = regex_ss.str().substr(
>+ 0, static_cast<size_t>(regex_ss.tellp()) - 2);
>+
>+ // Build a suppression specification which *keeps* functions
>+ // whose ELF symbols match the regular expression contained
>+ // in function_names_regexp. This will also keep the ELF
>+ // symbols (not designated by any debug info) whose names
>+ // match this regexp.
>+ function_suppression_sptr fn_suppr(new function_suppression);
>+ fn_suppr->set_label("whitelist");
>+ fn_suppr->set_symbol_name_not_regex_str(regex);
>+ fn_suppr->set_drops_artifact_from_ir(true);
>+ result.push_back(fn_suppr);
>+
>+ // Build a suppression specification which *keeps* variables
>+ // whose ELF symbols match the regular expression contained
>+ // in function_names_regexp. This will also keep the ELF
>+ // symbols (not designated by any debug info) whose names
>+ // match this regexp.
>+ variable_suppression_sptr var_suppr(new variable_suppression);
>+ var_suppr->set_label("whitelist");
>+ var_suppr->set_symbol_name_not_regex_str(regex);
>+ var_suppr->set_drops_artifact_from_ir(true);
>+ result.push_back(var_suppr);
>+ }
>+ return result;
>+}
>+
> /// Generate a suppression specification from kernel abi whitelist
> /// files.
> ///
>diff --git a/tests/.gitignore b/tests/.gitignore
>index 05451408c4f7..dec5f38802a7 100644
>--- a/tests/.gitignore
>+++ b/tests/.gitignore
>@@ -21,6 +21,7 @@ runtestdifffilter
> runtestdiffpkg
> runtestdiffsuppr
> runtestini
>+runtestkmiwhitelist
> runtestlookupsyms
> runtestreaddwarf
> runtestreadwrite
>diff --git a/tests/Makefile.am b/tests/Makefile.am
>index 14e83608b0e5..7baa623a0289 100644
>--- a/tests/Makefile.am
>+++ b/tests/Makefile.am
>@@ -45,6 +45,7 @@ runtestcorediff \
> runtestabidiffexit \
> runtestini \
> runtesttoolsutils \
>+runtestkmiwhitelist \
> $(CXX11_TESTS)
>
> if ENABLE_RUNNING_TESTS_WITH_PY3
>@@ -136,6 +137,9 @@ runtestini_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
> runtesttoolsutils_SOURCES = test-tools-utils.cc
> runtesttoolsutils_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
>
>+runtestkmiwhitelist_SOURCES = test-kmi-whitelist.cc
>+runtestkmiwhitelist_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
>+
> runtestsvg_SOURCES=test-svg.cc
> runtestsvg_LDADD=$(top_builddir)/src/libabigail.la
>
>diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
>index 214a110a7595..c5ac1ff35189 100644
>--- a/tests/data/Makefile.am
>+++ b/tests/data/Makefile.am
>@@ -1590,4 +1590,9 @@ test-fedabipkgdiff/nss-util/nss-util-3.12.6-1.fc14.x86_64.rpm \
> test-fedabipkgdiff/nss-util/nss-util-3.24.0-2.0.fc25.x86_64.rpm \
> \
> test-ini/test01-equal-in-property-string.abignore.expected \
>-test-ini/test01-equal-in-property-string.abignore
>+test-ini/test01-equal-in-property-string.abignore \
>+\
>+test-kmi-whitelist/whitelist-with-single-entry \
>+test-kmi-whitelist/whitelist-with-another-single-entry \
>+test-kmi-whitelist/whitelist-with-duplicate-entry \
>+test-kmi-whitelist/whitelist-with-two-sections
>diff --git a/tests/data/test-kmi-whitelist/whitelist-with-another-single-entry b/tests/data/test-kmi-whitelist/whitelist-with-another-single-entry
>new file mode 100644
>index 000000000000..dc601b8b8119
>--- /dev/null
>+++ b/tests/data/test-kmi-whitelist/whitelist-with-another-single-entry
>@@ -0,0 +1,2 @@
>+[abi_whitelist]
>+ test_another_symbol
>diff --git a/tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry b/tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry
>new file mode 100644
>index 000000000000..1721e76c946b
>--- /dev/null
>+++ b/tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry
>@@ -0,0 +1,3 @@
>+[abi_whitelist]
>+ test_symbol
>+ test_symbol
>diff --git a/tests/data/test-kmi-whitelist/whitelist-with-single-entry b/tests/data/test-kmi-whitelist/whitelist-with-single-entry
>new file mode 100644
>index 000000000000..748db09e09f2
>--- /dev/null
>+++ b/tests/data/test-kmi-whitelist/whitelist-with-single-entry
>@@ -0,0 +1,2 @@
>+[abi_whitelist]
>+ test_symbol
>diff --git a/tests/data/test-kmi-whitelist/whitelist-with-two-sections b/tests/data/test-kmi-whitelist/whitelist-with-two-sections
>new file mode 100644
>index 000000000000..7efeef273c9f
>--- /dev/null
>+++ b/tests/data/test-kmi-whitelist/whitelist-with-two-sections
>@@ -0,0 +1,5 @@
>+[abi_whitelist]
>+ test_symbol1
>+
>+[abi2_whitelist]
>+ test_symbol2
>diff --git a/tests/test-kmi-whitelist.cc b/tests/test-kmi-whitelist.cc
>new file mode 100644
>index 000000000000..355ee2a650d7
>--- /dev/null
>+++ b/tests/test-kmi-whitelist.cc
>@@ -0,0 +1,162 @@
>+// -*- Mode: C++ -*-
>+//
>+// Copyright (C) 2020 Google, Inc.
>+//
>+// This file is part of the GNU Application Binary Interface Generic
>+// Analysis and Instrumentation Library (libabigail). This library is
>+// free software; you can redistribute it and/or modify it under the
>+// terms of the GNU Lesser General Public License as published by the
>+// Free Software Foundation; either version 3, or (at your option) any
>+// later version.
>+
>+// This library is distributed in the hope that it will be useful, but
>+// WITHOUT ANY WARRANTY; without even the implied warranty of
>+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>+// General Lesser Public License for more details.
>+
>+// You should have received a copy of the GNU Lesser General Public
>+// License along with this program; see the file COPYING-LGPLV3. If
>+// not, see <http://www.gnu.org/licenses/>.
>+
>+// Author: Matthias Maennich
>+
>+/// @file
>+///
>+/// This program tests suppression generation from KMI whitelists.
>+
>+#include <string>
>+
>+#include "abg-fwd.h"
>+#include "abg-suppression.h"
>+#include "abg-tools-utils.h"
>+#include "test-utils.h"
>+
>+using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelists;
>+using abigail::suppr::suppression_sptr;
>+using abigail::suppr::suppressions_type;
>+using abigail::suppr::function_suppression_sptr;
>+using abigail::suppr::variable_suppression_sptr;
>+using abigail::suppr::is_function_suppression;
>+using abigail::suppr::is_variable_suppression;
>+
>+const static std::string whitelist_with_single_entry
>+ = std::string(abigail::tests::get_src_dir())
>+ + "/tests/data/test-kmi-whitelist/whitelist-with-single-entry";
>+
>+const static std::string whitelist_with_another_single_entry
>+ = std::string(abigail::tests::get_src_dir())
>+ + "/tests/data/test-kmi-whitelist/whitelist-with-another-single-entry";
>+
>+const static std::string whitelist_with_two_sections
>+ = std::string(abigail::tests::get_src_dir())
>+ + "/tests/data/test-kmi-whitelist/whitelist-with-two-sections";
>+
>+const static std::string whitelist_with_duplicate_entry
>+ = std::string(abigail::tests::get_src_dir())
>+ + "/tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry";
>+
>+bool
>+suppressions_are_consistent(const suppressions_type& suppr,
>+ const std::string& expr)
>+{
>+ if (suppr.size() != 2)
>+ return false;
>+
>+ function_suppression_sptr left = is_function_suppression(suppr[0]);
>+ variable_suppression_sptr right = is_variable_suppression(suppr[1]);
>+
>+ return // correctly casted
>+ (left && right)
>+ // same label
>+ && (left->get_label() == right->get_label())
>+ // same mode
>+ && (left->get_drops_artifact_from_ir()
>+ == right->get_drops_artifact_from_ir())
>+ // same regex
>+ && (left->get_symbol_name_not_regex_str()
>+ == right->get_symbol_name_not_regex_str())
>+ // regex as expected
>+ && (left->get_symbol_name_not_regex_str() == expr);
>+}
>+
>+bool
>+testNoWhitelist()
>+{
>+ const std::vector<std::string> abi_whitelist_paths;
>+ suppressions_type suppr
>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>+ return suppr.empty();
>+}
>+
>+bool
>+testSingleEntryWhitelist()
>+{
>+ std::vector<std::string> abi_whitelist_paths;
>+ abi_whitelist_paths.push_back(whitelist_with_single_entry);
>+ suppressions_type suppr
>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>+ return !suppr.empty() && suppressions_are_consistent(suppr, "^test_symbol$");
>+}
>+
>+bool
>+testWhitelistWithDuplicateEntries()
>+{
>+ std::vector<std::string> abi_whitelist_paths;
>+ abi_whitelist_paths.push_back(whitelist_with_duplicate_entry);
>+ suppressions_type suppr
>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>+ return !suppr.empty() && suppressions_are_consistent(suppr, "^test_symbol$");
>+}
>+
>+bool
>+testTwoWhitelists()
>+{
>+ std::vector<std::string> abi_whitelist_paths;
>+ abi_whitelist_paths.push_back(whitelist_with_single_entry);
>+ abi_whitelist_paths.push_back(whitelist_with_another_single_entry);
>+ suppressions_type suppr
>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>+ return !suppr.empty()
>+ && suppressions_are_consistent(suppr,
>+ "^test_symbol$|^test_another_symbol$");
Hi again,
This test case needs adjusting for the new test to pass (I added sorting
of the regex elements in a final change). It needs to read:
"^test_another_symbol$|^test_symbol$");
>+}
>+
>+bool
>+testTwoWhitelistsWithDuplicates()
>+{
>+ std::vector<std::string> abi_whitelist_paths;
>+ abi_whitelist_paths.push_back(whitelist_with_duplicate_entry);
>+ abi_whitelist_paths.push_back(whitelist_with_another_single_entry);
>+ suppressions_type suppr
>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>+ return !suppr.empty()
>+ && suppressions_are_consistent(suppr,
>+ "^test_symbol$|^test_another_symbol$");
Same here.
I am not sending a v2 for this.
Cheers,
Matthias
>+}
>+
>+bool
>+testWhitelistWithTwoSections()
>+{
>+ std::vector<std::string> abi_whitelist_paths;
>+ abi_whitelist_paths.push_back(whitelist_with_two_sections);
>+ suppressions_type suppr
>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>+ return !suppr.empty()
>+ && suppressions_are_consistent(suppr,
>+ "^test_symbol1$|^test_symbol2$");
>+}
>+
>+int
>+main(int, char*[])
>+{
>+ bool is_ok = true;
>+
>+ is_ok = is_ok && testNoWhitelist();
>+ is_ok = is_ok && testSingleEntryWhitelist();
>+ is_ok = is_ok && testWhitelistWithDuplicateEntries();
>+ is_ok = is_ok && testTwoWhitelists();
>+ is_ok = is_ok && testTwoWhitelistsWithDuplicates();
>+ is_ok = is_ok && testWhitelistWithTwoSections();
>+
>+ return !is_ok;
>+}
>--
>2.25.0.rc1.283.g88dfdc4193-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] KMI Whitelists: Add functionality to make whitelists additive
2020-01-01 0:00 [PATCH 1/2] KMI Whitelists: Add functionality to make whitelists additive Matthias Maennich via libabigail
2020-01-01 0:00 ` Matthias Maennich via libabigail
@ 2020-01-01 0:00 ` Dodji Seketeli
2020-01-01 0:00 ` [PATCH 2/2] KMI Whitelists: Drop old whitelist extraction methods Matthias Maennich via libabigail
2 siblings, 0 replies; 6+ messages in thread
From: Dodji Seketeli @ 2020-01-01 0:00 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, kernel-team
Matthias Maennich <maennich@google.com> a écrit:
[...]
> If multiple KMI whitelists are specified, either by passing
> --kmi-whitelist several times or by having multiple whitelist sections
> in the whitelist files, the generated suppressions are created as an
> intersection of symbols. That is rather unusual, as whitelisting should
> rather work additive. That means that the symbols (or expressions
> thereof) defined across several sections or files shall be considered a
> union of symbols. This patch combines the whitelist parsing to create
> exactly one function_suppression and one variable suppression. A test
> case has been added to ensure the functionality is working.
The idea of the patch is great IMHO thank you for working on it.
I just have a few nits to raise, mostly stylistic, here and there.
[...]
> diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
> index a153af689740..ea44e4dd36aa 100644
> --- a/include/abg-tools-utils.h
> +++ b/include/abg-tools-utils.h
> @@ -90,6 +90,10 @@ bool
> gen_suppr_spec_from_kernel_abi_whitelist(const string& abi_whitelist_path,
> suppr::suppressions_type& s);
>
> +suppr::suppressions_type
> +gen_suppr_spec_from_kernel_abi_whitelists(
> + const std::vector<string>& abi_whitelist_paths);
> +
When breaking a "long" line that contains a fonction declaration, please
do it like this:
suppr::suppressions_type
gen_suppr_spec_from_kernel_abi_whitelists
(const std::vector<string>& abi_whitelist_paths);
or like this:
suppr::suppressions_type
gen_suppr_spec_from_kernel_abi_whitelists(int foo,
char bar);
This is for the sake of consistency with the existing code base,
because in the grand scheme of things, there is nothing wrong with the
way you wrote it.
[...]
> index 0495905253ed..dcc302546beb 100644
> --- a/src/abg-tools-utils.cc
> +++ b/src/abg-tools-utils.cc
[...]
> @@ -38,8 +38,10 @@
> #include <sys/time.h>
> #include <dirent.h>
> #include <time.h>
> +#include <algorithm>
> #include <cstdlib>
> #include <cstring>
> +#include <iterator>
> #include <ctype.h>
> #include <errno.h>
> #include <libgen.h>
> @@ -1870,6 +1872,117 @@ gen_suppr_spec_from_headers(const string& headers_root_dir)
> return result;
> }
>
> +/// Generate a suppression specification from kernel abi whitelist
> +/// files.
> +///
> +/// A kernel ABI whitelist file is an INI file that usually has only
> +/// one section. The name of the section is a string that ends up
> +/// with the sub-string "whitelist". For instance
> +/// RHEL7_x86_64_whitelist.
> +///
> +/// Then the content of the section is a set of function or variable
> +/// names, one name per line. Each function or variable name is the
> +/// name of a function or a variable whose changes are to be keept.
> +///
> +/// A whitelist file can have multiple sections (adhering to the naming
> +/// conventions and multiple files can be passed. The suppression that
> +/// is created takes all whitelist sections from all files into account.
> +/// Symbols (or expression of such) are deduplicated in the final
> +/// suppression expression.
> +///
> +/// This function reads the white lists and generates a
> +/// function_suppression_sptr and variable_suppression_sptr and returns
> +/// a vector containing those.
> +///
> +/// @param abi_whitelist_paths a vector of KMI whitelist paths
> +///
> +/// @return a vector or suppressions
> +suppressions_type
> +gen_suppr_spec_from_kernel_abi_whitelists(
> + const std::vector<std::string>& abi_whitelist_paths)
Likewise for breaking the long line:
gen_suppr_spec_from_kernel_abi_whitelists
(const std::vector<std::string>& abi_whitelist_paths)
> +{
> +
> + std::vector<std::string> whitelisted_names;
> + for (std::vector<std::string>::const_iterator path_iter
> + = abi_whitelist_paths.begin(),
The style used in the rest of the codebase for assignation statement is
to keep the '=' on the same line as the left-hand-side operand; i.e:
> + for (std::vector<std::string>::const_iterator path_iter > =
abi_whitelist_paths.begin(),
Also, when long lines are involved in the 'for' statement like that,
the convention used in the code base is to have the init, condition
and increment statements of the for loop start on their own line. So
that for loop would look like:
for (std::vector<std::string>::const_iterator path_iter =
abi_whitelist_paths.begin(),
path_end = abi_whitelist_paths.end();
path_iter != path_end;
++path_iter)
{
[...]
> +
> + abigail::ini::config whitelist;
> + if (!read_config(*path_iter, whitelist))
> + continue;
> +
> + const ini::config::sections_type& whitelist_sections
> + = whitelist.get_sections();
Likewise, please keep the '=' on the same line as the left-hand-side
operand.
> +
> + for (ini::config::sections_type::const_iterator section_iter
> + = whitelist_sections.begin(),
> + section_end = whitelist_sections.end();
> + section_iter != section_end; ++section_iter)
Likewise, loop style.
> + {
> + std::string section_name = (*section_iter)->get_name();
> + if (!string_ends_with(section_name, "whitelist"))
> + continue;
> + for (ini::config::properties_type::const_iterator prop_iter
> + = (*section_iter)->get_properties().begin(),
> + prop_end = (*section_iter)->get_properties().end();
> + prop_iter != prop_end; ++prop_iter)
Likewise, loop style.
> + {
> + if (const simple_property_sptr& prop
> + = is_simple_property(*prop_iter))
Likewise, equality.
> + if (prop->has_empty_value())
> + {
> + const std::string& name = prop->get_name();
> + if (!name.empty())
> + whitelisted_names.push_back(name);
> + }
> + }
> + }
> + }
> +
> + suppressions_type result;
> + if (!whitelisted_names.empty())
> + {
> + // Drop duplicates
> + whitelisted_names.erase(
> + std::unique(whitelisted_names.begin(), whitelisted_names.end()),
> + whitelisted_names.end());
The style of breaking the function call accross several lines would
be:
whitelisted_names.erase(std::unique(whitelisted_names.begin(),
whitelisted_names.end()),
whitelisted_names.end());
> +
> + // Build the regular expression to suppress non whitelisted symbols.
> + std::stringstream regex_ss;
I think, a more accurate way to put this would be to say something
like:
// Build a regular expression representing the union of all
// the function and variable names expressed in the white list.
> + regex_ss << "^";
> + std::copy(whitelisted_names.begin(), whitelisted_names.end(),
> + std::ostream_iterator<std::string>(regex_ss, "$|^"));
> + regex_ss.seekp(0, std::ios::end);
> + const std::string& regex = regex_ss.str().substr(
> + 0, static_cast<size_t>(regex_ss.tellp()) - 2);
Same line breaking style around function call expression.
[...]
>>+bool
>>+testTwoWhitelistsWithDuplicates()
>>+{
>>+ std::vector<std::string> abi_whitelist_paths;
>>+ abi_whitelist_paths.push_back(whitelist_with_duplicate_entry);
>>+ abi_whitelist_paths.push_back(whitelist_with_another_single_entry);
>>+ suppressions_type suppr
>>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>>+ return !suppr.empty()
>>+ && suppressions_are_consistent(suppr,
>>+ "^test_symbol$|^test_another_symbol$");
>
> Same here.
>
> I am not sending a v2 for this.
OK.
This is OK to commit to master with the comments above addressed.
Thanks a lot for working on this!
--
Dodji
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/2] KMI Whitelists: Drop old whitelist extraction methods
2020-01-01 0:00 [PATCH 1/2] KMI Whitelists: Add functionality to make whitelists additive Matthias Maennich via libabigail
2020-01-01 0:00 ` Matthias Maennich via libabigail
2020-01-01 0:00 ` Dodji Seketeli
@ 2020-01-01 0:00 ` Matthias Maennich via libabigail
2020-01-01 0:00 ` Dodji Seketeli
2 siblings, 1 reply; 6+ messages in thread
From: Matthias Maennich via libabigail @ 2020-01-01 0:00 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, maennich
The previous commit introduces a new (tested) way of creating function
and variable suppressions from multiple whitelist definitions. Migrate
to this new way of processing KMI whitelists.
* include/abg-tools-utils.h
(gen_suppr_spec_from_kernel_abi_whitelist): Delete declaration.
* src/abg-tools-utils.cc
(gen_suppr_spec_from_kernel_abi_whitelist): Delete definition
and migrate users to gen_suppr_spec_from_kernel_abi_whitelists.
* tools/abidiff.cc (set_suppressions): Migrate from using
gen_suppr_spec_from_kernel_abi_whitelist to
gen_suppr_spec_from_kernel_abi_whitelists.
* tools/abidw.cc (set_suppressions): Likewise.
* tools/abipkgdiff.cc: Drop unused using definition.
* tools/kmidiff.cc: Likewise.
Signed-off-by: Matthias Maennich <maennich@google.com>
---
include/abg-tools-utils.h | 4 --
src/abg-tools-utils.cc | 117 ++------------------------------------
tools/abidiff.cc | 12 ++--
tools/abidw.cc | 12 ++--
tools/abipkgdiff.cc | 1 -
tools/kmidiff.cc | 1 -
6 files changed, 18 insertions(+), 129 deletions(-)
diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
index ea44e4dd36aa..240e36b2f363 100644
--- a/include/abg-tools-utils.h
+++ b/include/abg-tools-utils.h
@@ -86,10 +86,6 @@ void convert_char_stars_to_char_star_stars(const vector<char*>&,
suppr::type_suppression_sptr
gen_suppr_spec_from_headers(const string& hdrs_root_dir);
-bool
-gen_suppr_spec_from_kernel_abi_whitelist(const string& abi_whitelist_path,
- suppr::suppressions_type& s);
-
suppr::suppressions_type
gen_suppr_spec_from_kernel_abi_whitelists(
const std::vector<string>& abi_whitelist_paths);
diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index dcc302546beb..e833f91a99a8 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -1944,7 +1944,8 @@ gen_suppr_spec_from_kernel_abi_whitelists(
suppressions_type result;
if (!whitelisted_names.empty())
{
- // Drop duplicates
+ // Drop duplicates to simplify the regex we are generating
+ std::sort(whitelisted_names.begin(), whitelisted_names.end());
whitelisted_names.erase(
std::unique(whitelisted_names.begin(), whitelisted_names.end()),
whitelisted_names.end());
@@ -1983,111 +1984,6 @@ gen_suppr_spec_from_kernel_abi_whitelists(
return result;
}
-/// Generate a suppression specification from kernel abi whitelist
-/// files.
-///
-/// A kernel ABI whitelist file is an INI file that usually has only
-/// one section. The name of the section is a string that ends up
-/// with the sub-string "whitelist". For instance
-/// RHEL7_x86_64_whitelist.
-///
-/// Then the content of the section is a set of function or variable
-/// names, one name per line. Each function or variable name is the
-/// name of a function or a variable whose changes are to be keept.
-///
-/// This function reads the white list and generates a
-/// function_suppression_sptr or variable_suppression_sptr (or
-/// several, if there are more than one section) that is added to a
-/// vector of suppressions.
-///
-/// @param abi_whitelist_path the path to the Kernel ABI whitelist.
-///
-/// @param supprs the resulting vector of suppressions to which the
-/// new function suppressions resulting from reading the whitelist are
-/// added. This vector is updated iff the function returns true.
-///
-/// @return true iff the abi whitelist file was read and function
-/// suppressions could be generated as a result.
-bool
-gen_suppr_spec_from_kernel_abi_whitelist(const string& abi_whitelist_path,
- suppressions_type& supprs)
-{
- abigail::ini::config whitelist;
- if (!read_config(abi_whitelist_path, whitelist))
- return false;
-
- bool created_a_suppr = false;
-
- const ini::config::sections_type &whitelist_sections =
- whitelist.get_sections();
- for (ini::config::sections_type::const_iterator s =
- whitelist_sections.begin();
- s != whitelist_sections.end();
- ++s)
- {
- string section_name = (*s)->get_name();
- if (!string_ends_with(section_name, "whitelist"))
- continue;
-
- function_suppression_sptr fn_suppr;
- variable_suppression_sptr var_suppr;
- string function_names_regexp, variable_names_regexp;
- for (ini::config::properties_type::const_iterator p =
- (*s)->get_properties().begin();
- p != (*s)->get_properties().end();
- ++p)
- {
- if (simple_property_sptr prop = is_simple_property(*p))
- if (prop->has_empty_value())
- {
- const string &name = prop->get_name();
- if (!name.empty())
- {
- if (!function_names_regexp.empty())
- function_names_regexp += "|";
- function_names_regexp += "^" + name + "$";
-
- if (!variable_names_regexp.empty())
- variable_names_regexp += "|";
- variable_names_regexp += "^" + name + "$";
- }
- }
- }
-
- if (!function_names_regexp.empty())
- {
- // Build a suppression specification which *keeps* functions
- // whose ELF symbols match the regular expression contained
- // in function_names_regexp. This will also keep the ELF
- // symbols (not designated by any debug info) whose names
- // match this regexp.
- fn_suppr.reset(new function_suppression);
- fn_suppr->set_label(section_name);
- fn_suppr->set_symbol_name_not_regex_str(function_names_regexp);
- fn_suppr->set_drops_artifact_from_ir(true);
- supprs.push_back(fn_suppr);
- created_a_suppr = true;
- }
-
- if (!variable_names_regexp.empty())
- {
- // Build a suppression specification which *keeps* variables
- // whose ELF symbols match the regular expression contained
- // in function_names_regexp. This will also keep the ELF
- // symbols (not designated by any debug info) whose names
- // match this regexp.
- var_suppr.reset(new variable_suppression);
- var_suppr->set_label(section_name);
- var_suppr->set_symbol_name_not_regex_str(variable_names_regexp);
- var_suppr->set_drops_artifact_from_ir(true);
- supprs.push_back(var_suppr);
- created_a_suppr = true;
- }
- }
-
- return created_a_suppr;
-}
-
/// Get the path to the default system suppression file.
///
/// @return a copy of the default system suppression file.
@@ -2262,11 +2158,10 @@ load_generate_apply_suppressions(dwarf_reader::read_context &read_ctxt,
++i)
read_suppressions(*i, supprs);
- for (vector<string>::const_iterator i =
- kabi_whitelist_paths.begin();
- i != kabi_whitelist_paths.end();
- ++i)
- gen_suppr_spec_from_kernel_abi_whitelist(*i, supprs);
+ const suppressions_type& wl_suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(kabi_whitelist_paths);
+
+ supprs.insert(supprs.end(), wl_suppr.begin(), wl_suppr.end());
}
abigail::dwarf_reader::add_read_context_suppressions(read_ctxt, supprs);
diff --git a/tools/abidiff.cc b/tools/abidiff.cc
index fda8d2f98565..7ca427446911 100644
--- a/tools/abidiff.cc
+++ b/tools/abidiff.cc
@@ -59,7 +59,7 @@ using abigail::tools_utils::emit_prefix;
using abigail::tools_utils::check_file;
using abigail::tools_utils::guess_file_type;
using abigail::tools_utils::gen_suppr_spec_from_headers;
-using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelist;
+using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelists;
using abigail::tools_utils::load_default_system_suppressions;
using abigail::tools_utils::load_default_user_suppressions;
using abigail::tools_utils::abidiff_status;
@@ -760,11 +760,11 @@ set_suppressions(ReadContextType& read_ctxt, const options& opts)
}
}
- for (vector<string>::const_iterator i =
- opts.kernel_abi_whitelist_paths.begin();
- i != opts.kernel_abi_whitelist_paths.end();
- ++i)
- gen_suppr_spec_from_kernel_abi_whitelist(*i, supprs);
+ const suppressions_type& wl_suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(
+ opts.kernel_abi_whitelist_paths);
+
+ supprs.insert(supprs.end(), wl_suppr.begin(), wl_suppr.end());
add_read_context_suppressions(read_ctxt, supprs);
}
diff --git a/tools/abidw.cc b/tools/abidw.cc
index d520fc3d03a5..19ae9b0d2622 100644
--- a/tools/abidw.cc
+++ b/tools/abidw.cc
@@ -375,12 +375,12 @@ set_suppressions(read_context& read_ctxt, options& opts)
if (suppr)
supprs.push_back(suppr);
- using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelist;
- for (vector<string>::const_iterator i =
- opts.kabi_whitelist_paths.begin();
- i != opts.kabi_whitelist_paths.end();
- ++i)
- gen_suppr_spec_from_kernel_abi_whitelist(*i, opts.kabi_whitelist_supprs);
+ using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelists;
+ const suppressions_type& wl_suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(opts.kabi_whitelist_paths);
+
+ opts.kabi_whitelist_supprs.insert(opts.kabi_whitelist_supprs.end(),
+ wl_suppr.begin(), wl_suppr.end());
add_read_context_suppressions(read_ctxt, supprs);
add_read_context_suppressions(read_ctxt, opts.kabi_whitelist_supprs);
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index f3065fdf05ac..e8352115ccc0 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -136,7 +136,6 @@ using abigail::tools_utils::base_name;
using abigail::tools_utils::get_rpm_arch;
using abigail::tools_utils::file_is_kernel_package;
using abigail::tools_utils::gen_suppr_spec_from_headers;
-using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelist;
using abigail::tools_utils::get_default_system_suppression_file_path;
using abigail::tools_utils::get_default_user_suppression_file_path;
using abigail::tools_utils::get_vmlinux_path_from_kernel_dist;
diff --git a/tools/kmidiff.cc b/tools/kmidiff.cc
index 9a1645284a01..86cfb3de9827 100644
--- a/tools/kmidiff.cc
+++ b/tools/kmidiff.cc
@@ -60,7 +60,6 @@ using abigail::comparison::get_default_harmful_categories_bitmap;
using abigail::suppr::suppression_sptr;
using abigail::suppr::suppressions_type;
using abigail::suppr::read_suppressions;
-using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelist;
using abigail::tools_utils::guess_file_type;
using abigail::tools_utils::file_type;
using abigail::xml_reader::read_corpus_group_from_native_xml_file;
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] KMI Whitelists: Drop old whitelist extraction methods
2020-01-01 0:00 ` [PATCH 2/2] KMI Whitelists: Drop old whitelist extraction methods Matthias Maennich via libabigail
@ 2020-01-01 0:00 ` Dodji Seketeli
2020-01-01 0:00 ` Matthias Maennich via libabigail
0 siblings, 1 reply; 6+ messages in thread
From: Dodji Seketeli @ 2020-01-01 0:00 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, kernel-team
Matthias Maennich <maennich@google.com> a écrit:
> The previous commit introduces a new (tested) way of creating function
> and variable suppressions from multiple whitelist definitions. Migrate
> to this new way of processing KMI whitelists.
>
> * include/abg-tools-utils.h
> (gen_suppr_spec_from_kernel_abi_whitelist): Delete declaration.
> * src/abg-tools-utils.cc
> (gen_suppr_spec_from_kernel_abi_whitelist): Delete definition
> and migrate users to gen_suppr_spec_from_kernel_abi_whitelists.
> * tools/abidiff.cc (set_suppressions): Migrate from using
> gen_suppr_spec_from_kernel_abi_whitelist to
> gen_suppr_spec_from_kernel_abi_whitelists.
> * tools/abidw.cc (set_suppressions): Likewise.
> * tools/abipkgdiff.cc: Drop unused using definition.
> * tools/kmidiff.cc: Likewise.
This is OK to commit to master once the first patch of the series is in.
Thank you for working on this!
Cheers,
--
Dodji
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KMI Whitelists: Drop old whitelist extraction methods
2020-01-01 0:00 ` Dodji Seketeli
@ 2020-01-01 0:00 ` Matthias Maennich via libabigail
0 siblings, 0 replies; 6+ messages in thread
From: Matthias Maennich via libabigail @ 2020-01-01 0:00 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail, kernel-team
On Tue, Jan 21, 2020 at 05:24:11PM +0100, Dodji Seketeli wrote:
>Matthias Maennich <maennich@google.com> a écrit:
>
>> The previous commit introduces a new (tested) way of creating function
>> and variable suppressions from multiple whitelist definitions. Migrate
>> to this new way of processing KMI whitelists.
>>
>> * include/abg-tools-utils.h
>> (gen_suppr_spec_from_kernel_abi_whitelist): Delete declaration.
>> * src/abg-tools-utils.cc
>> (gen_suppr_spec_from_kernel_abi_whitelist): Delete definition
>> and migrate users to gen_suppr_spec_from_kernel_abi_whitelists.
>> * tools/abidiff.cc (set_suppressions): Migrate from using
>> gen_suppr_spec_from_kernel_abi_whitelist to
>> gen_suppr_spec_from_kernel_abi_whitelists.
>> * tools/abidw.cc (set_suppressions): Likewise.
>> * tools/abipkgdiff.cc: Drop unused using definition.
>> * tools/kmidiff.cc: Likewise.
>
>This is OK to commit to master once the first patch of the series is in.
Hi Dodji!
Thanks for the review!
I applied the patches after correcting them both(!) according to your
comments.
Cheers,
Matthias
>
>Thank you for working on this!
>
>Cheers,
>
>--
> Dodji
^ permalink raw reply [flat|nested] 6+ messages in thread