[Fixed] Ogre::FileSystemArchive issues

Discussion area about developing or extending OGRE, adding plugins for it or building applications on it. No newbie questions please, use the Help forum for that.
Post Reply
User avatar
Eugene
OGRE Team Member
OGRE Team Member
Posts: 183
Joined: Mon Mar 24, 2008 4:54 pm
Location: Odessa, Ukraine

[Fixed] Ogre::FileSystemArchive issues

Post by Eugene » Wed Sep 26, 2012 11:27 am

One of updates for our project (Live Interior 3D) does not passed Apple AppStore validation process due to the problems with Ogre::FileSystemArchive implementation. Specifically, application should not try to open its own resources in application bundle in read/write mode, or validation process would be failed. It`s similar to writing to C:\Program Files\ in Windows world.

Ogre::FileSystemArchive::load() tries to create temporary file to determine is it created in readonly or readwrite mode. This mechanism is bad not only because it brokes Apple`s guidelines but also it is unreliable (RO/RW mode and access rights for subfolders could be different from RO/RW mode and access rights for parent folder) and still have unnecessary file creation overhead.

Additionally Ogre::FileSystemArchive::open() has bug in implementation that always open stream in readonly mode if location was determined to be read-write

Code: Select all

	DataStreamPtr FileSystemArchive::open(const String& filename, bool readOnly) const
	{
		<...>
		if (!readOnly && isReadOnly())
		{
			mode |= std::ios::out;
			rwStream = OGRE_NEW_T(std::fstream, MEMCATEGORY_GENERAL)();
			rwStream->open(full_path.c_str(), mode);
			baseStream = rwStream;
		}
		else
		{
			roStream = OGRE_NEW_T(std::ifstream, MEMCATEGORY_GENERAL)();
			roStream->open(full_path.c_str(), mode);
			baseStream = roStream;
		}
To avoid all those problems, RO/RW mode should be passed explicitly via additional flag:

Code: Select all

ResourceGroupManager::addResourceLocation(..., bool readOnly = true)
    ArchiveManager::load(..., bool readOnly)
        FileSystemArchiveFactory::createInstance(..., bool readOnly)
            FileSystemArchive::FileSystemArchive(..., bool readOnly)
instead of unnecessary and error-prone runtime detection.

To fix bug in FileSystemArchive::open() code above should be replaced with

Code: Select all

	DataStreamPtr FileSystemArchive::open(const String& filename, bool readOnly) const
	{
		<...>
		if (!readOnly && isReadOnly())
		{
			OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS, 
				"Cannot open a file in read-write mode in a read-only archive", 
				"FileSystemArchive::open");
		}

		if(!readOnly)
		{
			mode |= std::ios::out;
			rwStream = OGRE_NEW_T(std::fstream, MEMCATEGORY_GENERAL)();
			rwStream->open(full_path.c_str(), mode);
			baseStream = rwStream;
		}
		else
		{
			roStream = OGRE_NEW_T(std::ifstream, MEMCATEGORY_GENERAL)();
			roStream->open(full_path.c_str(), mode);
			baseStream = roStream;
		}
Last edited by Eugene on Fri Nov 02, 2012 12:30 pm, edited 2 times in total.
0 x

User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
Contact:

Re: Ogre::FileSystemArchive issues

Post by Klaim » Wed Sep 26, 2012 4:07 pm

To avoid all those problems, RO/RW mode should be passed explicitly via additional flag:
May I suggest to use an enum instead of a bool, to make it more clear when you read the calling code?
0 x

User avatar
Eugene
OGRE Team Member
OGRE Team Member
Posts: 183
Joined: Mon Mar 24, 2008 4:54 pm
Location: Odessa, Ukraine

Re: Ogre::FileSystemArchive issues

Post by Eugene » Tue Oct 23, 2012 2:18 pm

Not yet fixed.
Ogre::FileSystemArchive::load() tries to create temporary file to determine is it created in readonly or readwrite mode. This mechanism is bad not only because it brokes Apple`s guidelines and fails AppStore validation process but also it is unreliable (RO/RW mode and access rights for subfolders could be different from RO/RW mode and access rights for parent folder) and still have unnecessary file creation overhead.
0 x

User avatar
masterfalcon
OGRE Team Member
OGRE Team Member
Posts: 4270
Joined: Sun Feb 25, 2007 4:56 am
Location: Bloomington, MN
Contact:

Re: Ogre::FileSystemArchive issues

Post by masterfalcon » Tue Oct 23, 2012 2:26 pm

Yes, you are correct. Sorry, been busy. I was also kind of waiting to see if you had an opinion on Klaim's suggestion.
0 x

User avatar
Eugene
OGRE Team Member
OGRE Team Member
Posts: 183
Joined: Mon Mar 24, 2008 4:54 pm
Location: Odessa, Ukraine

Re: Ogre::FileSystemArchive issues

Post by Eugene » Tue Oct 23, 2012 2:46 pm

masterfalcon wrote:Yes, you are correct. Sorry, been busy. I was also kind of waiting to see if you had an opinion on Klaim's suggestion.
It`s excellent suggestion. But I`m only reporter here, as my problem is already solved by commenting out this RW check, and I don`t want to implement generic fix and go through the Ogre bureaucracy after half year attempts to push WinRT patches - at least not in this decade.
0 x

User avatar
masterfalcon
OGRE Team Member
OGRE Team Member
Posts: 4270
Joined: Sun Feb 25, 2007 4:56 am
Location: Bloomington, MN
Contact:

Re: Ogre::FileSystemArchive issues

Post by masterfalcon » Tue Oct 23, 2012 4:09 pm

How's this?
Attachments
readonly-patch.diff.zip
(1.73 KiB) Downloaded 72 times
0 x

User avatar
Eugene
OGRE Team Member
OGRE Team Member
Posts: 183
Joined: Mon Mar 24, 2008 4:54 pm
Location: Odessa, Ukraine

Re: Ogre::FileSystemArchive issues

Post by Eugene » Tue Oct 23, 2012 5:15 pm

masterfalcon wrote:How's this?
It seems not very good to force Ogre::FactoryObj::createInstance() to have extra parameter readOnly. As call stack is

Code: Select all

ResourceGroupManager::addResourceLocation(..., bool readOnly = true)
    ArchiveManager::load(..., bool readOnly)
        FileSystemArchiveFactory::createInstance(..., bool readOnly)
            FileSystemArchive::FileSystemArchive(..., bool readOnly)
than may be better add createInstance(name, readOnly) to the class ArchiveFactory rather than FactoryObj,
and ArchiveManager::load would call this method instead of createInstance(name) as it knows that it has at least ArchiveFactory or derived.
Default implementation for ArchiveFactory::createInstance(name, readOnly) could assert that readOnly is true and invoke createInstance(name)
0 x

User avatar
Eugene
OGRE Team Member
OGRE Team Member
Posts: 183
Joined: Mon Mar 24, 2008 4:54 pm
Location: Odessa, Ukraine

Re: Ogre::FileSystemArchive issues

Post by Eugene » Fri Nov 02, 2012 12:16 pm

masterfalcon wrote:How's this?
Here https://bitbucket.org/sinbad/ogre/pull- ... -only/diff is revised patch, where readOnly flag is added to ArchiveFactory rather than to FactoryObj.
0 x

Post Reply