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

[Dodji Seketeli]

Ondrej Oprala <ooprala@redhat.com> a écrit:

> diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc

[...]

> @@ -82,16 +83,21 @@ static bool verbose;
>  /// This contains the set of files of a given package.  It's populated
>  /// by a worker function that is invoked on each file contained in the
>  /// package.  So this global variable is filled by the
> -/// file_tree_walker_callback_fn() function.  Its content is relevant
> -/// only during the time after which the current package has been
> +/// file_tree_walker_callback_{elf,suppr}_fn() functions.  Its content is
> +/// relevant only during the time after which the current package has been

This change should now be adjusted to the newer names of the callback
functions.

[...]

> @@ -108,13 +114,15 @@ struct options
>    options()
>      : display_usage(),
>        missing_operand(),
> +      abignore(true),
>        keep_tmp_files(),
>        compare_dso_only(),
>        show_linkage_names(true),
>        show_redundant_changes(),
>        show_added_syms(true),
>        show_added_binaries(true),
> -      fail_if_no_debug_info()
> +      fail_if_no_debug_info(),
> +      suppression_paths()

default-initializing the suppression_paths member here is not necessary
because the default constructor of the vector type is going to be
invoked by the default constructor of the option type.  Or am I missing
something?

>    {}
>  };
>  
> @@ -146,6 +154,10 @@ public:
>  /// A convenience typedef for a shared pointer to elf_file.
>  typedef shared_ptr<elf_file> elf_file_sptr;
>  
> +/// A convenience typedef for a pointer to a function type that
> +/// the ftw() function accepts.
> +typedef int (*ftw_cb_type)(const char *, const struct stat*, int);
> +
>  /// Abstract the result of comparing two packages.
>  ///
>  /// This contains the the paths of the set of added binaries, removed
> @@ -653,9 +665,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*/)
> +first_package_tree_walker_callback_fn(const char *fpath,
> +				      const struct stat *,
> +				      int /*flag*/)

Please update the comment of this function to say that this callback is
invoked when walking the *first* package.

[...]

>  {
>    struct stat s;
>    lstat(fpath, &s);
> @@ -668,6 +680,30 @@ file_tree_walker_callback_fn(const char *fpath,
>    return 0;
>  }
>  
> +/// A callback function invoked by the ftw() function while walking
> +/// the directory of files extracted from the second package, unless
> +/// "--no-abignore" is specified on the command line.
> +///
> +/// @param fpath the path to the file being considered.
> +///
> +/// @param stat the stat struct of the file.
> +static int
> +second_package_tree_walker_callback_fn(const char *fpath,
> +				       const struct stat *,
> +				       int /*flag*/)
> +{
> +  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 (prog_options->abignore && string_ends_with(fpath, ".abignore"))
> +	prog_options->suppression_paths.push_back(fpath);
> +    }
> +  return 0;
> +}
>  /// Update the diff context from the @ref options data structure.
>  ///
>  /// @param ctxt the diff context to update.
> @@ -858,9 +894,11 @@ compare(const elf_file& elf1,
>  /// @param opts the options the current program has been called with.
>  ///
>  /// @param true upon successful completion, false otherwise.
> +

This is an unnecessary white space change.

>  static bool
>  create_maps_of_package_content(package& package,
> -			       const options& opts)
> +			       const options& opts,
> +			       ftw_cb_type callback)
>  {
>    elf_file_paths.clear();
>    if (verbose)
> @@ -870,9 +908,7 @@ create_maps_of_package_content(package& package,
>  	 << package.extracted_dir_path()
>  	 << " ...";
>  
> -  if (ftw(package.extracted_dir_path().c_str(),
> -	  file_tree_walker_callback_fn,
> -	  16))
> +  if (ftw(package.extracted_dir_path().c_str(), callback, 16))
>      {
>        cerr << "Error while inspecting files in package"
>  	   << package.extracted_dir_path() << "\n";
> @@ -919,14 +955,15 @@ create_maps_of_package_content(package& package,
>  /// @return true upon successful completion, false otherwise.
>  static bool
>  extract_package_and_map_its_content(package& package,
> -				    const options& opts)
> +				    const options& opts,
> +				    ftw_cb_type callback)
>  {
>    if (!extract_package(package))
>      return false;
>  
>    bool result = true;
>    if (!package.is_debug_info())
> -    result |= create_maps_of_package_content(package, opts);
> +    result |= create_maps_of_package_content(package, opts, callback);
>  
>    return result;
>  }
> @@ -947,8 +984,12 @@ 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,
> +					   first_package_tree_walker_callback_fn)
> +      /// We go through the files of the newer (second) pkg to look for
> +      /// suppression specifications, matching the "*.abignore" name pattern.

I think this comment should rather be inside the function
second_package_tree_walker_callback_fn.  Because at this level, the
second_package_tree_walker_callback_fn can do more (and does more) than
looking for suppression specifications.

Oh, and thank you very much for the test great quantity of new tests!
That is really cool (and useful, haha) :-)

So this patch is OK to commit to master with the changes above, if the
result still passes "make distcheck", of course.

Cheers!

-- 
		Dodji