[PATCH 1/2] KMI Whitelists: Add functionality to make whitelists additive

[Dodji Seketeli]

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