[Bug default/19082] New: Make abipkgdiff recognize suppression specifications to apply to comparisons

[Ondrej Oprala]

Hey, thanks for the review, I've addressed all your concerns
and distcheck (still) passes.

I'm attaching the redone patch.

Thanks,
   Ondrej

On 12.10.2015 16:02, Dodji Seketeli wrote:
> Hi!
>
> Thank you for the nice patch, it's really appreciated!
>
> Please see my comments below.
>
>> --- a/tools/abipkgdiff.cc
> [...]
>
>> +static vector<string> suppression_paths;
> Rather than putting this here, I'd rather declare a (global) variable of
> type (called, e.g, prog_options) pointer to the options type, right
> before the main() and initialize that pointer in main(), right after
> we've parsed the command line options.  That pointer to the options
> would then be globally accessible to the walker functions, if need be.
>
> I believe that could have some interesting consequences, which I talk
> about below.
>
> [...]
>
>>   /// Abstract the result of comparing two packages.
>>   ///
>>   /// This contains the the paths of the set of added binaries, removed
>> @@ -653,9 +660,9 @@ extract_package(const package& package)
>>   ///
>>   /// @param stat the stat struct of the file.
>>   static int
>> -file_tree_walker_callback_fn(const char *fpath,
>> -			     const struct stat *,
>> -			     int /*flag*/)
>> +file_tree_walker_elf_callback_fn(const char *fpath,
>> +				 const struct stat *,
>> +				 int /*flag*/)
> [...]
>
>> +static int
>> +file_tree_walker_suppr_callback_fn(const char *fpath,
>> +				   const struct stat *,
>> +				   int /*flag*/)
>> +{
> I would call the two functions above first_package_tree_walker_callback_fn and
> second_package_tree_walker_callback_fn, because I think that really is
> the intent of having two walker functions.  One is to walk the content
> of the first package, and the other is to walk the content of the second
> package.
>
> In the second walking function:
>
>> +  struct stat s;
>> +  lstat(fpath, &s);
>> +
>> +  if (!S_ISLNK(s.st_mode))
>> +    {
>> +      if (guess_file_type(fpath) == abigail::tools_utils::FILE_TYPE_ELF)
>> +	elf_file_paths.push_back(fpath);
>> +      else if (string_ends_with(fpath, ".abignore"))
>> +	suppression_paths.push_back(fpath);
> ... I would conditionalize the action of recognizing the suppression
> specification here to prog_options->abignore
>
> So that it would read something like:
>
>      if (prog_options->abignore
>          && string_ends_with(fpath, ".abignore"))
>        prog_options->suppression_paths.push_back(fpath);
>
>> +    }
>> +  return 0;
>> +}
> [...]
>
> @@ -696,8 +727,8 @@ set_diff_context_from_opts(diff_context_sptr ctxt,
>        | abigail::comparison::HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY);
>   
>>     suppressions_type supprs;
>> -  for (vector<string>::const_iterator i = opts.suppression_paths.begin();
>> -       i != opts.suppression_paths.end();
>> +  for (vector<string>::const_iterator i = suppression_paths.begin();
>> +       i != suppression_paths.end();
>>          ++i)
> This change would then become unnecessary.
>
> [...]
>   
>> @@ -947,8 +979,14 @@ prepare_packages(package&	first_package,
>>   		 package&	second_package,
>>   		 const options& opts)
>>   {
>> -  if (!extract_package_and_map_its_content(first_package, opts)
>> -      || !extract_package_and_map_its_content(second_package, opts))
>> +  if (!extract_package_and_map_its_content(first_package, opts,
>> +					   file_tree_walker_elf_callback_fn)
>> +  /// We go through the files of the newer (second) pkg to look for suppression
>> +  /// specifications, matching the "*.abignore" name pattern.
>> +      || !extract_package_and_map_its_content(second_package, opts,
>> +					      opts.abignore ?
>> +					      file_tree_walker_suppr_callback_fn
>> +					      : file_tree_walker_elf_callback_fn))
> Here, the change would then look like:
>
>> +  if (!extract_package_and_map_its_content(first_package, opts,
>> +					   first_package_tree_walker_callback_fn)
>> +      || !extract_package_and_map_its_content(second_package, opts,
>> +					      second_package_tree_walker_callback_fn))
> which I think is simpler because there is no condition any more.  The
> condition happens at a lower logical level, where we really need to act
> depending on that condition: take the suppression specification into
> account, if the user asked for it.
>
> [...]
>
>> @@ -1142,13 +1180,15 @@ parse_command_line(int argc, char* argv[], options& opts)
>>   	opts.fail_if_no_debug_info = true;
>>         else if (!strcmp(argv[i], "--verbose"))
>>   	verbose = true;
>> +      else if (!strcmp(argv[i], "--no-abignore"))
>> +	opts.abignore = false;
>>         else if (!strcmp(argv[i], "--suppressions")
>>   	       || !strcmp(argv[i], "--suppr"))
>>   	{
>>   	  int j = i + 1;
>>   	  if (j >= argc)
>>   	    return false;
>> -	  opts.suppression_paths.push_back(argv[j]);
>> +	  suppression_paths.push_back(argv[j]);
> This change would then be unnecessary.
>
> What do you think?
>
> Cheers,
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Bug-19082-Recognize-suppression-spec-files.patch
Type: text/x-patch
Size: 51436 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libabigail/attachments/20150101/5e9176de/attachment.bin>