D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham abandoned this revision.
shubham added a comment.


  Abandoned in favor of D17632 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[David Faure]

dfaure added a comment.


  My proposal is at https://phabricator.kde.org/D17632

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[David Faure]

dfaure added a comment.


  In fact this is the wrong location for this code. The checks on the dest dir 
are in CopyJobPrivate::slotResultStating. I'm working on a different fix.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> jobtest.cpp:1004
> +
> +QSignalSpy spy(job, &KJob::error);
> +job->setUiDelegate(nullptr);

At runtime, Qt says:

  QSignalSpy: Not a valid signal: ''

Indeed this is the getter, not a signal. `result()` is the signal, but you 
don't really need to connect to it anyway.

> jobtest.cpp:1007
> +QVERIFY(!job->exec());
> +QCOMPARE(job->error(), (int)KIO::ERR_CANNOT_WRITE);
> +QCOMPARE(spy.count(), 1); // one error should be emitted by the move job

FAIL!  : JobTest::moveDirectoryToInaccessibleFilesystem() Compared values are 
not the same

  Actual   (job->error())  : 115
  Expected ((int)KIO::ERR_CANNOT_WRITE): 129

115 is ERR_ACCESS_DENIED, see `grep -w 15 src/core/global.h `

I think that's because you went too far (due to copy/pasting). The dest dir 
shouldn't be inaccessible, it should only be readonly.

Also you forgot the cleanup at the end of the method, which breaks running the 
test multiple times.

OK, even with all this the test doesn't pass.
Stepping into statNextSrc, I see that it goes into startRenameJob on line 846 
so it doesn't get into your new code which is further down.
That's because the src and dest are on the same partition, so a simple rename 
is enough.

Changing dst_dir to use otherTmpDir() ... ok, the error code changes, now it's 
137, i.e. ERR_COULD_NOT_MKDIR :-)
This requires further investigation...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham added a comment.


  Did anyone tried it?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham updated this revision to Diff 47549.
shubham added a comment.


  Unit test

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17545?vs=47503&id=47549

REVISION DETAIL
  https://phabricator.kde.org/D17545

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/copyjob.cpp

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham added a comment.


  @dfaure I'm not so familiar writing unit tests. This is one I have made by 
doing some modifications. would you like to give me some help?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham added a comment.


