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

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

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