[Scummvm-devel] File handling revamp

[Max Horn]

Hi folks,

as mentioned on this list and elsewhere a few times in the past, I  
think our file handling system needs an overhaul. We already  
introduced the FSNode stuff some time ago to try to improve the  
situation with regards to abstracting the notion of paths, but that's  
obviously not enough.

Let me list the problems I perceive right now, and some proposal as to  
how to address this. As always, suggestions and constructive criticism  
is highly welcome. Sorry, this mail is quite long, but that just shows  
how bad I think the File handling code really is ... ;).


* Lack of documentation, both for implementors/porters, and for users.
-> Solution: Add more documentation :). I feel responsible for that,  
but need help. In particular, I'll need people to read whatever I  
fabricate (not just spelling, but also whether it is sufficiently  
detailed). But of course everyone's welcome to contribute to the docs.


* No good error handling, in particular, no clear error handling  
semantics. We provide a bool File::ioFailed() method, and also an  
eof() method, but we do not specify when exactly it will be set, and  
by which methods.
-> Solution: Adapt the ferror/clearerr semantics. They might not be  
the best in the world, but they are already explicitly defined and  
well-documented. People can just look at "man ferror", POSIX specs  
etc. if something is not clear in our docs. Writing good doxygen  
comments gets easier, too.


* The overly convoluted, complicated, under-documented and abused  
File::addDefaultDirectory() system (The blame is on me to a big  
extent, of course :). Semantics are unclear here, too. Right now, we  
blindly add every directory in our reach and it's friends, and then  
just hope the files we want are included somewhere there. And that  
there are no name clashes. No way of saying "game data files are in  
these dirs, GUI theme stuff please only from that dir". One specific  
problem: The AdvancedDetector's detectGame() function needs to support  
taking either a FSList param, or alternatively uses File::open() to  
locate files. Besides complicating the code, this causes a disparity  
between detecting and running games, which can lead to subtle bugs.

-> Solution: Scrap it! And replace it with something better. Right now  
I am not sure what *exactly* that means; we first will have to compile  
a list of requirements. I.e., what exactly are those search paths used  
for?. Then come up with a design proposal based on that.
One approach might be to introduce a SearchDirectory class, which is  
inited with a FSNode, and could cache the list of all files in it (and  
optionally of its subdirs, recursively). Then either File::open  
(optionally) takes (a list of) SearchDirectory object(s), and/or a  
global set of active SearchDirectories could be stored. This approach  
makes it easy to dynamically add/remove dirs from the search list  
(just dump the SearchDirectory object); or to search for files only in  
a subset of all search dirs. Not to mention that we get proper OO- 
style code encapsulation.


* Path separators (slashes) are silently creeping into File::open  
calls, decreasing portability in a fashion that currently is hard to  
detect.
-> Solution: Change the code to use FSNodes as it should, or implement  
the SearchDirectory proposal above, or something else which makes it  
possible to conveniently load files from a (set of) dirs.
Also: add a check to File::open() that warns (ultimately: errors out)  
when a slash is used in a file name.


* We have a very complex File::open method (did I already mention that  
we don't document the semantics of stuff anywhere? ;).
It has to do quite a lot: Handling search paths, trying to ignore  
case, fall back solution for alternate file locations, etc.. This  
leads to a very complex behavior, which can once more lead to subtle  
bugs.
-> Solution: Split it into two parts:
1) Locate the file, via search paths & directory FSNodes (which makes  
it possible to add support for case insensitivity in a much more  
logical and portable way), but also via port specific fallback options  
(like for Mac OS X & iPhone right now).
2) Open the file. See also below.


* On ports which do not support FILE, or where FILE is bugged, porters  
have to use #define tricks to get class File to work. The DS, PS2 and  
Symbian ports do just that, see common/file.cpp
-> Solution: Make it possible to provide alternate File-class  
implementations, just like for FSNodes. One way would be by adding a  
FileManager class, with internal methods like
   SeekableReadStream *openFileForReading(String name);
   SeekableReadStream *openFileForReading(FSNode);
   WriteStream *openFileForWriting(String name);
