[PATCH] abipkgdiff: Fix comparing CTF kABIs using Oracle Linux RPMs

[PATCH] abipkgdiff: Fix comparing CTF kABIs using Oracle Linux RPMs

[Guillermo E. Martinez]

Hello,

This patch is meant to fix how `abipkgdiff' using CTF origin is looking
for dependencies to analyze kABIs. IMHO `file_is_kernel_package'
function is relying on package name, so when `--ctf' is provided, it only
will work with executables under uncompressed directories and it never
will try to find `vmlinux.cfa' because it is just needed by KABIs. I
think that an option to force handle a packages as kernel would be nice
regardless the package name.

Please let me know your comments,
Thanks in advanced!,
guillermo
--

Using abipkgdiff to analyze kABIs from OL packages with CTF debug format
is not able to identify kernel packages, so the analysts is done as user
space RPMs, `file_is_kernel_package' function uses package name to
identify if the package contains kernel information, in OL distribution
the those packages are named as: `kernel-uek-core-*', then a wrong flow
is executed, additionally `vmlinux.ctfa' ship in a non debug package, so
the expected location is not satisfied .

	* 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.
	(file_is_kernel_package): Adjust package name identifying a kernel package.
	(build_corpus_group_from_kernel_dist_under): Add `root' package directory
	to debug info array.

Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
---
 src/abg-ctf-reader.cc  | 16 ++++++----------
 src/abg-tools-utils.cc | 14 ++++++++++++--
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
index 7159a578..ac0d3104 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..d7f8b71f 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;
@@ -1780,7 +1780,7 @@ file_is_kernel_package(const string& file_name, file_type file_type)
     {
       if (!get_rpm_name(file_name, package_name))
 	return false;
-      result = (package_name == "kernel");
+      result = (string_begins_with(package_name, "kernel"));
     }
   else if (file_type == FILE_TYPE_DEB)
     {
@@ -2812,6 +2812,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,
-- 
2.39.1

Re: [PATCH] abipkgdiff: Fix comparing CTF kABIs using Oracle Linux RPMs

[Guillermo E. Martinez]

Hello,

Any comment of this patch?

Thanks!,
guillermo

On Thu, Feb 16, 2023 at 06:57:43PM -0600, Guillermo E. Martinez wrote:
> Hello,
> 
> This patch is meant to fix how `abipkgdiff' using CTF origin is looking
> for dependencies to analyze kABIs. IMHO `file_is_kernel_package'
> function is relying on package name, so when `--ctf' is provided, it only
> will work with executables under uncompressed directories and it never
> will try to find `vmlinux.cfa' because it is just needed by KABIs. I
> think that an option to force handle a packages as kernel would be nice
> regardless the package name.
> 
> Please let me know your comments,
> Thanks in advanced!,
> guillermo
> --
> 
> Using abipkgdiff to analyze kABIs from OL packages with CTF debug format
> is not able to identify kernel packages, so the analysts is done as user
> space RPMs, `file_is_kernel_package' function uses package name to
> identify if the package contains kernel information, in OL distribution
> the those packages are named as: `kernel-uek-core-*', then a wrong flow
> is executed, additionally `vmlinux.ctfa' ship in a non debug package, so
> the expected location is not satisfied .
> 
> 	* 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.
> 	(file_is_kernel_package): Adjust package name identifying a kernel package.
> 	(build_corpus_group_from_kernel_dist_under): Add `root' package directory
> 	to debug info array.
> 
> Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
> ---
>  src/abg-ctf-reader.cc  | 16 ++++++----------
>  src/abg-tools-utils.cc | 14 ++++++++++++--
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 7159a578..ac0d3104 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..d7f8b71f 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;
> @@ -1780,7 +1780,7 @@ file_is_kernel_package(const string& file_name, file_type file_type)
>      {
>        if (!get_rpm_name(file_name, package_name))
>  	return false;
> -      result = (package_name == "kernel");
> +      result = (string_begins_with(package_name, "kernel"));
>      }
>    else if (file_type == FILE_TYPE_DEB)
>      {
> @@ -2812,6 +2812,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,
> -- 
> 2.39.1
> 

Re: [PATCH] abipkgdiff: Fix comparing CTF kABIs using Oracle Linux RPMs

[Dodji Seketeli]

Hello Guillermo,

"Guillermo E. Martinez via Libabigail" <libabigail@sourceware.org> a
écrit:

> Hello,
>
> This patch is meant to fix how `abipkgdiff' using CTF origin is looking
> for dependencies to analyze kABIs. IMHO `file_is_kernel_package'
> function is relying on package name, so when `--ctf' is provided, it only
> will work with executables under uncompressed directories and it never
> will try to find `vmlinux.cfa' because it is just needed by KABIs. I
> think that an option to force handle a packages as kernel would be nice
> regardless the package name.
>
> Please let me know your comments,
> Thanks in advanced!,

Ahh, okay, I see.  Actually, I think the initial way in which
file_is_kernel_package detects that a package is a kernel package is
sub-optimal.  Rather than looking at the name of the package, I think it
should instead look for the 'vmlinuz' binary file inside the package.

I will soon post a patch that amends yours by fixing that initial issue
and adds the vmlinux.ctfa related fix to it.

[...]

Thanks for looking into this.

Cheers,

-- 
		Dodji

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

[Dodji Seketeli]

Hello Guillermo,

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.

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

Thanks.


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

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

[Guillermo E. Martinez]

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

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

[Dodji Seketeli]

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