Review Request 112481: Revert port to QFile::Permissions

[Aurélien Gâteau]

> On Sept. 3, 2013, 4:35 p.m., David Faure wrote:
> > It's now kioslave/file/file.cpp, please adjust indeed.
> > 
> > Don't simplify isReadable: there's 000 (the first if), 444 (the second if), and other combinations (004, 040, 400) (the case where neither of these simple tests pass so we must use additional API to check for readability).
> > You suggestion "a 4 anywhere" doesn't make the file necessarily readable to us (if we don't own it).
> 
> Aurélien Gâteau wrote:
>     But then the second if should be "(S_IRUSR|S_IRGRP|S_IROTH) == d->m_permissions", not "(S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions", no?

Actually rather "((S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions) == (S_IRUSR|S_IRGRP|S_IROTH))"


- Aurélien


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


On Sept. 3, 2013, 4:04 p.m., Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112481/
> -----------------------------------------------------------
> 
> (Updated Sept. 3, 2013, 4:04 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Description
> -------
> 
> Partly revert commit b03e81a61311ae1b64b0d37415477f9c08fe6142 which ported KFileItem to QFile::Permissions.
> 
> Quoting David Faure:
> 
> "Port back from QFile::Permissions to mode_t given that mode_t is available in
> win32-msvc*/qplatformdefs.h (not everywhere, but in the places where the above
> commits did a half job at porting from mode_t to QFile::Permissions).
> 
> QFile::Permissions is more limited (no suid bits) and having to convert between
> mode_t and QFile::Permissions is just extra work."
> 
> I am not sure about the following part of KFileItemPrivate::init():
> 
>     } else { // link pointing to nowhere (see kio/file/file.cc)
>         mode = (QT_STAT_MASK - 1) | S_IRWXU | S_IRWXG | S_IRWXO;
>     }
> 
> I blindly brought back from the old code without understanding it. If anything, the comment needs to be updated as there is no kio/file/file.cc anymore.
> 
> I am also wondering if this part of the code in isReadable() could be simplified a bit:
> 
>     // No read permission at all
>     if (!(S_IRUSR & d->m_permissions) && !(S_IRGRP & d->m_permissions) && !(S_IROTH & d->m_permissions))
>         return false;
> 
>     // Read permissions for all: save a stat call
>     if ((S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions)
>         return true;
>  
> Isn't this the same as this?
> 
>     return (S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions;
> 
> Same thing for isWritable().
> 
> 
> Diffs
> -----
> 
>   kio/tests/kfileitemtest.cpp c8b7a5d 
>   staging/kio/src/core/kfileitem.h 623b426 
>   staging/kio/src/core/kfileitem.cpp 970dea8 
> 
> Diff: http://git.reviewboard.kde.org/r/112481/diff/
> 
> 
> Testing
> -------
> 
> All tests pass.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130903/6da1aa5f/attachment.html>