From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130542 invoked by alias); 13 Oct 2015 07:04:26 -0000 Mailing-List: contact libabigail-help@sourceware.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: Sender: libabigail-owner@sourceware.org Received: (qmail 130090 invoked by uid 89); 13 Oct 2015 07:04:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.98.7 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 X-Spam-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-HELO: ms.seketeli.fr From: Dodji Seketeli To: Ondrej Oprala Cc: libabigail@sourceware.org Subject: Re: [Bug default/19082] New: Make abipkgdiff recognize suppression specifications to apply to comparisons Organization: Me, myself and I References: <561684AD.3000809@redhat.com> <86h9lw9occ.fsf@seketeli.org> <561C9F25.10608@redhat.com> X-Operating-System: Red Hat Enterprise Linux Workstation 7.2 Beta X-URL: http://www.seketeli.net/~dodji Date: Thu, 01 Jan 2015 00:00:00 -0000 In-Reply-To: <561C9F25.10608@redhat.com> (Ondrej Oprala's message of "Tue, 13 Oct 2015 08:05:25 +0200") Message-ID: <861tcz6yh9.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2015-q4/txt/msg00072.txt.bz2 Ondrej Oprala a =C3=A9crit: > diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc [...] > @@ -82,16 +83,21 @@ static bool verbose; > /// This contains the set of files of a given package. It's populated > /// by a worker function that is invoked on each file contained in the > /// package. So this global variable is filled by the > -/// file_tree_walker_callback_fn() function. Its content is relevant > -/// only during the time after which the current package has been > +/// file_tree_walker_callback_{elf,suppr}_fn() functions. Its content is > +/// relevant only during the time after which the current package has be= en This change should now be adjusted to the newer names of the callback functions. [...] > @@ -108,13 +114,15 @@ struct options > options() > : display_usage(), > missing_operand(), > + abignore(true), > keep_tmp_files(), > compare_dso_only(), > show_linkage_names(true), > show_redundant_changes(), > show_added_syms(true), > show_added_binaries(true), > - fail_if_no_debug_info() > + fail_if_no_debug_info(), > + suppression_paths() default-initializing the suppression_paths member here is not necessary because the default constructor of the vector type is going to be invoked by the default constructor of the option type. Or am I missing something? > {} > }; >=20=20 > @@ -146,6 +154,10 @@ public: > /// A convenience typedef for a shared pointer to elf_file. > typedef shared_ptr elf_file_sptr; >=20=20 > +/// A convenience typedef for a pointer to a function type that > +/// the ftw() function accepts. > +typedef int (*ftw_cb_type)(const char *, const struct stat*, int); > + > /// Abstract the result of comparing two packages. > /// > /// This contains the the paths of the set of added binaries, removed > @@ -653,9 +665,9 @@ extract_package(const package& package) > /// > /// @param stat the stat struct of the file. > static int > -file_tree_walker_callback_fn(const char *fpath, > - const struct stat *, > - int /*flag*/) > +first_package_tree_walker_callback_fn(const char *fpath, > + const struct stat *, > + int /*flag*/) Please update the comment of this function to say that this callback is invoked when walking the *first* package. [...] > { > struct stat s; > lstat(fpath, &s); > @@ -668,6 +680,30 @@ file_tree_walker_callback_fn(const char *fpath, > return 0; > } >=20=20 > +/// A callback function invoked by the ftw() function while walking > +/// the directory of files extracted from the second package, unless > +/// "--no-abignore" is specified on the command line. > +/// > +/// @param fpath the path to the file being considered. > +/// > +/// @param stat the stat struct of the file. > +static int > +second_package_tree_walker_callback_fn(const char *fpath, > + const struct stat *, > + int /*flag*/) > +{ > + struct stat s; > + lstat(fpath, &s); > + > + if (!S_ISLNK(s.st_mode)) > + { > + if (guess_file_type(fpath) =3D=3D abigail::tools_utils::FILE_TYPE_= ELF) > + elf_file_paths.push_back(fpath); > + else if (prog_options->abignore && string_ends_with(fpath, ".abign= ore")) > + prog_options->suppression_paths.push_back(fpath); > + } > + return 0; > +} > /// Update the diff context from the @ref options data structure. > /// > /// @param ctxt the diff context to update. > @@ -858,9 +894,11 @@ compare(const elf_file& elf1, > /// @param opts the options the current program has been called with. > /// > /// @param true upon successful completion, false otherwise. > + This is an unnecessary white space change. > static bool > create_maps_of_package_content(package& package, > - const options& opts) > + const options& opts, > + ftw_cb_type callback) > { > elf_file_paths.clear(); > if (verbose) > @@ -870,9 +908,7 @@ create_maps_of_package_content(package& package, > << package.extracted_dir_path() > << " ..."; >=20=20 > - if (ftw(package.extracted_dir_path().c_str(), > - file_tree_walker_callback_fn, > - 16)) > + if (ftw(package.extracted_dir_path().c_str(), callback, 16)) > { > cerr << "Error while inspecting files in package" > << package.extracted_dir_path() << "\n"; > @@ -919,14 +955,15 @@ create_maps_of_package_content(package& package, > /// @return true upon successful completion, false otherwise. > static bool > extract_package_and_map_its_content(package& package, > - const options& opts) > + const options& opts, > + ftw_cb_type callback) > { > if (!extract_package(package)) > return false; >=20=20 > bool result =3D true; > if (!package.is_debug_info()) > - result |=3D create_maps_of_package_content(package, opts); > + result |=3D create_maps_of_package_content(package, opts, callback); >=20=20 > return result; > } > @@ -947,8 +984,12 @@ prepare_packages(package& first_package, > package& second_package, > const options& opts) > { > - if (!extract_package_and_map_its_content(first_package, opts) > - || !extract_package_and_map_its_content(second_package, opts)) > + if (!extract_package_and_map_its_content(first_package, opts, > + first_package_tree_walker_callback_fn) > + /// We go through the files of the newer (second) pkg to look for > + /// suppression specifications, matching the "*.abignore" name pat= tern. I think this comment should rather be inside the function second_package_tree_walker_callback_fn. Because at this level, the second_package_tree_walker_callback_fn can do more (and does more) than looking for suppression specifications. Oh, and thank you very much for the test great quantity of new tests! That is really cool (and useful, haha) :-) So this patch is OK to commit to master with the changes above, if the result still passes "make distcheck", of course. Cheers! --=20 Dodji