Skip to content

Commit

Permalink
Check dir exec + read permissions before reading
Browse files Browse the repository at this point in the history
  • Loading branch information
@shundhammer
shundhammer committed Jun 4, 2017
1 parent 82a7b4e commit a3af773
Showing 1 changed file with 25 additions and 9 deletions.
34 changes: 25 additions & 9 deletions src/DirReadJob.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <sys/stat.h>
#include <unistd.h>
#include <stdio.h>
#include <sys/errno.h>

#include <QMutableListIterator>

Expand Down Expand Up @@ -163,7 +162,31 @@ void LocalDirReadJob::startReading()

// logDebug() << _dir << endl;

if ( ( _diskDir = opendir( dirName.toUtf8() ) ) )
bool ok = true;

if ( access( dirName.toUtf8(), X_OK | R_OK ) != 0 )
{
ok = false;
logWarning() << "No permission to read directory " << dirName << endl;

This comment has been minimized.

Sign in to view

Copy link
@slodki

slodki Jun 5, 2017 •
edited
Loading

Contributor

This statement can be wrong. Should be "No permission to read directory dirName content's details"

This comment has been minimized.

Sign in to view

Copy link
@shundhammer

shundhammer Jun 5, 2017

Author Owner

Well, this covers two cases: No read permission or no execute permission.

So, technically, yes, it might not be 100% accurate, but it confuses the user a lot less than having separate messages for either case or to fabricate a 100%-correct-but-hard-to-understand message. 😄

I've seen too many such messages during my 25+ years as a professional software developer: Mathematically correct, but incomprehensible to the mere human.

To all intents and purposes, the result is the same: The directory cannot be read. It is a moot point to the normal user which of the syscalls exactly failed; the net result is that he won't get information about that directory. If he can, he might check and adapt the permissions, if not, he'll just have to deal with it.

This comment has been minimized.

Sign in to view

Copy link
@slodki

slodki Jun 5, 2017

Contributor

But when user will want to fix it it will use chmod +r (or use file manger to add read permission) and will get the same error "no permission to read". It will be confused.

This comment has been minimized.

Sign in to view

Copy link
@shundhammer

shundhammer Jun 5, 2017

Author Owner

So you think somebody who knows how to use chmod doesn't know what permissions he needs to actually access a directory? Ah, come on.
_dir->setReadState( DirError );
finishReading( _dir );
}

if ( ok )
{
_diskDir = opendir( dirName.toUtf8() );

if ( ! _diskDir )
{
ok = false;
_dir->setReadState( DirError );
logWarning() << "opendir(" << dirName << ") failed" << endl;
// opendir() doesn't set 'errno' according to POSIX :-(
finishReading( _dir );
}
}

if ( ok )
{
_dir->setReadState( DirReading );

Expand Down Expand Up @@ -301,13 +324,6 @@ void LocalDirReadJob::startReading()
_dir->setReadState( DirFinished );
finishReading( _dir );
}
else
{
_dir->setReadState( DirError );
logWarning() << "opendir(" << dirName << ") failed" << endl;
// opendir() doesn't set 'errno' according to POSIX :-(
finishReading( _dir );
}

finished();
// Don't add anything after finished() since this deletes this job!
Expand Down

0 comments on commit a3af773

Please sign in to comment.