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

Authored by shubham on Dec 12 2018, 3:23 PM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

BUG: 141564

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
shubham created this revision.Dec 12 2018, 3:23 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 12 2018, 3:23 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
shubham requested review of this revision.Dec 12 2018, 3:23 PM

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.

src/core/copyjob.cpp
864

Not sure ERR_CYCLIC_COPY is the correct error for this?

shubham updated this revision to Diff 47465.Dec 12 2018, 4:06 PM
dfaure requested changes to this revision.Dec 13 2018, 8:56 AM
dfaure added inline comments.
src/core/copyjob.cpp
862

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.

863

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

864

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

865

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).

This revision now requires changes to proceed.Dec 13 2018, 8:56 AM
shubham updated this revision to Diff 47503.Dec 13 2018, 10:24 AM
shubham marked 3 inline comments as done.

Done above requested changes.

And there should be a unittest for it.

This still needs doing

src/core/copyjob.cpp
867

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

Code above calls buildErrorString

868–872

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

shubham added inline comments.Dec 13 2018, 10:50 AM
src/core/copyjob.cpp
868–872
emitresult() emits the corresponding signal and then commits suicide, hence no such chance possible.
shubham marked an inline comment as done.Dec 13 2018, 10:52 AM
dfaure added inline comments.Dec 13 2018, 11:32 AM
src/core/copyjob.cpp
868–872

Still, don't call statNextSrc() then.

shubham marked an inline comment as done.Dec 13 2018, 1:35 PM

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

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.

This comment was removed by shubham.
shubham added a comment.EditedDec 13 2018, 2:52 PM

@dfaure This is one unit test I have made by doing some modifications. would you like to give me some help?

shubham updated this revision to Diff 47549.Dec 14 2018, 8:40 AM

Unit test

shubham marked an inline comment as done.Dec 14 2018, 8:42 AM

Did anyone tried it?

dfaure added inline comments.Dec 16 2018, 5:31 PM
autotests/jobtest.cpp
1004

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.

1007

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...

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.

shubham abandoned this revision.Jan 20 2019, 7:39 AM

Abandoned in favor of D17632