meven added inline comments. INLINE COMMENTS
> dfaure wrote in kmountpoint.cpp:438 > Hmm, what if the symlink *is* the very last component, like your previous > iteration tried to handle? I made the incorrect assumption, I had already checked it > dfaure wrote in kmountpoint.cpp:445 > 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. On the contrary, thanks this is again a great review. Sorry for missing cases. > OK so maybe the recursion and redoing the stat's for all levels is correct. I think so, that's what canonicalFilePath does anyway. 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
