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

[Dodji Seketeli <dodji@seketeli.org>]

Hello Guillermo,

"Guillermo E. Martinez" <guillermo.e.martinez@oracle.com> a écrit:

> I'm agree with your approach!.

Thanks!

I have thus applied the patch to the master branch.  I am putting below
the exact patch that was applied.

Cheers,

From ca4f4d894c8fe4ce9eb4599c8b4f02694bdde587 Mon Sep 17 00:00:00 2001
From: "Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>
Date: Mon, 27 Feb 2023 19:45:11 +0100
Subject: [PATCH] abipkgdiff: Fix kernel package detection when comparing kABIs

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 in 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