From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay10.mail.gandi.net (relay10.mail.gandi.net [IPv6:2001:4b98:dc4:8::230]) by sourceware.org (Postfix) with ESMTPS id 187213858D33 for ; Wed, 8 Mar 2023 16:59:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 187213858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: (Authenticated sender: dodji@seketeli.org) by mail.gandi.net (Postfix) with ESMTPSA id 41A1F240011; Wed, 8 Mar 2023 16:59:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seketeli.org; s=gm1; t=1678294774; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=cd9jescJUTnIel2aUiejudw/Lq03Yn2yJUOJO2m0a0U=; b=cFlTnlWq2c0qzzX+zpw8q/07fsYWDqAMI4GA9a+kI/30mUEIUZaazMRF3rjS1esjAAbzRn /BgG49Cx+824UFIPSImg/ASiB9JELXpw5QzCcI/w8tlUiZurBVXp/zXIFbairASwcxVCi+ tJc6o7JQAVe+kOlB6oocDSJUGAWNJ/JPbLS4Y9O2G23XzDLSXuFBWLjkAk1Lxl5gm+r0YH aFJ84SrXkT1p94r2DfuuEtUww759YLKqLhloEJUpkM5M7Mqb+Uo3F8eet8yTjPb5MbaGnM rfgs0jeV6siIDuytXB5YC/MvnMKTmCzl1pxnhkc36m19mlNL76khPdJG0pz9DQ== Received: by localhost (Postfix, from userid 1000) id BFA19581C7B; Wed, 8 Mar 2023 15:34:24 +0100 (CET) From: Dodji Seketeli To: "Guillermo E. Martinez via Libabigail" Cc: "Guillermo E. Martinez" Subject: Re: [PATCH] tools-utils: Looks for vmlinux binary in RPM debug package Organization: Me, myself and I References: <20230306155038.3316079-1-guillermo.e.martinez@oracle.com> X-Operating-System: Fedora 38 X-URL: http://www.seketeli.net/~dodji Date: Wed, 08 Mar 2023 15:34:24 +0100 In-Reply-To: <20230306155038.3316079-1-guillermo.e.martinez@oracle.com> (Guillermo E. Martinez via Libabigail's message of "Mon, 6 Mar 2023 09:50:38 -0600") Message-ID: <875ybbpb0v.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hello, "Guillermo E. Martinez via Libabigail" a =C3=A9crit: 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& fir= st_package, >=20=20 > string vmlinux_path1, vmlinux_path2; >=20=20 > - 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& fir= st_package, > return abigail::tools_utils::ABIDIFF_ERROR; > } >=20=20 > - 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& d= ist_root, > string kernel_modules_root; > string debug_info_root; > if (dir_exists(dist_root + "/lib/modules")) > - { > - dist_root + "/lib/modules"; > debug_info_root =3D debug_info_root_path.empty() > - ? dist_root > + ? dist_root + "/usr/lib/debug" > : debug_info_root_path; > - debug_info_root +=3D "/usr/lib/debug"; > - } Here is how I am changing this finally: if (dir_exists(dist_root + "/lib/modules")) { kernel_modules_root =3D dist_root + "/lib/modules"; debug_info_root =3D 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 =3D false; > - string from =3D dist_root; > - if (find_vmlinux_and_module_paths(from, vmlinux_path, module_paths)) > + if (find_vmlinux_and_module_paths(dist_root, vmlinux_path, module_path= s) || > + find_vmlinux_and_module_paths(debug_info_root, vmlinux_path, modul= e_paths)) > found =3D 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 =3D 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" Date: Mon, 6 Mar 2023 09:50:38 -0600 Subject: [PATCH] tools-utils: Fix looking for vmlinux binary in debuginfo p= ackage 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 Signed-off-by: Dodji Seketeli --- 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& dis= t_root, string debug_info_root; if (dir_exists(dist_root + "/lib/modules")) { - dist_root + "/lib/modules"; + kernel_modules_root =3D dist_root + "/lib/modules"; debug_info_root =3D debug_info_root_path.empty() - ? dist_root + ? dist_root + "/usr/lib/debug" : debug_info_root_path; - debug_info_root +=3D "/usr/lib/debug"; } =20 if (dir_is_empty(debug_info_root)) debug_info_root.clear(); =20 bool found =3D false; - string from =3D 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 =3D true; =20 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, =20 string vmlinux_path1, vmlinux_path2; =20 - 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; } =20 - 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 '" --=20 2.39.2 --=20 Dodji