and then the File class simply calls these in open(), and otherwise  
calls-through to the stream object it obtained.


* Right now, the default Savefile also makes use of FILE. Same  
problems as above. We could change it to either use class File (which  
we currently don't, to avoid the nastiness that is called File::open).  
Or we could use the above FileManager class


* Writing to files: This is not really a serious issue with our  
current system, but something that contributes its share to the  
complexity of class File and to the generic portability issues. I am  
aware of the following reasons why we *write* data to files:
1) save states (handled via savefile managers)
2) config file
3) debug/resource dumps, screenshots, stuff like that
4) MT-32 emu cache; GUI theme cache; other cache files?
Well, for 1) we already provide dedicated code (the savefile manager).  
For 2) we badly need to provide dedicated code (see next point), and 3  
is purely optional. So, in short, ports could let openFileForWriting()  
return a dummy object which does nothing, if they don't support  
writing to arbitrary files.


* The config file right now is load & saved via class File. On the  
Dreamcast, saving is disabled using #ifdef. The PS2 port generates a  
fake special path to be able to handle the config file in a special  
way. Maybe other ports to extra trickery as well.
In addition, lots of ports provide special #ifdef'ed code to determine  
the config file location. Nasty. Yet another place porters have to be  
aware of.
-> Solution: Add another set of FileManager methods:
   SeekableReadStream *openConfigFileForReading();
   WriteStream *openConfigFileForWriting();
The default implementation could look like it does now, but the DC,  
PS2 and other ports could easily customize it. No more complicated  
tricks!

* File buffering: Recently some ports were bitten by the fact that  
lots of our code rlies on file buffering. If you look at line 205 in  
common/file.cpp, you'll also notice a
     setvbuf(file, NULL, _IOFBF, 8192);
(for __amigaos4__ only).
Well, we should document this need; we should have such an setvbuf on  
all systems where it is supported and makes sense. By letting backends  
override the file code in a more flexible way, we hopefully also make  
it easier to implement read buffering. If desired, we could even try  
to provide some generic read buffering code.


That's all for now. But I guess the list is long enough. Though I am  
sure I forgot stuff. Porters, speak up if you can think of something  
else that'd help! Engine maintainers and everybody else, speak up if  
you have concerns missing on this list!


Some more random rambling before I close:

* The more I think about it, the more I like the idea of having a   
pure virtual FileManager class (to be implemented by ports, with a  
default implementation based on FILE), which provides some  
minimalistic core API for creating File/DumpFile/ConfigFile/SaveFile  
streams. The point being that a port could use the exact same  
implementation for all these types of files, or use a completely  
custom one for each.
Also, if we decide that we still need a global set of  
SearchDirectories, then it might be useful to let the FileManager  
manage them.

* We could even merge the SaveFileManager into the FileManager, though  
I haven't really thought much about that, it's just a wild idea; the  
current system works well, so I would only change it if it provides a  
benefit to us, as such a change will force our porters to change an  
otherwise working system.

* For those who haven't noticed: One goal I have in mind for ScummVM  
is to move *all* port specific code into backends/, and not use any  
#ifdef tricks in our main code base. This may not be 100% possible,  
but I'd like to get as close as possible. It's just so much easier to  
create a new port when you only have to look into backends/ to see  
what all you have to do. Oh, and when things you have to implement are  
properly documented.... ah, what a nice dream :)

* As a first immediate (and temporary) step, I propose to split the  
reading and the writing part of our existing class File. The attached  
patch does that. This is only  temporary, and simplifies the code in  
class File a bit, enabling further refactoring. Whether we keep the  
split between File and DumpFile or eventually merge them back together  
remains to be seen, though right now I am in favor for keeping it.


Cheers,
Max

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dumpfile.patch
Type: application/octet-stream
Size: 31205 bytes
Desc: not available
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20080729/1dd814fb/attachment.obj>
-------------- next part --------------