Important organizational changes in abipkgdiff.cc

This patch introduces many changes that should hopefully improve
legibility and ease of maintenance.  Here is a list of the topic of
the changes:

  * Avoid using shortened names when the line is not too long.

  * Use shared_ptr when possible.

  * When a function parameter is not meant to be nil, do not pass it
    as a pointer; rather, pass it as a reference.

  * Avoid doing things that can "fail" in a destructor; e.g, spawning
    a process.  Also, it's not common practise to cleanup a resource in a
    type destructor, when that resource has not been created in one of the
    member functions of the type.  It eases maintenance when resource
    creation and cleanup is performed at the same logical level.

	* tools/abipkgdiff.cc (option::package{1,2}): Rename
	option::pkg{1,2} into this, to increase legibility.
	(option::debug_package{1,2}): Likewise, rename
	option::debug_pkg{1,2} into this.
	(elf_file::~elf_file): Do not "delete this" in a destructor.  This
	leads to double free.  It's when someone invokes the "delete"
	operator on a pointer to the object that the destructor of the
	object is executed automatically; so if in the destructor the
	delete operator is called again, bad things are going to happen.
	As the destructor is now empty, remove it altogether.
	(elf_file_sptr): New typedef for shared_ptr<elf_file>.
	(package::path): Rename package::pkg_path into this, for better
	legibility.
	(package::extracted_package_dir_path): Rename
	package::extracted_pkg_dir_path into this.
	(package::type): Rename package::pkg_type into this.
	(package::is_debug_info): Rename package::is_debuginfo_pkg into
	this.
	(package::path_elf_file_sptr_map): Rename
	package::dir_elf_files_map into this because this is a map of
	path -> elf_file_sptr.  Also, now the value of the map element is
	a elf_file_sptr, no more an elf_file*.
	(package::debug_info_package): Rename package::debuginfo_pkg into
	this.
	(package::package): Adjust for the changes above.
	(package::{erase_extraction_directory,
	erase_extraction_directories}): New member functions.
	(elf_file_paths): Renamed dir_elf_files_path into this.
	(erase_created_temporary_directories)
	(create_maps_of_package_content)
	(extract_package_and_map_its_content, prepare_packages): New
	static functions.
	(get_soname, elf_file_type, extract_rpm): Make this static.
	(extract_package): Take a const package& rather than a
	package_sptr to express that the function really expects a non-nil
	object by reference (not by copy) and that the object won't be
	modified.  Using a reference removes the possibility that the
	pointer could be nil, causing crashes in the code where
	parameter->something was used.  Now only parameter.something can
	be used, so no crash possible there.  This is more solid code.
	(file_tree_walker_callback_fn): Rename callback() into this.  It
	makes the code more legible and kind of 'self-documented'.  At
	least you get the hint that this is a callback function for some
	file tree walking (ftw) function. Adjust for the relevant names
	renaming above.
	(compare): Rename compute_abidiff into this; again, this increases
	legibility; at least at the point of use of this function.  Rename
	compare_package() into a an overload of compare() as well.
	compare_package() used to take a vector of packages.  It was hard
	to guess by reading the signature of the function, which element
	of the vector is expected to be the first vector of the
	comparison, which one is to be the second, etc.  Now, this
	function takes two packages, named first_package and
	second_package.  That is more "typed"; that is, the signature is
	more meaningful.  Greater legibility, hopefully.  And in the body
	of the function, the debug information packages are now accessed
	using the package::debug_info_package data member.  Again, this is
	less surprising, I believe.  Also, explicitly erase the temporary
	files that were created during this comparison.  All this
	simplifies the logic of this function, hopefully.
	(parse_command_line): Make this static.  Add new --d1 and --d2
	command line switches that are shortcuts of --debug-info-pkg1 and
	--debug-info-pkg2.  Adjust this function for the relevant name
	changes above.  Make lines be shorter than 80 characters.
	(main): Do not create any vector of parameters anymore as the
	compare_packages() function don't take any vector of parameter
	anymore.  Just instantiate first_package and second_package now.
	Adjust for the relevant name changes above.  This hopefully
	simplifies the logic of this function.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
This commit is contained in:
Dodji Seketeli 2015-07-07 09:44:58 +02:00
parent c53994326e
commit e0b4f2b7e2

View File