void JobTest::moveDirectoryToInaccessibleFilesystem()
{
#ifdef Q_OS_WIN
QSKIP("Skipping unaccessible folder test on Windows, cannot remove all 
permissions from a folder");
#endif
// Given a directory that cannot be moved to destination beacause 
filesystem is write protected
const QString src = homeTmpDir() + "srcdir";
const QString dest = homeTmpDir() + "destdir";
//QVERIFY(QDir().mkpath(src));
QVERIFY(QFileInfo(src).isDir());
QVERIFY(QFile(dest).setPermissions(QFile::Permissions())); // Make it 
inaccessible

KIO::CopyJob *job = KIO::move(QUrl::fromLocalFile(src), 
QUrl::fromLocalFile(dest), KIO::HideProgressInfo);
job->setUiDelegate(nullptr);
job->setUiDelegateExtension(nullptr);

// The job should fail with "access denied"
QVERIFY(!job->exec());
QCOMPARE(job->error(), (int)KIO::ERR_CANNOT_WRITE);

QVERIFY(QFile::exists(dest));

// Cleanup
QVERIFY(!QFile::exists(src));
}

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[David Faure]

dfaure added a comment.


  The logic is the same in all cases, the mode doesn't really make a 
difference, so I'm ok with picking one -- let's say Move, so we can also check 
that nothing disappeared.
  File or folder also makes no difference, so I'm ok with just one of these.
  The one thing that does make a difference is having more source items to 
copy/move, as discussed here, so maybe the test could try moving two files or 
two folders.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham marked an inline comment as done.
shubham added a comment.


  @dfaure do you need tests for all three modes ie. copy,move and link
  And individually for files and folders

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> shubham wrote in copyjob.cpp:868-872
>   emitresult() emits the corresponding signal and then commits suicide, hence 
> no such chance possible.

Still, don't call statNextSrc() then.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham added inline comments.

INLINE COMMENTS

> davidedmundson wrote in copyjob.cpp:868-872
> emitting a result and then continuing seems questionable, you'll end up with 
> the job emitting another result later

emitresult() emits the corresponding signal and then commits suicide, hence no 
such chance possible.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[David Edmundson]

davidedmundson added a comment.


  > And there should be a unittest for it.
  
  This still needs doing

INLINE COMMENTS

> copyjob.cpp:867
> +q->setError(ERR_CANNOT_WRITE);
> +q->setErrorText(m_currentDestURL.toDisplayString());
> +q->emitResult();

that's not going to be a helpful error text on it's own.

Code above calls buildErrorString

> copyjob.cpp:868-872
> +q->emitResult();
> +if (that) {
> +statNextSrc();
> +}
> +return;

emitting a result and then continuing seems questionable, you'll end up with 
the job emitting another result later

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: davidedmundson, ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham updated this revision to Diff 47503.
shubham marked 3 inline comments as done.
shubham added a comment.


  Done above requested changes.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17545?vs=47465&id=47503

REVISION DETAIL
  https://phabricator.kde.org/D17545

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, #frameworks, dfaure
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> copyjob.cpp:862
> +// if the destination file system doesn't support writing, do not 
> stat
> +QFileInfo destInfo(m_currentDestURL.toString());
> +if ((m_mode == CopyJob::Copy || m_mode == CopyJob::Move) && 
> (destInfo.isDir() && destInfo.isWritable())) {

Wrong mix of URLs and paths. How can this ever work? QFileInfo takes local 
paths, and you're giving it a full URL?

All this should be inside if (m_currentDestURL.isLocalFile()) and use 
m_currentDestURL.toLocalFile() as the path to give to QFileInfo.

> copyjob.cpp:863
> +QFileInfo destInfo(m_currentDestURL.toString());
> +if ((m_mode == CopyJob::Copy || m_mode == CopyJob::Move) && 
> (destInfo.isDir() && destInfo.isWritable())) {
> +QPointer that = q;

Why the "copy or move" test? The only alternative is creating a symlink, which 
also requires being able to write, no?

> copyjob.cpp:865
> +QPointer that = q;
> +emit q->warning(q, buildErrorString(ERR_CYCLIC_COPY, 
> m_currentDestURL.toDisplayString()));
> +if (that) {

This used to be an error, now it gets degraded to a warning. This means 
applications performing the copy will think it actually worked, only the user 
got (maybe) a warning

I think this should be an error.

And there should be a unittest for it. 
JobTest::copyFolderWithUnaccessibleSubfolder shows how to make a folder 
non-writable (and still be able to clean it up at the end). A similar test 
should be added where the destination is the one that is unaccessible (or just 
unwritable).

> broulik wrote in copyjob.cpp:864
> Not sure `ERR_CYCLIC_COPY` is the correct error for this?

It doesn't sound like it, no. This should rather be ERR_CANNOT_WRITE.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham added a subscriber: ngraham.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham added a reviewer: dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks, dfaure
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham updated this revision to Diff 47465.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17545?vs=47462&id=47465

REVISION DETAIL
  https://phabricator.kde.org/D17545

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Kai Uwe Broulik]

broulik added a comment.


  The bug report is about checking whether it's writable, you're checking the 
protocol here, and `file` will always "support writing". This patch could still 
make sense, though.

INLINE COMMENTS

> copyjob.cpp:864
> +QPointer that = q;
> +emit q->warning(q, buildErrorString(ERR_CYCLIC_COPY, 
> m_currentDestURL.toDisplayString()));
> +if (that) {

Not sure `ERR_CYCLIC_COPY` is the correct error for this?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

To: shubham, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D17545: Do not stat move/copy job if the destination file system does not support writing

[Shubham]

shubham created this revision.
shubham added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
shubham requested review of this revision.

REVISION SUMMARY
  BUG: 141564

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17545

AFFECTED FILES
  src/core/copyjob.cpp

To: shubham, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns