-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libewf: on Linux check for system library first? #111
Comments
I can see the logic, but, the "problem" with that library (from my perspective) is it is updated (well maintained) regularly. If QH uses the system SO.2 file that was not shipped with QH, there's a good chance some of the function calls will, at some stage, maybe sooner rather than later, start to fail. At least by making it call the shipped SO file that I compiled for distribution with QH itself, I know that all my calls from QH will work with that library, until the time comes that I recompile the libewf library myself. |
As long as the library doesn't receive any updates that break the current API the soname Additionally you could put the GetProcAddress part into a separate function so if loading the system library succeeds but loading the symbols fails you can fall back to the bundled library (pseudo code): HANDLE handle;
function load_ewf_library(string filename) -> bool
{
handle = LoadLibrary("libewf.so.2");
if (!handle) return false;
if (load_symbol_addresses(handle) == false) {
FreeLibrary(handle);
return false;
}
return true;
}
// ...
if (load_ewf_library("libewf.so.2") == false &&
load_ewf_library(bundled_library_path) == false)
{
// error: cannot load library
} |
Here you can find a modified version: https://github.com/darealshinji/quickhash/tree/libewf Here's the diff with the changes: darealshinji@913fb85 It builds but I haven't checked if it actually does what it's supposed to do. |
Wouldn't it make sense on Linux to look for
libewf.so.2
in the system directories first and then try to load the bundled library?I also think the extra
FileExists()
checks aren't needed.The text was updated successfully, but these errors were encountered: