[PATCH] tools-utils: Looks for vmlinux binary in RPM debug package

[PATCH] tools-utils: Looks for vmlinux binary in RPM debug package

[Guillermo E. Martinez]

When Libabigail tools work with a `kernel' package, it looks for
`vmlinux' file in release RPM uncompress directory, if it is not
found, then try find it in `debug_info_root' directory.

	* src/abg-tools-utils.cc
	(get_binary_paths_from_kernel_dist): Add `debug_info_root' if
	`vmlinux' file is not found in `dist_root' directory.

Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
---
 src/abg-tools-utils.cc | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index 94dd8d05..8493ae90 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -2572,20 +2572,16 @@ get_binary_paths_from_kernel_dist(const string&	dist_root,
   string kernel_modules_root;
   string debug_info_root;
   if (dir_exists(dist_root + "/lib/modules"))
-    {
-      dist_root + "/lib/modules";
       debug_info_root = debug_info_root_path.empty()
-	? dist_root
+	? dist_root + "/usr/lib/debug"
 	: debug_info_root_path;
-      debug_info_root += "/usr/lib/debug";
-    }
 
   if (dir_is_empty(debug_info_root))
     debug_info_root.clear();
 
   bool found = false;
-  string from = dist_root;
-  if (find_vmlinux_and_module_paths(from, vmlinux_path, module_paths))
+  if (find_vmlinux_and_module_paths(dist_root, vmlinux_path, module_paths) ||
+      find_vmlinux_and_module_paths(debug_info_root, vmlinux_path, module_paths))
     found = true;
 
   std::sort(module_paths.begin(), module_paths.end());
-- 
2.39.1

[PATCH v2] abipkgdiff: Remove useless code to process kernel RPM packages

[Guillermo E. Martinez]

In `abipkgdiff' working with a `kernel' package, the function
`get_vmlinux_path_from_kernel_dist' that looks for  `vmlinux' file
in never reached, due to check an useless predicate.

	* tools/abipkgdiff.cc
	(compare_prepared_linux_kernel_packages): Remove useless predicate.

Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
---
 tools/abipkgdiff.cc | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index c2fc09ca..46b920a1 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -3106,8 +3106,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
 
   string vmlinux_path1, vmlinux_path2;
 
-  if (!vmlinux_path1.empty()
-      && !get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))
+  if (!get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))
     {
       emit_prefix("abipkgdiff", cerr)
 	<< "Could not find vmlinux in debuginfo package '"
@@ -3116,8 +3115,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
       return abigail::tools_utils::ABIDIFF_ERROR;
     }
 
-  if (!vmlinux_path2.empty()
-      && !get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
+  if (!get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
     {
       emit_prefix("abipkgdiff", cerr)
 	<< "Could not find vmlinux in debuginfo package '"
-- 
2.39.1

Re: [PATCH] tools-utils: Looks for vmlinux binary in RPM debug package

[Dodji Seketeli]

Hello,

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

I looked at the two patches you sent and I am proposing a third one
which is kind of a merge of the two ;-)

> In `abipkgdiff' working with a `kernel' package, the function
> `get_vmlinux_path_from_kernel_dist' that looks for  `vmlinux' file
> in never reached, due to check an useless predicate.
>
> 	* tools/abipkgdiff.cc
> 	(compare_prepared_linux_kernel_packages): Remove useless predicate.

[...]

> diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
> index c2fc09ca..46b920a1 100644
> --- a/tools/abipkgdiff.cc
> +++ b/tools/abipkgdiff.cc
> @@ -3106,8 +3106,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
>  
>    string vmlinux_path1, vmlinux_path2;
>  
> -  if (!vmlinux_path1.empty()
> -      && !get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))
> +  if (!get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))

Right, thanks for fixing the thinko there!