@ -55,16 +55,16 @@ using abigail::tools_utils::guess_file_type;
using abigail::tools_utils::file_type;
using abigail::tools_utils::make_path_absolute;
vector<string> dir_elf_files_path;
vector<string> elf_file_paths;
struct options
{
bool display_usage;
bool missing_operand;
string pkg1;
string pkg2;
string debug_pkg1;
string debug_pkg2;
string package1;
string package2;
string debug_package1;
string debug_package2;
options()
: display_usage(false),
@ -92,13 +92,10 @@ struct elf_file
soname(soname),
type(type)
{}
~elf_file()
{
delete this;
}
};
typedef shared_ptr<elf_file> elf_file_sptr;
struct abi_changes
{
vector <string> added_binaries;
@ -108,33 +105,42 @@ struct abi_changes
struct package
{
string pkg_path;
string extracted_pkg_dir_path;
// string pkg_name;
abigail::tools_utils::file_type pkg_type;
bool is_debuginfo_pkg;
map<string, elf_file*> dir_elf_files_map;
shared_ptr<package> debuginfo_pkg;
string path;
string extracted_package_dir_path;
abigail::tools_utils::file_type type;
bool is_debug_info;
map<string, elf_file_sptr> path_elf_file_sptr_map;
shared_ptr<package> debug_info_package;
package(string path, string dir, abigail::tools_utils::file_type file_type,
bool is_debuginfo = false)
: pkg_path(path),
pkg_type(file_type),
is_debuginfo_pkg(is_debuginfo)
package(string path, string dir,
abigail::tools_utils::file_type file_type,
bool is_debug_info = false)
: path(path),
type(file_type),
is_debug_info(is_debug_info)
{
const char *tmpdir = getenv("TMPDIR");
if (tmpdir != NULL)
extracted_pkg_dir_path = tmpdir;
extracted_package_dir_path = tmpdir;
else
extracted_pkg_dir_path = "/tmp";
extracted_pkg_dir_path = extracted_pkg_dir_path + "/" + dir;
extracted_package_dir_path = "/tmp";
extracted_package_dir_path = extracted_package_dir_path + "/" + dir;
}
~package()
void
erase_extraction_directory() const
{
string cmd = "rm -rf " + extracted_pkg_dir_path;
string cmd = "rm -rf " + extracted_package_dir_path;
system(cmd.c_str());
}
void
erase_extraction_directories() const
{
erase_extraction_directory();
if (debug_info_package)
debug_info_package->erase_extraction_directory();
}
};
typedef shared_ptr<package> package_sptr;
@ -144,12 +150,12 @@ display_usage(const string& prog_name, ostream& out)
{
out << "usage: " << prog_name << " [options] <package1> <package2>\n"
<< " where options can be:\n"
<< " --debug-info-pkg1 <path> Path of debug-info package of package1\n"
<< " --debug-info-pkg2 <path> Path of debug-info package of package2\n"
<< " --help Display help message\n";
<< " --debug-info-pkg1|--d1 <path> Path of debug-info package of package1\n"
<< " --debug-info-pkg2|--d2 <path> Path of debug-info package of package2\n"
<< " --help Display help message\n";
}
string
static string
get_soname(Elf *elf, GElf_Ehdr *ehdr)
{
string result;
@ -195,8 +201,7 @@ get_soname(Elf *elf, GElf_Ehdr *ehdr)
return result;
}
elf_type
static elf_type
elf_file_type(const GElf_Ehdr* ehdr)
{
switch (ehdr->e_type)
@ -213,28 +218,35 @@ elf_file_type(const GElf_Ehdr* ehdr)
}
}
bool
extract_rpm(const string& pkg_path, const string &extracted_pkg_dir_path)
static bool
extract_rpm(const string& pkg_path, const string &extracted_package_dir_path)
{
string cmd = "mkdir " + extracted_pkg_dir_path + " && cd " +
extracted_pkg_dir_path + " && rpm2cpio " + pkg_path +
string cmd = "mkdir " + extracted_package_dir_path + " && cd " +
extracted_package_dir_path + " && rpm2cpio " + pkg_path +
" | cpio -dium --quiet";
if (!system(cmd.c_str()))
return true;
return false;
}
static void
erase_created_temporary_directories(const package& first_package,
const package& second_package)
{
first_package.erase_extraction_directories();
second_package.erase_extraction_directories();
}
static bool
extract_package(package_sptr pkg)
extract_package(const package& package)
{
switch(pkg->pkg_type)
switch(package.type)
{
case abigail::tools_utils::FILE_TYPE_RPM:
if (!extract_rpm(pkg->pkg_path, pkg->extracted_pkg_dir_path))
if (!extract_rpm(package.path, package.extracted_package_dir_path))
{
cerr << "Error while extracting package" << pkg->pkg_path << endl;
cerr << "Error while extracting package" << package.path << endl;
return false;
}
return true;
@ -246,7 +258,9 @@ extract_package(package_sptr pkg)
}
static int
callback(const char *fpath, const struct stat *, int /*flag*/)
file_tree_walker_callback_fn(const char *fpath,
const struct stat *,
int /*flag*/)
{
struct stat s;
lstat(fpath, &s);
@ -254,19 +268,19 @@ callback(const char *fpath, const struct stat *, int /*flag*/)
if (!S_ISLNK(s.st_mode))
{
if (guess_file_type(fpath) == abigail::tools_utils::FILE_TYPE_ELF)
dir_elf_files_path.push_back(fpath);
elf_file_paths.push_back(fpath);
}
return 0;
}
void
compute_abidiff (const elf_file* elf1, const string debug_dir1,
const elf_file* elf2, const string &debug_dir2)
static void
compare(const elf_file& elf1, const string& debug_dir1,
const elf_file& elf2, const string& debug_dir2)
{
cout << "Changes between " << elf1->name << " and " << elf2->name;
cout << "Changes between " << elf1.name << " and " << elf2.name;
cout << " =======>\n";
string cmd = "abidiff " +
elf1->path + " " + elf2->path;
elf1.path + " " + elf2.path;
if (!debug_dir1.empty())
cmd += " --debug-info-dir1 " + debug_dir1;
if (!debug_dir2.empty())
@ -275,95 +289,127 @@ compute_abidiff (const elf_file* elf1, const string debug_dir1,
}
static bool
compare_packages(vector<package_sptr>& packages)
create_maps_of_package_content(package& package)
{
for (vector< package_sptr>::iterator it = packages.begin();
it != packages.end();
++it)
elf_file_paths.clear();
if (ftw(package.extracted_package_dir_path.c_str(),
file_tree_walker_callback_fn,
16))
{
if (!extract_package(*it))
return false;
// Getting files path available in packages
if (!(*it)->is_debuginfo_pkg)
{
dir_elf_files_path.clear();
if (!(ftw((*it)->extracted_pkg_dir_path.c_str(), callback, 16)))
{
for (vector<string>::const_iterator iter =
dir_elf_files_path.begin();
iter != dir_elf_files_path.end(); ++iter)
{
int fd = open((*iter).c_str(), O_RDONLY);
if(fd == -1)
return false;
elf_version (EV_CURRENT);
Elf *elf = elf_begin (fd, ELF_C_READ_MMAP, NULL);
GElf_Ehdr ehdr_mem;
GElf_Ehdr *ehdr = gelf_getehdr (elf, &ehdr_mem);
string soname;
elf_type e = elf_file_type(ehdr);
if (e == ELF_TYPE_DSO)
soname = get_soname(elf, ehdr);
string file_base_name(basename(const_cast<char*>((*iter).c_str())));
if (soname.empty())
(*it)->dir_elf_files_map[file_base_name] =
new elf_file((*iter), file_base_name, e, soname);
else
(*it)->dir_elf_files_map[soname] =
new elf_file((*iter), file_base_name, e, soname);
}
}
else
cerr << "Error while getting list of files in package"
<< (*it)->extracted_pkg_dir_path << std::endl;
}
cerr << "Error while inspecting files in package"
<< package.extracted_package_dir_path << std::endl;
return false;
}
for (vector<string>::const_iterator file =
elf_file_paths.begin();
file != elf_file_paths.end();
++file)
{
int fd = open((*file).c_str(), O_RDONLY);
if(fd == -1)
return false;
elf_version (EV_CURRENT);
Elf *elf = elf_begin (fd, ELF_C_READ_MMAP, NULL);
GElf_Ehdr ehdr_mem;
GElf_Ehdr *ehdr = gelf_getehdr (elf, &ehdr_mem);
string soname;
elf_type e = elf_file_type(ehdr);
if (e == ELF_TYPE_DSO)
soname = get_soname(elf, ehdr);
string file_base_name(basename(const_cast<char*>((*file).c_str())));
if (soname.empty())
package.path_elf_file_sptr_map[file_base_name] =
elf_file_sptr(new elf_file((*file), file_base_name, e, soname));
else
package.path_elf_file_sptr_map[soname] =
elf_file_sptr( new elf_file((*file), file_base_name, e, soname));
}
return true;
}
static bool
extract_package_and_map_its_content(package& package)
{
if (!extract_package(package))
return false;
bool result = true;
if (!package.is_debug_info)
result |= create_maps_of_package_content(package);
return result;
}
static bool
prepare_packages(package& first_package,
package& second_package)
{
if (!extract_package_and_map_its_content(first_package)
|| !extract_package_and_map_its_content(second_package))
return false;
if ((first_package.debug_info_package
&& !extract_package(*first_package.debug_info_package))
|| (second_package.debug_info_package
&& !extract_package(*second_package.debug_info_package)))
return false;
return true;
}
static bool
compare(package& first_package, package& second_package)
{
if (!prepare_packages(first_package, second_package))
return false;
// Setting debug-info path of libraries
string debug_dir1, debug_dir2, relative_debug_path = "/usr/lib/debug/";
if (packages[0]->debuginfo_pkg != NULL)
if (first_package.debug_info_package
&& second_package.debug_info_package)
{
debug_dir1 = packages[2]->extracted_pkg_dir_path + relative_debug_path;
if (packages[1]->debuginfo_pkg != NULL)
debug_dir2 = packages[3]->extracted_pkg_dir_path + relative_debug_path;
debug_dir1 =
first_package.debug_info_package->extracted_package_dir_path +
relative_debug_path;
if (second_package.debug_info_package)
debug_dir2 =
second_package.debug_info_package->extracted_package_dir_path +
relative_debug_path;
}
else if (packages[1]->debuginfo_pkg != NULL)
debug_dir2 = packages[2]->extracted_pkg_dir_path + relative_debug_path;
for (map<string, elf_file*>::iterator it =
packages[0]->dir_elf_files_map.begin();
it != packages[0]->dir_elf_files_map.end();
for (map<string, elf_file_sptr>::iterator it =
first_package.path_elf_file_sptr_map.begin();
it != first_package.path_elf_file_sptr_map.end();
++it)
{
map<string, elf_file_sptr>::iterator iter =
second_package.path_elf_file_sptr_map.find(it->first);
map<string, elf_file*>::iterator iter =
packages[1]->dir_elf_files_map.find(it->first);
if (iter != packages[1]->dir_elf_files_map.end())
if (iter != second_package.path_elf_file_sptr_map.end())
{
compute_abidiff(it->second, debug_dir1, iter->second, debug_dir2);
packages[1]->dir_elf_files_map.erase(iter);
compare(*it->second, debug_dir1,
*iter->second, debug_dir2);
second_package.path_elf_file_sptr_map.erase(iter);
}
else
abi_diffs.removed_binaries.push_back(it->second->name);
}
for (map<string, elf_file*>::iterator it =
packages[1]->dir_elf_files_map.begin();
it != packages[1]->dir_elf_files_map.end();
for (map<string, elf_file_sptr>::iterator it =
second_package.path_elf_file_sptr_map.begin();
it != second_package.path_elf_file_sptr_map.end();
++it)
{
abi_diffs.added_binaries.push_back(it->second->name);
}
abi_diffs.added_binaries.push_back(it->second->name);
if (abi_diffs.removed_binaries.size())
{
cout << "Removed binaries\n";
for (vector<string>::iterator it = abi_diffs.removed_binaries.begin();
it != abi_diffs.removed_binaries.end(); ++it)
{
cout << *it << std::endl;
}
cout << *it << std::endl;
}
if (abi_diffs.added_binaries.size())
@ -371,15 +417,15 @@ compare_packages(vector<package_sptr>& packages)
cout << "Added binaries\n";
for (vector<string>::iterator it = abi_diffs.added_binaries.begin();
it != abi_diffs.added_binaries.end(); ++it)
{
cout << *it << std::endl;
}
cout << *it << std::endl;
}
erase_created_temporary_directories(first_package, second_package);
return true;
}
bool
static bool
parse_command_line(int argc, char* argv[], options& opts)
{
if (argc < 2)
@ -389,14 +435,15 @@ parse_command_line(int argc, char* argv[], options& opts)
{
if (argv[i][0] != '-')
{
if (opts.pkg1.empty())
opts.pkg1 = abigail::tools_utils::make_path_absolute(argv[i]).get();
else if (opts.pkg2.empty())
opts.pkg2 = abigail::tools_utils::make_path_absolute(argv[i]).get();
if (opts.package1.empty())
opts.package1 = abigail::tools_utils::make_path_absolute(argv[i]).get();
else if (opts.package2.empty())
opts.package2 = abigail::tools_utils::make_path_absolute(argv[i]).get();
else
return false;
}
else if (!strcmp(argv[i], "--debug-info-pkg1"))
else if (!strcmp(argv[i], "--debug-info-pkg1")
|| !strcmp(argv[i], "--d1"))
{
int j = i + 1;
if (j >= argc)
@ -404,10 +451,12 @@ parse_command_line(int argc, char* argv[], options& opts)
opts.missing_operand = true;
return true;
}
opts.debug_pkg1 = abigail::tools_utils::make_path_absolute(argv[j]).get();
opts.debug_package1 =
abigail::tools_utils::make_path_absolute(argv[j]).get();
++i;
}
else if (!strcmp(argv[i], "--debug-info-pkg2"))
else if (!strcmp(argv[i], "--debug-info-pkg2")
|| !strcmp(argv[i], "--d2"))
{
int j = i + 1;
if (j >= argc)
@ -415,7 +464,8 @@ parse_command_line(int argc, char* argv[], options& opts)
opts.missing_operand = true;
return true;
}
opts.debug_pkg2 = abigail::tools_utils::make_path_absolute(argv[j]).get();
opts.debug_package2 =
abigail::tools_utils::make_path_absolute(argv[j]).get();
++i;
}
else if (!strcmp(argv[i], "--help"))
@ -455,43 +505,44 @@ main(int argc, char* argv[])
return 1;
}
if (opts.pkg1.empty() || opts.pkg2.empty())
if (opts.package1.empty() || opts.package2.empty())
{
cerr << "Please enter two packages to compare" << endl;
return 1;
}
packages.push_back(package_sptr (new package(
opts.pkg1, "pkg1", guess_file_type(opts.pkg1))));
packages.push_back(package_sptr(new package(
opts.pkg2, "pkg2", guess_file_type(opts.pkg2))));
if (!opts.debug_pkg1.empty())
{
packages.push_back(package_sptr (new package(
opts.debug_pkg1, "debug_pkg1", guess_file_type(opts.debug_pkg1), true)));
packages[0]->debuginfo_pkg = packages[packages.size() - 1];
}
if (!opts.debug_pkg2.empty())
{
packages.push_back(package_sptr (new package(
opts.debug_pkg2, "debug_pkg2", guess_file_type(opts.debug_pkg2), true)));
packages[1]->debuginfo_pkg = packages[packages.size() - 1];
}
switch (packages.at(0)->pkg_type)
package_sptr first_package(new package(opts.package1, "package1",
guess_file_type(opts.package1)));
package_sptr second_package(new package(opts.package2, "package2",
guess_file_type(opts.package2)));
if (!opts.debug_package1.empty())
first_package->debug_info_package =
package_sptr(new package(opts.debug_package1, "debug_package1",
guess_file_type(opts.debug_package1),
/*is_debug_info=*/true));
if (!opts.debug_package2.empty())
second_package->debug_info_package =
package_sptr(new package(opts.debug_package2, "debug_package2",
guess_file_type(opts.debug_package2),
/*is_debug_info=*/true));
switch (first_package->type)
{
case abigail::tools_utils::FILE_TYPE_RPM:
if (!(packages.at(1)->pkg_type == abigail::tools_utils::FILE_TYPE_RPM))
if (!(second_package->type == abigail::tools_utils::FILE_TYPE_RPM))
{
cerr << opts.pkg2 << " should be an RPM file\n";
cerr << opts.package2 << " should be an RPM file\n";
return 1;
}
break;
default:
cerr << opts.pkg1 << " should be a valid package file \n";
cerr << opts.package1 << " should be a valid package file \n";
return 1;
}
return compare_packages(packages);
return compare(*first_package, *second_package);
}