[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