dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kmountpoint.cpp:438
> + QStringList splitted = path.split(QDir::separator());
> + splitted.pop_back();
> + QString parentPath;
Hmm, what if the symlink *is* the very last component, like your previous
iteration tried to handle?
> kmountpoint.cpp:445
> + if (fileinfo.isSymLink()) {
> + return findByPath(fileinfo.symLinkTarget());
> + }
I don't want to be a pain, but this is still wrong....
If /home/dfaure is a symlink to /opt/dfaure, and then /opt/dfaure/tmp is a
symlink to /tmp, then the canonical path (and therefore the mount point) for
/home/dfaure/tmp is in fact /tmp.
But this is going to call findByPath(/opt/dfaure) (the symlink target of the
first symlink found in the path), and stop there, assuming that everything at
that target is part of the same mountpoint, which isn't necessarily the case.
I guess this should be findByPath(symLinkTarget() + remainder_of_path)
One possible objection is a case like /a/b/c/d/e/f/g where g is a symlink to h
(in the same directory), because then both levels of recursion will stat a, b,
c, d, e, f. But maybe this is unavoidable. I don't know how clever
canonicalPath() implementations are to optimize such things, while still
allowing for /a/b/c/d/e to be a symlink to something totally different, like
/x/y, where /x itself might be a symlink (!!).
OK so maybe the recursion and redoing the stat's for all levels is correct.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D26407
To: meven, #frameworks, ngraham, broulik, dfaure
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,
bruns
