[Scummvm-devel] File handling revamp

Pawel Kolodziejski aquadran at xtr.net.pl
Fri Aug 1 22:25:39 CEST 2008


On 2008-07-29, at 01:38, Max Horn wrote:

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


Could File class handle std, stdout console streams too ?


>
> * 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
>
> <dumpfile.patch>
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's  
> challenge
> Build the coolest Linux based applications with Moblin SDK & win  
> great prizes
> Grand prize is a trip for two to an Open Source event anywhere in  
> the world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/_______________________________________________
> Scummvm-devel mailing list
> Scummvm-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/scummvm-devel





More information about the Scummvm-devel mailing list