Skip to content
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.

Sign up for GitHub

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

Linux/MacOSX port of DriverLoader #10

Merged
merged 12 commits into from
Apr 25, 2016
Merged

Linux/MacOSX port of DriverLoader #10

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c1922d0
Linux/MacOSX port of DriverLoader
a1batross Apr 7, 2016
93a0275
Enable linking with -ldl. (harmless for Windows)
a1batross Apr 7, 2016
c981ac6
Move CMAKE_DL_LIBS variable to private section of target_link_libraries
a1batross Apr 8, 2016
79c81e9
Include platform-definiton header, add spaces in exception error string
a1batross Apr 8, 2016
e2968e0
Combine two identical macro
a1batross Apr 8, 2016
0f5bd0d
Add forgotten header, don't macro constructors
a1batross Apr 8, 2016
24fd52a
Add missing includes and fix mistype in std::getline call
a1batross Apr 8, 2016
c16a2fa
Include float.h for FLT_EPSILON
a1batross Apr 8, 2016
66fb7e2
Add missing fstream include. Fix string concat(when C++'s auto is evil)
a1batross Apr 8, 2016
d2c79dd
Fix invalid dlopen() call. Use RTLD_NOW, so it will return error on o…
a1batross Apr 8, 2016
48ff871
Remove macro around constructors
a1batross Apr 16, 2016
c3cc22e
Remove random \
a1batross Apr 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions DriverLoader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@
#define WIN32_LEAN_AND_MEAN
#define NO_MINMAX
#include <windows.h>
#elif defined(OSVR_LINUX)
#include <dlfcn.h>
#elif defined(OSVR_MACOSX)
#include <dlfcn.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is dlfcn.h how you open a .dylib on Mac OS X, or just how you open a .so? The terribly confusing part is that I know OS X has both of those... (@godbyk? @d235j?)

Update: It looks like it's correct for loading whatever CMAKE_SHARED_MODULE_SUFFIX is on Mac. Can't work worse than it does right now, so that's good :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I load libraries the same way in my project. Works perfect on both OSes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having the same code in separate OSVR_LINUX and OSVR_MACOSX branches or should they be conflated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a mistype.

#endif

namespace osvr {
Expand All @@ -48,10 +52,8 @@ namespace vive {
/// Platform-specific handle to dynamic library
#if defined(OSVR_WINDOWS)
HMODULE driver_ = nullptr;
#elif defined(OSVR_MACOSX)
#error "Implementation incomplete!"
#elif defined(OSVR_LINUX)
#error "Implementation incomplete!"
#elif defined(OSVR_MACOSX) || defined(OSVR_LINUX)
void *driver_ = nullptr;
#endif

/// Destructor: should contain platform-specific code to unload dynamic
Expand All @@ -61,10 +63,10 @@ namespace vive {
if (driver_) {
FreeLibrary(driver_);
}
#elif defined(OSVR_MACOSX)
#error "Implementation incomplete! Unload dynamic library here!"
#elif defined(OSVR_LINUX)
#error "Implementation incomplete! Unload dynamic library here!"
#elif defined(OSVR_MACOSX) || defined(OSVR_LINUX)
if (driver_) {
dlclose(driver_);
}
#endif
}
};
Expand All @@ -91,12 +93,19 @@ namespace vive {
throw CouldNotLoadEntryPoint();
}
factory_ = reinterpret_cast<DriverFactory>(proc);
#elif defined(OSVR_MACOSX)
#error \
"Implementation incomplete! Load dynamic library here and retrieve entry point function here!"
#elif defined(OSVR_LINUX)
#error \
"Implementation incomplete! Load dynamic library here and retrieve entry point function here!"
#elif defined(OSVR_LINUX) || defined(OSVR_MACOSX)
impl_->driver_ = dlopen(driverFile.c_str());
if (!impl_->driver_) {
reset();
throw CouldNotLoadDriverModule(dlerror());
}

auto proc = dlsym(impl_->driver_, ENTRY_POINT_FUNCTION_NAME);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to call dlerror() first here unconditionally? I'm looking at some of the code that loads OSVR plugins and it looks like that's what I do https://github.com/OSVR/libfunctionality/blob/master/src/libfunctionality/LoadPluginLibdl.h#L62

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dlerror() returns NULL when is no error. But in that case, this exception will be never thrown.

Also, calling dlerror() without taking returned pointer to string -- is really bad idea.
POSIX's libdl gives really nice reports through dlerror(). For example, in case loading 64 bit library in 32 bit app gives "Wrong elf class ELFCLASS64". So user can know where's the problem. And developer, ofc. :)

if (!proc) {
reset();
throw CouldNotLoadEntryPoint(dlerror());
}
factory_ = reinterpret_cast<DriverFactory>(proc); \
Copy link

@phiresky phiresky Apr 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a backslash at the end of this line (line 108)? Seems to break the build for me

#endif
}

Expand Down
14 changes: 14 additions & 0 deletions DriverLoader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,28 @@
namespace osvr {
namespace vive {
struct CouldNotLoadDriverModule : std::runtime_error {
#if defined(OSVR_LINUX) || defined(OSVR_MACOSX)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have these defines, you need to include <osvr/Util/PlatformDefines.h> - but I'd just leave those constructors available on all platforms. Just because only mac and linux use them doesn't mean having them will hurt the Windows build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, is Windows can give a error string.
But yes, there is can be a checking for NULL pointer. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually meant leave both constructors - a default constructor and one that takes an extra error message, with no platform ifdefs in this header at all...

CouldNotLoadDriverModule(const char *errString)
: std::runtime_error(
"Could not load driver module." +
std::string(errString)) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably add a space between the hard-coded C-string and the appended error string.

#else
CouldNotLoadDriverModule()
: std::runtime_error("Could not load driver module.") {}
#endif
};

struct CouldNotLoadEntryPoint : std::runtime_error {
#if defined(OSVR_LINUX) || defined(OSVR_MACOSX)
CouldNotLoadEntryPoint(const char *errString)
: std::runtime_error(
"Could not load entry point function from driver." +
std::string(errString)) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space here, too.

#else
CouldNotLoadEntryPoint()
: std::runtime_error(
"Could not load entry point function from driver.") {}
#endif
};

struct CouldNotGetInterface : std::runtime_error {
Expand Down