Re: [PATCH, RFC] abipkgdiff: Fix kernel package detection when comparing

["Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>]

On Mon, Feb 27, 2023 at 07:57:42PM +0100, Dodji Seketeli wrote:
> Hello Guillermo,
> 

Hello Dodji,

> Here is the amended patch that I am proposing to address the issue you
> reported.
> 
> I try to fix the problem of tools_utils::file_is_kernel_package first,
> then I added your CTF-specific fixes on top.
> 

Nice.

> Can you please tell me what you think about this approach?
> 

I'm agree with your approach!.
> Thanks.
> 

Thanks to you,
guillermo

> 
> Using abipkgdiff to analyze kABIs from Oracle Linux packages with CTF
> debug format, abipkgdiff is not able to identify kernel packages
> because the naming of OL kernel packages differs from the naming used
> on other RPM-based distributions.
> 
> As abipkgdiff fails to see that it's looking at a Linux kernel
> package, the binaries are analyzed as user space binaries and that is
> not what we want.
> 
> This patch addresses the issue by looking for the "vmlinuz" binary
> inside the package to determine that it's a kernel package.  In other
> words, tools_utils::file_is_kernel_package is changed to look for
> "vmlinuz" inside the package, rather than look for a particular
> pattern in the package name of the package.
> 
> Additionally, when the kernel package contains CTF debug information,
> the `vmlinux.ctfa' file is not necessarily shipped the debuginfo
> package.  This patch thus adjusts the search path of that file in that
> case.
> 
>         * include/abg-tools-utils.h (rpm_contains_file): Declare new
>         function.
>         * src/abg-ctf-reader.cc (ctf::reader::find_ctfa_file): Use
>         `find_file_under_dir' utility function to locate `vmlinux.ctfa'
>         file.
>         (ctf::reader::process_ctf_archive): Adjust dictionary name
>         according to module name, removing characters after dot.
>         * src/abg-tools-utils.cc (file_has_ctf_debug_info): Use
>         `find_file_under_dir' utility function to locate `vmlinux.ctfa'
>         file.
>         (rpm_contains_file): Define new function.
>         (file_is_kernel_package): Use the new `rpm_contains_file' to look
> 	for the `vmlinuz' file inside the RPM package.  For Debian
> 	packages however, we don't keep looking at the naming pattern as
> 	we don't yet have a deb_contains_file function.  Also, this
> 	function now takes the full path to the RPM.
>         (build_corpus_group_from_kernel_dist_under): for CTF, add the
>         `root' directory of the extracted package to the set of
>         directories under which we should look for debug info.
> 	* tools/abipkgdiff.cc (maybe_handle_kabi_whitelist_pkg)
> 	(create_maps_of_package_content, compare_prepared_package, main):
> 	Adjust call to file_is_kernel_package as it now takes the full
> 	path to the package.
> 
> Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> ---
>  include/abg-tools-utils.h |  4 +++
>  src/abg-ctf-reader.cc     | 16 ++++-------
>  src/abg-tools-utils.cc    | 60 ++++++++++++++++++++++++++++++++-------
>  tools/abipkgdiff.cc       | 19 ++++++-------
>  4 files changed, 69 insertions(+), 30 deletions(-)
> 
> diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
> index 70745b0f..43bf6a3e 100644
> --- a/include/abg-tools-utils.h
> +++ b/include/abg-tools-utils.h
> @@ -300,6 +300,10 @@ bool
>  file_is_kernel_package(const string& file_path,
>  		       file_type file_type);
>  
> +bool
> +rpm_contains_file(const string& rpm_path,
> +		  const string& file_name);
> +
>  bool
>  file_is_kernel_debuginfo_package(const string& file_path,
>  				 file_type file_type);
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 54257d11..7db8dbc6 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> @@ -336,12 +336,8 @@ public:
>      // for vmlinux.ctfa should be provided with --debug-info-dir
>      // option.
>      for (const auto& path : debug_info_root_paths())
> -      {
> -	ctfa_dirname = *path;
> -	ctfa_file = ctfa_dirname + "/vmlinux.ctfa";
> -	if (file_exists(ctfa_file))
> -	  return true;
> -      }
> +      if (tools_utils::find_file_under_dir(*path, "vmlinux.ctfa", ctfa_file))
> +        return true;
>  
>      return false;
>    }
> @@ -428,10 +424,10 @@ public:
>  	&& corpus_group())
>        {
>  	tools_utils::base_name(corpus_path(), dict_name);
> -
> -	if (dict_name != "vmlinux")
> -	  // remove .ko suffix
> -	  dict_name.erase(dict_name.length() - 3, 3);
> +	// remove .* suffix
> +	std::size_t pos = dict_name.find(".");
> +	if (pos != string::npos)
> +	  dict_name.erase(pos);
>  
>  	std::replace(dict_name.begin(), dict_name.end(), '-', '_');
>        }
> diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
> index 81f9aa75..85719cdc 100644
> --- a/src/abg-tools-utils.cc
> +++ b/src/abg-tools-utils.cc
> @@ -501,7 +501,7 @@ file_has_ctf_debug_info(const string& elf_file_path,
>  
>    // vmlinux.ctfa could be provided with --debug-info-dir
>    for (const auto& path : debug_info_root_paths)
> -    if (dir_contains_ctf_archive(*path, vmlinux))
> +    if (find_file_under_dir(*path, "vmlinux.ctfa", vmlinux))
>        return true;
>  
>    return false;
> @@ -1764,34 +1764,64 @@ get_rpm_arch(const string& str, string& arch)
>  
>  /// Tests if a given file name designates a kernel package.
>  ///
> -/// @param file_name the file name to consider.
> +/// @param file_path the path to the file to consider.
>  ///
>  /// @param file_type the type of the file @p file_name.
>  ///
>  /// @return true iff @p file_name of kind @p file_type designates a
>  /// kernel package.
>  bool
> -file_is_kernel_package(const string& file_name, file_type file_type)
> +file_is_kernel_package(const string& file_path, file_type file_type)
>  {
>    bool result = false;
> -  string package_name;
>  
>    if (file_type == FILE_TYPE_RPM)
>      {
> -      if (!get_rpm_name(file_name, package_name))
> -	return false;
> -      result = (package_name == "kernel");
> +      if (rpm_contains_file(file_path, "vmlinuz"))
> +	result = true;
>      }
>    else if (file_type == FILE_TYPE_DEB)
>      {
> -      if (!get_deb_name(file_name, package_name))
> -	return false;
> -      result = (string_begins_with(package_name, "linux-image"));
> +      string file_name;
> +      base_name(file_path, file_name);
> +      string package_name;
> +      if (get_deb_name(file_name, package_name))
> +	result = (string_begins_with(package_name, "linux-image"));
>      }
>  
>    return result;
>  }
>  
> +/// Test if an RPM package contains a given file.
> +///
> +/// @param rpm_path the path to the RPM package.
> +///
> +/// @param file_name the file name to test the presence for in the
> +/// rpm.
> +///
> +/// @return true iff the file named @file_name is present in the RPM.
> +bool
> +rpm_contains_file(const string& rpm_path, const string& file_name)
> +{
> +    vector<string> query_output;
> +  // We don't check the return value of this command because on some
> +  // system, the command can issue errors but still emit a valid
> +  // output.  We'll rather rely on the fact that the command emits a
> +  // valid output or not.
> +  execute_command_and_get_output("rpm -qlp "
> +				 + rpm_path + " 2> /dev/null",
> +				 query_output);
> +
> +  for (auto& line : query_output)
> +    {
> +      line = trim_white_space(line);
> +      if (string_ends_with(line, file_name))
> +	return true;
> +    }
> +
> +  return false;
> +}
> +
>  /// Tests if a given file name designates a kernel debuginfo package.
>  ///
>  /// @param file_name the file name to consider.
> @@ -2812,6 +2842,16 @@ build_corpus_group_from_kernel_dist_under(const string&	root,
>        vector<char**> di_roots;
>        di_roots.push_back(&di_root_ptr);
>  
> +#ifdef WITH_CTF
> +      shared_ptr<char> di_root_ctf;
> +      if (requested_fe_kind & corpus::CTF_ORIGIN)
> +        {
> +          di_root_ctf = make_path_absolute(root.c_str());
> +          char *di_root_ctf_ptr = di_root_ctf.get();
> +          di_roots.push_back(&di_root_ctf_ptr);
> +        }
> +#endif
> +
>        abigail::elf_based_reader_sptr reader =
>          create_best_elf_based_reader(vmlinux,
>                                       di_roots,
> diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
> index a4b4f1a1..c2fc09ca 100644
> --- a/tools/abipkgdiff.cc
> +++ b/tools/abipkgdiff.cc
> @@ -2022,8 +2022,8 @@ maybe_handle_kabi_whitelist_pkg(const package& pkg, options &opts)
>    if (pkg.type() != abigail::tools_utils::FILE_TYPE_RPM)
>      return false;
>  
> -  string pkg_name = pkg.base_name();
> -  bool is_linux_kernel_package = file_is_kernel_package(pkg_name, pkg.type());
> +  bool is_linux_kernel_package = file_is_kernel_package(pkg.path(),
> +							pkg.type());
>  
>    if (!is_linux_kernel_package)
>      return false;
> @@ -2036,7 +2036,7 @@ maybe_handle_kabi_whitelist_pkg(const package& pkg, options &opts)
>      return false;
>  
>    string rpm_arch;
> -  if (!get_rpm_arch(pkg_name, rpm_arch))
> +  if (!get_rpm_arch(pkg.base_name(), rpm_arch))
>      return false;
>  
>    string kabi_wl_path = kabi_wl_pkg->extracted_dir_path();
> @@ -2412,8 +2412,7 @@ create_maps_of_package_content(package& package, options& opts)
>    // if package is linux kernel package and its associated debug
>    // info package looks like a kernel debuginfo package, then try to
>    // go find the vmlinux file in that debug info file.
> -  string pkg_name = package.base_name();
> -  bool is_linux_kernel_package = file_is_kernel_package(pkg_name,
> +  bool is_linux_kernel_package = file_is_kernel_package(package.path(),
>  							package.type());
>    if (is_linux_kernel_package)
>      {
> @@ -3219,7 +3218,7 @@ compare_prepared_package(package& first_package, package& second_package,
>  {
>    abidiff_status status = abigail::tools_utils::ABIDIFF_OK;
>  
> -  if (abigail::tools_utils::file_is_kernel_package(first_package.base_name(),
> +  if (abigail::tools_utils::file_is_kernel_package(first_package.path(),
>  						   first_package.type()))
>      {
>        opts.show_symbols_not_referenced_by_debug_info = false;
> @@ -3737,14 +3736,14 @@ main(int argc, char* argv[])
>  		  | abigail::tools_utils::ABIDIFF_ERROR);
>  	}
>  
> -      if (file_is_kernel_package(first_package->base_name(),
> +      if (file_is_kernel_package(first_package->path(),
>  				 abigail::tools_utils::FILE_TYPE_RPM)
> -	  || file_is_kernel_package(second_package->base_name(),
> +	  || file_is_kernel_package(second_package->path(),
>  				    abigail::tools_utils::FILE_TYPE_RPM))
>  	{
> -	  if (file_is_kernel_package(first_package->base_name(),
> +	  if (file_is_kernel_package(first_package->path(),
>  				     abigail::tools_utils::FILE_TYPE_RPM)
> -	      != file_is_kernel_package(second_package->base_name(),
> +	      != file_is_kernel_package(second_package->path(),
>  					abigail::tools_utils::FILE_TYPE_RPM))
>  	    {
>  	      emit_prefix("abipkgdiff", cerr)
> -- 
> 2.39.2
> 
> 
> 
> -- 
> 		Dodji