Change requested in DynLibManager::load() function

[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]

[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).

[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.

[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...