Review Request 112173: Do not use QFileInfo to obtain the size of a symlink in kio_trash

[Dawit Alemayehu]

> On Aug. 20, 2013, 1:29 p.m., David Faure wrote:
> > QFileInfo is more portable. Why not just write || info.isSymlink() in the first if?

I know QFileInfo is more portable. I use it exclusively whenever I can, but the logic it uses to calculate the size of a symlink will not work for this use case ; so I fail to see how simply adding "|| info.isSymlink()" to the first if statement solves this problem. That is why I said see the documentation for QFileInfo. Simply put info.size() returns the size of the actual file the symlink points to instead of the size of the symlink itself. You do not want that in this case because it will trigger an incorrect error message. 

There is a very simple test for this. Create a symlink to a file that is larger than the size of your trashcan. Make sure your trashcan is empty. Now delete the symlink by clicking on Move to Trash in Dolphin and Konqueror and see what you get.


> On Aug. 20, 2013, 1:29 p.m., David Faure wrote:
> > kioslave/trash/discspaceutil.cpp, line 43
> > <http://git.reviewboard.kde.org/r/112173/diff/1/?file=183601#file183601line43>
> >
> >     And if you really want to use stat, then this is a bit convoluted. No need to write a perfect isDir(), you're only using it in an "else" branch, i.e. you know that isFile() returned false already.
> >     IMHO just else if (S_ISDIR(buff.st_mode)) would be simpler and easier to read.
> >     (and folding isFile, too)
> >     
> >     But anyway, QFileInfo can do the job just fine IMHO.

I will simplify the patch and only handle the case of a symlink by itself and leave everything else alone. Perhaps that would make the problem as well as the fix more clearer.


- Dawit


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


On Aug. 20, 2013, 1:22 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112173/
> -----------------------------------------------------------
> 
> (Updated Aug. 20, 2013, 1:22 p.m.)
> 
> 
> Review request for KDE Runtime, David Faure and Tobias Koenig.
> 
> 
> Description
> -------
> 
> The attached patch fixes a bug where deleting a symlink to a very large file causes a "trash has reached its limit error. This happens because the code that is used to determine the amount of space available in the trashcan uses QFileInfo::size to determine the size of a file. This will not work because QFileInfo::size returns the size of the actual file the symlink points to and not the size of the symlink itself. See the documentation for QFileInfo.
> 
> 
> This addresses bug 253776.
>     http://bugs.kde.org/show_bug.cgi?id=253776
> 
> 
> Diffs
> -----
> 
>   kioslave/trash/discspaceutil.cpp 8e76ece 
> 
> Diff: http://git.reviewboard.kde.org/r/112173/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

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