Page 1 of 1
Change requested in DynLibManager::load() function
Posted: Tue May 13, 2008 6:14 am
by knox_1234
Hi, in the DynLibManager::load() function, if the library that we are trying to load doesn't exist, then it results in memory leaks. Can the function be modified like this?
Code: Select all
DynLib* DynLibManager::load( const String& filename)
{
DynLibList::iterator i = mLibList.find(filename);
if (i != mLibList.end())
{
return i->second;
}
else
{
// modified .... Otherwise, we were having memory leaks if a particular module was not loaded...
DynLib *pLib = NULL ;
try
{
pLib = new DynLib(filename);
pLib->load();
}
catch(Ogre::Exception& e)
{
if(pLib)
delete pLib ;
pLib = NULL ;
OGRE_EXCEPT(Exception::ERR_INTERNAL_ERROR, e.getDescription(), "DynLibManager::load") ;
}
mLibList[filename] = pLib;
return pLib;
}
}
[/code]
Posted: Tue May 13, 2008 12:37 pm
by madmarx
(well done
)
I just want to say you can try to submit it as a patch, if you want, or ask someone else to do it for you, so that is becomes part of the official DynLibManager (if accepted).
Posted: Tue May 13, 2008 3:49 pm
by Praetor
Seems like a reasonable fix. The best thing to do with things like this are submit them as patches to sourceforge. Otherwise they might get lost in the forums as posts pile up. However, since this is an easy one I'll look into right now.
[EDIT] In your version there if the library load fails it will insert a NULL object into the library manager's list, causing problems later on. I don't think we want to delete the misloaded library immediately there. Instead, we should report the error but continue to manage the library as if it were loaded. The DynLibManager's normal operations will clean it up for us later.
Posted: Thu May 15, 2008 3:36 am
by syd
In your version there if the library load fails it will insert a NULL object into the library manager's list, causing problems later on. I don't think we want to delete the misloaded library immediately there. Instead, we should report the error but continue to manage the library as if it were loaded. The DynLibManager's normal operations will clean it up for us later.
indeed, right now, knowing DynLib::load throws an exception when it fails, Root::loadPlugin (the caller) won't store the DynLib pointer in the mPluginLibs container.
Now if the exception is caught in DynLibManager::load, Root::loadPlugin will store a null pointer, then tries to start it.
but the idea is good.
a possible workaround:
Code: Select all
DynLib* DynLibManager::load( const String& filename)
{
DynLibList::iterator i = mLibList.find(filename);
if (i != mLibList.end())
{
return i->second;
}
else
{
// modified .... Otherwise, we were having memory leaks if a particular module was not loaded...
DynLib *pLib = NULL ;
try
{
pLib = new DynLib(filename);
pLib->load();
}
catch(Ogre::Exception& e)
{
if(pLib)
delete pLib ;
pLib = NULL ;
//send exception to Root::loadPlugin
throw e;
}
mLibList[filename] = pLib;
return pLib;
}
}
void Root::loadPlugin(const String& pluginName)
{
// Load plugin library
DynLib* lib = NULL ;
try
{
DynLibManager::getSingleton().load( pluginName );
} catch(Ogre::Exception& e)
{
LogManager::getSingleton().logMessage("Warning: plugin " + pluginName + " couldn't be loaded: plugin ignored");
return;
}
// Store for later unload
mPluginLibs.push_back(lib);
// Call startup function
DLL_START_PLUGIN pFunc = (DLL_START_PLUGIN)lib->getSymbol("dllStartPlugin");
if (!pFunc)
OGRE_EXCEPT(Exception::ERR_ITEM_NOT_FOUND, "Cannot find symbol dllStartPlugin in library " + pluginName,
"Root::loadPlugin");
// This must call installPlugin
pFunc();
}
i hope this compiles
Edit:
after another though on this, I'm thinking it may be better not changing anything
since the application may depend on the plugin, even if this create a tiny memory leek, it would be less confusing to find the problem from a big error window then hidden in the log...