>      {
>        emit_prefix("abipkgdiff", cerr)
>  	<< "Could not find vmlinux in debuginfo package '"
> @@ -3116,8 +3115,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
>        return abigail::tools_utils::ABIDIFF_ERROR;
>      }
>  
> -  if (!vmlinux_path2.empty()
> -      && !get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
> +  if (!get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
>      {

Likewise.

>        emit_prefix("abipkgdiff", cerr)
>  	<< "Could not find vmlinux in debuginfo package '"

[...]

> When Libabigail tools work with a `kernel' package, it looks for
> `vmlinux' file in release RPM uncompress directory, if it is not
> found, then try find it in `debug_info_root' directory.
>
> 	* src/abg-tools-utils.cc
> 	(get_binary_paths_from_kernel_dist): Add `debug_info_root' if
> 	`vmlinux' file is not found in `dist_root' directory.

[...]

> diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
> index 94dd8d05..8493ae90 100644
> --- a/src/abg-tools-utils.cc
> +++ b/src/abg-tools-utils.cc
> @@ -2572,20 +2572,16 @@ get_binary_paths_from_kernel_dist(const string&	dist_root,
>    string kernel_modules_root;
>    string debug_info_root;
>    if (dir_exists(dist_root + "/lib/modules"))
> -    {
> -      dist_root + "/lib/modules";
>        debug_info_root = debug_info_root_path.empty()
> -	? dist_root
> +	? dist_root + "/usr/lib/debug"
>  	: debug_info_root_path;
> -      debug_info_root += "/usr/lib/debug";
> -    }

Here is how I am changing this finally:

  if (dir_exists(dist_root + "/lib/modules"))
    {
      kernel_modules_root = dist_root + "/lib/modules";
      debug_info_root = debug_info_root_path.empty()
	? dist_root + "/usr/lib/debug"
	: debug_info_root_path;
    }

This is because kernel_modules_root is thus going to be used below ...

[...]


>    bool found = false;
> -  string from = dist_root;
> -  if (find_vmlinux_and_module_paths(from, vmlinux_path, module_paths))
> +  if (find_vmlinux_and_module_paths(dist_root, vmlinux_path, module_paths) ||
> +      find_vmlinux_and_module_paths(debug_info_root, vmlinux_path, module_paths))
>      found = true;

... Like this:

  // If vmlinux_path is empty, we want to look for it under
  // debug_info_root, because this is where Enterprise Linux packages
  // put it.  Modules however are to be looked for under
  // kernel_modules_root.
  if (// So, Let's look for modules under kernel_modules_root ...
      find_vmlinux_and_module_paths(kernel_modules_root,
				    vmlinux_path,
				    module_paths)
      // ... and if vmlinux_path is empty, look for vmlinux under the
      // debug info root.
      || find_vmlinux_and_module_paths(debug_info_root,
				       vmlinux_path,
				       module_paths))
    found = true;

[...]

So, below is the resulting patch.

Could you please test it in your environment and tell me if it works for
you, the way you expect it?

Thanks.

From dc375bd1b7f15f41665f002ec61f68ce1e7ff889 Mon Sep 17 00:00:00 2001
From: "Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>
Date: Mon, 6 Mar 2023 09:50:38 -0600
Subject: [PATCH] tools-utils: Fix looking for vmlinux binary in debuginfo package

When abipkgdiff is invoked on a `kernel' package,
compare_prepared_linux_kernel_packages fails to look for the `vmlinux'
file from the debuginfo package, because of a thinko.

Then, build_corpus_group_from_kernel_dist_under, also fails to find
`vmlinux' from the debuginfo package because of another thinko.

This patch fixes the two thinkos.

	* src/abg-tools-utils.cc
	(get_binary_paths_from_kernel_dist): Fix a thinko and really use
	the kernel_modules_root variable.  Look for modules under
	kernel_modules_root and look for vmlinux (if necessary) under
	debug_info_root. Add comments.
	(compare_prepared_linux_kernel_packages): Fix another thinko.  Now
	we do have the path to vmlinux, from debuginfo packages before
	getting into get_binary_paths_from_kernel_dist.

Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-tools-utils.cc | 20 +++++++++++++++-----
 tools/abipkgdiff.cc    |  6 ++----
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index 94dd8d05..87af4445 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -2573,19 +2573,29 @@ get_binary_paths_from_kernel_dist(const string&	dist_root,
   string debug_info_root;
   if (dir_exists(dist_root + "/lib/modules"))
     {
-      dist_root + "/lib/modules";
+      kernel_modules_root = dist_root + "/lib/modules";
       debug_info_root = debug_info_root_path.empty()
-	? dist_root
+	? dist_root + "/usr/lib/debug"
 	: debug_info_root_path;
-      debug_info_root += "/usr/lib/debug";
     }
 
   if (dir_is_empty(debug_info_root))
     debug_info_root.clear();
 
   bool found = false;
-  string from = dist_root;
-  if (find_vmlinux_and_module_paths(from, vmlinux_path, module_paths))
+  // If vmlinux_path is empty, we want to look for it under
+  // debug_info_root, because this is where Enterprise Linux packages
+  // put it.  Modules however are to be looked for under
+  // kernel_modules_root.
+  if (// So, Let's look for modules under kernel_modules_root ...
+      find_vmlinux_and_module_paths(kernel_modules_root,
+				    vmlinux_path,
+				    module_paths)
+      // ... and if vmlinux_path is empty, look for vmlinux under the
+      // debug info root.
+      || find_vmlinux_and_module_paths(debug_info_root,
+				       vmlinux_path,
+				       module_paths))
     found = true;
 
   std::sort(module_paths.begin(), module_paths.end());
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index c2fc09ca..46b920a1 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -3106,8 +3106,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
 
   string vmlinux_path1, vmlinux_path2;
 
-  if (!vmlinux_path1.empty()
-      && !get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))
+  if (!get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))
     {
       emit_prefix("abipkgdiff", cerr)
 	<< "Could not find vmlinux in debuginfo package '"
@@ -3116,8 +3115,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
       return abigail::tools_utils::ABIDIFF_ERROR;
     }
 
-  if (!vmlinux_path2.empty()
-      && !get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
+  if (!get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
     {
       emit_prefix("abipkgdiff", cerr)
 	<< "Could not find vmlinux in debuginfo package '"
-- 
2.39.2



-- 
		Dodji

Re: [PATCH] tools-utils: Looks for vmlinux binary in RPM debug package

[Guillermo E. Martinez]

On Wed, Mar 08, 2023 at 03:34:24PM +0100, Dodji Seketeli wrote:
> Hello,
> 

Hello,

> "Guillermo E. Martinez via Libabigail" <libabigail@sourceware.org> a
> écrit:
> 
> I looked at the two patches you sent and I am proposing a third one
> which is kind of a merge of the two ;-)
> 
> > In `abipkgdiff' working with a `kernel' package, the function
> > `get_vmlinux_path_from_kernel_dist' that looks for  `vmlinux' file
> > in never reached, due to check an useless predicate.
> >
> > 	* tools/abipkgdiff.cc
> > 	(compare_prepared_linux_kernel_packages): Remove useless predicate.
> 
> [...]
> 
> > diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
> > index c2fc09ca..46b920a1 100644
> > --- a/tools/abipkgdiff.cc
> > +++ b/tools/abipkgdiff.cc
> > @@ -3106,8 +3106,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
> >  
> >    string vmlinux_path1, vmlinux_path2;
> >  
> > -  if (!vmlinux_path1.empty()
> > -      && !get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))
> > +  if (!get_vmlinux_path_from_kernel_dist(debug_dir1, vmlinux_path1))
> 
> Right, thanks for fixing the thinko there!
> 

Actually you point me about it. :-).

> >      {
> >        emit_prefix("abipkgdiff", cerr)
> >  	<< "Could not find vmlinux in debuginfo package '"
> > @@ -3116,8 +3115,7 @@ compare_prepared_linux_kernel_packages(package& first_package,
> >        return abigail::tools_utils::ABIDIFF_ERROR;
> >      }
> >  
> > -  if (!vmlinux_path2.empty()
> > -      && !get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
> > +  if (!get_vmlinux_path_from_kernel_dist(debug_dir2, vmlinux_path2))
> >      {
> 
> Likewise.
> 
> >        emit_prefix("abipkgdiff", cerr)
> >  	<< "Could not find vmlinux in debuginfo package '"
> 
> [...]
> 
> > When Libabigail tools work with a `kernel' package, it looks for
> > `vmlinux' file in release RPM uncompress directory, if it is not
> > found, then try find it in `debug_info_root' directory.
> >
> > 	* src/abg-tools-utils.cc
> > 	(get_binary_paths_from_kernel_dist): Add `debug_info_root' if
> > 	`vmlinux' file is not found in `dist_root' directory.
> 
> [...]
> 
> > diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
> > index 94dd8d05..8493ae90 100644
> > --- a/src/abg-tools-utils.cc
> > +++ b/src/abg-tools-utils.cc
> > @@ -2572,20 +2572,16 @@ get_binary_paths_from_kernel_dist(const string&	dist_root,
> >    string kernel_modules_root;
> >    string debug_info_root;
> >    if (dir_exists(dist_root + "/lib/modules"))
> > -    {
> > -      dist_root + "/lib/modules";
> >        debug_info_root = debug_info_root_path.empty()
> > -	? dist_root
> > +	? dist_root + "/usr/lib/debug"
> >  	: debug_info_root_path;
> > -      debug_info_root += "/usr/lib/debug";
> > -    }
> 
> Here is how I am changing this finally:
> 
>   if (dir_exists(dist_root + "/lib/modules"))
>     {
>       kernel_modules_root = dist_root + "/lib/modules";
>       debug_info_root = debug_info_root_path.empty()
> 	? dist_root + "/usr/lib/debug"
> 	: debug_info_root_path;
>     }
> 
> This is because kernel_modules_root is thus going to be used below ...
> 
> [...]
> 
> 
> >    bool found = false;
> > -  string from = dist_root;
> > -  if (find_vmlinux_and_module_paths(from, vmlinux_path, module_paths))
> > +  if (find_vmlinux_and_module_paths(dist_root, vmlinux_path, module_paths) ||
> > +      find_vmlinux_and_module_paths(debug_info_root, vmlinux_path, module_paths))
> >      found = true;
> 
> ... Like this:
> 
>   // If vmlinux_path is empty, we want to look for it under
>   // debug_info_root, because this is where Enterprise Linux packages
>   // put it.  Modules however are to be looked for under
>   // kernel_modules_root.
>   if (// So, Let's look for modules under kernel_modules_root ...
>       find_vmlinux_and_module_paths(kernel_modules_root,
> 				    vmlinux_path,
> 				    module_paths)
>       // ... and if vmlinux_path is empty, look for vmlinux under the
>       // debug info root.
>       || find_vmlinux_and_module_paths(debug_info_root,
> 				       vmlinux_path,
> 				       module_paths))
>     found = true;
> 
> [...]
> 
> So, below is the resulting patch.
> 
> Could you please test it in your environment and tell me if it works for
> you, the way you expect it?
> 

Working as intended.

> Thanks.
> 
>[...]

Thanks,
guillermo

Re: [PATCH] tools-utils: Looks for vmlinux binary in RPM debug package

[Dodji Seketeli]

Hello,

[...]

> On Wed, Mar 08, 2023 at 03:34:24PM +0100, Dodji Seketeli wrote:

>> So, below is the resulting patch.
>> 
>> Could you please test it in your environment and tell me if it works for
>> you, the way you expect it?

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

> Working as intended.

Dodji Seketeli <dodji@seketeli.org> a écrit:

[...]

> So, below is the resulting patch.

[...]

> From dc375bd1b7f15f41665f002ec61f68ce1e7ff889 Mon Sep 17 00:00:00 2001
> From: "Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>
> Date: Mon, 6 Mar 2023 09:50:38 -0600
> Subject: [PATCH] tools-utils: Fix looking for vmlinux binary in debuginfo package
>
> When abipkgdiff is invoked on a `kernel' package,
> compare_prepared_linux_kernel_packages fails to look for the `vmlinux'
> file from the debuginfo package, because of a thinko.
>
> Then, build_corpus_group_from_kernel_dist_under, also fails to find
> `vmlinux' from the debuginfo package because of another thinko.
>
> This patch fixes the two thinkos.
>
> 	* src/abg-tools-utils.cc
> 	(get_binary_paths_from_kernel_dist): Fix a thinko and really use
> 	the kernel_modules_root variable.  Look for modules under
> 	kernel_modules_root and look for vmlinux (if necessary) under
> 	debug_info_root. Add comments.
> 	(compare_prepared_linux_kernel_packages): Fix another thinko.  Now
> 	we do have the path to vmlinux, from debuginfo packages before
> 	getting into get_binary_paths_from_kernel_dist.
>
> Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>

OK, I have just applied this patch to master.

Thank you for time and effort.  It's appreciated.

[...]

Cheers,

-- 
		Dodji