[Scummvm-devel] PORTERS: Fixing an old flaw in various FSNode implementations

[Max Horn]

Hi everybody, especially you porters,

with this email (part of a series) I want to inform you about some  
changes related to the FSNode code, the class File, and in general how  
we handle files. I will try to describe briefly what changed, why, and  
how we suggest you deal with it.  As always, questions, suggestions,  
wishlists and constructive criticism are welcome :).


BACKGROUND:

There is a nasty flaw in several of our FSNode implementations, which  
all go back to the same nasty bug in the POSIX FS code (i.e. my fault,  
as usual *g*). Namely, we did not try anything to normalize paths in  
there, and used a very sloppy lastPathComponent() implementation.

Why is this bad? Well, for one thing, it makes it difficult if not  
impossible to compare nodes. If I create a node once for  "/foo/bar"  
and once for "/foo/bar/", then client code can compare those notes and  
find that they are not equal, which is maybe not waht is expected.  
Also, with the old code, displayName() would return "bar" resp.  
"bar/". Not to mention paths like
    /foo///bar/./qux///
or
    C:\\My Driver\\Files\\\

So, what FSNodes really should do is to normalize the paths (at least  
in basic ways), and also use robust code to extract the last  
component / extract the path of the "parent" node. This affects most  
node constructors, as well as the getChild() and getParent() methods.

To fix this in the POSIX FSNode class, I added two new generic funcs  
to common/str.h, including Doxygen comments:
   Common::String lastPathComponent(const Common::String &path, const  
char sep);
   Common::String normalizePath(const Common::String &path, const char  
sep);
which all ports are welcome to use.

The advantage of all this: No more worrying about adding/removing  
trailing (back)slashes from paths. Normalized paths don't have a  
trailing (back)slash, period.



PORTERS TASK: Review your FSNode code for robustness. In particular,  
double-check your constructors and the getChild() and getParent()  
methods. Do they normalize paths, are they prepared to correctly  
extract the last path component / parent dir even for "ugly" paths? I  
recommend looking at the code in backends/fs/posix/posix-fs.* as  
reference (and if you think you find bugs in there, please report  
them). Also, you may want to use Common::normalizePath &  
Common::lastPathComponent, which hopefully are sufficiently generic  
for most ports.

AFFECTED PORTS:
  Essentially *all* except the POSIX one (which has already been  
fixed), i.e.: DC, Windows, Wii, Symbian, PSP,
  Not quite sure about PS2 and Amiga, but those definitely could  
benefit from some cleanup (I am happy to share my thoughts on this  
with the respective porters, just ping me).

WARNING: Make sure to use Common::lastPathComponent and *not*  
AbstractFilesystemNode::lastPathComponent, which I recently added as  
part of a refactoring process. That method will be removed as quick as  
possible.



Cheers,
Max