Review Request: fix 4 TB wrap-around of filelight reported sizes on platforms with 64-bit long int

[Anssi Hannula]

> On Oct. 16, 2011, 8:23 a.m., Rolf Eike Beer wrote:
> > Your change makes perfect sense, but:
> > 
> > -according to https://projects.kde.org/projects/kde/kdeutils/filelight/repository/entry/src/part/fileTree.h?rev=master FileSize is defined as quint32, so this change will only make a changed FileSize properly work, but for the moment will not change anything
> > 
> > -a "long" is only 32 bits using Windows compilers like Microsoft Visual studio. Using a quint64 seems to be the right thing to make.

Ah, I'm sorry, I don't know how I missed that FileSize had been recently changed from unsigned long to quint32. So better apply the commit with a completely different commit message, then :) (or after FileSize has been changed)

Indeed, I guess making FileSize quint64 is the way to go (and probably removing the hack where the value is divided by 1024 to make wrap-around less likely). However, as noted above, the original code specifically avoids 64-bit int on 32-bit for performance reasons, so I'm a bit unsure about that as I don't know how much would it affect performance there. On the other hand, the wrap-around becomes more common all the time as disk sizes increase, and is probably more important an issue than performance...


- Anssi


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


On Oct. 15, 2011, 4:19 p.m., Anssi Hannula wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102870/
> -----------------------------------------------------------
> 
> (Updated Oct. 15, 2011, 4:19 p.m.)
> 
> 
> Review request for KDE Utils.
> 
> 
> Description
> -------
> 
> Filelight internally uses a FileSize type in fileTree.h to represent
> file sizes. It is defined as unsigned long and contains kilobytes,
> therefore it will wrap around at 4 TB when long int is 32-bit, while
> 64-bit is safe.
> 
> However, File::humanReadableSize() takes the size as uint instead of
> FileSize, causing it wrap-around even if long int would be big enough.
> This causes e.g. directories that take up 6 TB to have a caption
> saying 2 TB usage (the graph is generated correctly, though).
> 
> Change it to use FileSize to avoid wrap-around on 64-bit systems where
> long int is 64-bit, and to make it consistent with other code.
> 
> 32-bit systems continue to be affected.
> 
> This could be trivially fixed on 32-bit systems as well after this patch by making FileSize always a 64-bit integer (and maybe dropping the divide-by-1024 trick), but since 32-bit int seems to be used due to performance reasons and I couldn't test it on a real 32-bit system, I'll leave that to someone else who can test it or knows for sure that the performance impact would be less than the impact of the wrap-around bug.
> 
> 
> Diffs
> -----
> 
>   src/part/fileTree.h fe0a9ec 
>   src/part/fileTree.cpp 6da7a8f 
> 
> Diff: http://git.reviewboard.kde.org/r/102870/diff/diff
> 
> 
> Testing
> -------
> 
> Built filelight in kdeutils-4.6.5 with this change and wrap-around was fixed. Code in git master seems to have no actual code changes since then.
> 
> 
> Thanks,
> 
> Anssi Hannula
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20111016/2e8276f9/attachment.html>