Review Request 110225: Fix KMountPoint::List::findByPath(const QString&): /books is not a sub-path of /book

[Frank Reininghaus]

> On April 28, 2013, 8:26 a.m., David Faure wrote:
> > kdecore/io/kmountpoint.cpp, line 478
> > <http://git.reviewboard.kde.org/r/110225/diff/2/?file=141255#file141255line478>
> >
> >     The function would be shorter to write this way:
> >     
> >     
> >     if (parent == child) {
> >         return true;
> >     }
> >     if (parent.endsWith('/')) {
> >         return child.startsWith(parent);
> >     } else {
> >         return child.startsWith(parent + '/');
> >     }
> >     
> >     This also avoids the assert below.
> >     It creates a temp QString, though, so we need to choose between simplicity/readability and speed.
> >     
> >     Hmm, both implementations return false erroneously if parent="/books/" and child="/books", right?

You are right about the parent="/books/" and child="/books" issue. This works neither with the current code, nor with my patch, nor with your suggestion. I'll upload a new patch that takes care of this in a minute.

I think I would prefer a solution that does not create a temporary QString. Considering the issue mentioned above, I'm not sure if my new patch is more complex than a fixed version of your suggestion. But if you disagree, I'll be happy to change it.


> On April 28, 2013, 8:26 a.m., David Faure wrote:
> > kdecore/io/kmountpoint.cpp, line 488
> > <http://git.reviewboard.kde.org/r/110225/diff/2/?file=141255#file141255line488>
> >
> >     How can we be sure of that?
> >     
> >     We could have a mountpoint for /usr and one for /usr/local, so if the path is /usr we could end up here with child=/usr and parent=/usr/local, and the assert will fire.

At that point of the code, we know that child.startsWith(parent), and that the paths are not equal. Thus, it is guaranteed that child is longer than parent. I thought that it makes sense to document that by the assert because I access child.at(parent.length()) later on - people might wonder if that is actually safe to do.

But I'll remove the assert in the next version of the patch and just say in a comment that this is safe, maybe this is clearer.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110225/#review31692
-----------------------------------------------------------


On April 27, 2013, 9:29 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110225/
> -----------------------------------------------------------
> 
> (Updated April 27, 2013, 9:29 p.m.)
> 
> 
> Review request for kdelibs, David Faure and Jekyll Wu.
> 
> 
> Description
> -------
> 
> The current algorithm that tries to find out what mount point a path belongs to only checks if the first part of the string matches the mount point. The problem is that /books is then considered a path inside /book, which is obviously wrong.
> 
> I propose to fix this by verifying that either the mount point ends with a '/', or the first char of the path that does not match the mount point is a '/'. I've factored this check out into a separate function to keep the code readable.
> 
> Many thanks to Jekyll Wu, who analyzed this bug and found the right place in the code.
> 
> 
> This addresses bug 193298.
>     http://bugs.kde.org/show_bug.cgi?id=193298
> 
> 
> Diffs
> -----
> 
>   kdecore/io/kmountpoint.cpp aa7a6b1 
> 
> Diff: http://git.reviewboard.kde.org/r/110225/diff/
> 
> 
> Testing
> -------
> 
> Works for me.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130428/0d954bcb/attachment.htm>