Skip to content

Commit

Permalink
Fixed|Audio|Client: Catch missing symbol errors when loading audio pl…
Browse files Browse the repository at this point in the history 
…ugins

IssueID #2134
  • Loading branch information
@danij-deng
danij-deng committed Nov 28, 2015
1 parent 92d7649 commit f799742
Showing 1 changed file with 15 additions and 6 deletions.
21 changes: 15 additions & 6 deletions doomsday/apps/client/src/audio/audiodriver.cpp
View file
Expand Up @@ -50,7 +50,7 @@ DENG2_PIMPL(AudioDriver)
zap(iCd);
}

static LibraryFile *findAudioPlugin(String const &name)
static LibraryFile *tryFindAudioPlugin(String const &name)
{
if(!name.isEmpty())
{
Expand Down Expand Up @@ -213,14 +213,23 @@ void AudioDriver::load(String const &identifier)
#endif

// Perhaps a plugin audio driver?
if(LibraryFile *plugin = Instance::findAudioPlugin(identifier))
LibraryFile *plugin = Instance::tryFindAudioPlugin(identifier);
if(!plugin)
{
d->importInterfaces(*plugin);
return;
/// @throw LoadError Unknown driver specified.
throw LoadError("AudioDriver::load", "Unknown driver \"" + identifier + "\"");
}

/// @throw LoadError Unknown driver specified.
throw LoadError("AudioDriver::load", "Unknown driver \"" + identifier + "\"");
try
{
// Exchange entrypoints.
d->importInterfaces(*plugin);

This comment has been minimized.

Sign in to view

Copy link
@skyjake

skyjake Nov 28, 2015

Owner

Note that this doesn't fix http://tracker.dengine.net/issues/2134 because in the reported case, Library_New returns nullptr instead of throwing an exception. In this case, audiodriver.cpp:110 just early-exits out of the function, leaving all the entrypoints null.
}
catch(de::Library::SymbolMissingError const &er)
{
/// @throw LoadError One or more missing symbol.
throw LoadError("AudioDriver::load", "Failed exchanging entrypoints:\n" + er.asText());
}
}

void AudioDriver::unload()
Expand Down

11 comments on commit f799742

@danij-deng
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why is Library_New returning nullptr on Fedora? That is something I can't debug on Windows. (Evidently the error stems from the FluidSynth plugin - which isn't available to me),

(This whole Library / de::Library / de::LibraryFile / library_s stuff is extremely messy and confusing).

@skyjake
Copy link
Owner

Choose a reason for hiding this comment

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

The core problem is that the dynamic linker fails to load the shared library (due to a missing dependent shared library). Doomsday is not handling this error case properly.

I've already fixed the build config issue that produced the faulty plugin library (ef572a0), however AudioDriver still needs handle this error case because dynamic linking can fail for instance due to incorrect/missing/corrupt system libraries.

The messiness in the APIs is due to the incomplete transition from the old v1 APIs to libcore Library-based APIs (library_s/Library is essentially a C wrapper now).

@danij-deng
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have already found the library file using LibraryFile and checked that it appears to be a Doomsday audio plugin before attempting to instantiate the de::Library - is it possible that the former can succeed but the latter not?

@skyjake
Copy link
Owner

Choose a reason for hiding this comment

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

checked that it appears to be a Doomsday audio plugin

Have we actually checked this? The audio plugins are detected based on their file name; any other verification first requires loading the shared library.

@danij-deng
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What other verification is there besides exchanging entrypoints (which occurs after de::Library instantiation)?

@skyjake
Copy link
Owner

Choose a reason for hiding this comment

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

See plugins.cpp:135. Calling any entrypoints in the library is of course impossible if the loading of the shared library fails.

I can take care of applying the necessary changes in AudioDriver, if you'd like.

@danij-deng
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please do because this logic has not changed since the previous stable release.

@danij-deng
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you saying that FS 2 can locate and recognize files as de::LibraryFile yet it is possible that a de::Library can't be produced from it? (Something seems awry there...)

@skyjake
Copy link
Owner

Choose a reason for hiding this comment

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

de::LibraryFile does not try to load the library until requested, and dynamic linking can fail due to various reasons. You can see the logic here: http://source.dengine.net/apidoc/sdk/libraryfile_8cpp_source.html#l00095

So it is indeed possible that a file is recognized as a de::LibraryFile but it can't be successfully loaded. The interpretation procedure can't try to attempt loading of every shared library it locates in the file tree due to performance reasons.

@danij-deng
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. It sounds like AudioDriver simply needs to handle Library_New returning nullptr and throw a LoadError in that case.

@skyjake
Copy link
Owner

Choose a reason for hiding this comment

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

That is exactly what I was going to do, yeah.

Please sign in to comment.