[CopyJob] Check free space for remote urls before copying and other improvements
ClosedPublic

Authored by ahmadsamir on May 6 2020, 3:30 PM.

Details

Summary

Use KIO::FileSystemFreeSpaceJob to check free space for remote urls.

Thanks to sitter for pin-pointing the responsible code in the bug report,
and to kbroulik for pointing out the existence of KIO::FileSystemFreeSpaceJob.

Also use UDSEntry to check writability for both local and remote urls.

BUG: 418443

FIXED-IN: 5.71

Test Plan

testtrash unit test fails (again...)

  • Find/create a local partition with a small capacity, copying a file/dir that is bigger than the partition will show the error message as before
  • Do the same for a remote url (I tested with sftp://), an error message about not having enough free space is shown

Diff Detail

Repository
R241 KIO
Branch
l-freespace-remote-2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26572
Build 26590: arc lint + arc unit
ahmadsamir created this revision.May 6 2020, 3:30 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 6 2020, 3:30 PM
ahmadsamir requested review of this revision.May 6 2020, 3:30 PM

I couldn't seem to test the m_privilegeExecutionEnabled stuff, i.e. using dolphin, the paste action is disabled if the dir isn't owned by me.

ngraham added a subscriber: ngraham.May 6 2020, 5:35 PM

I couldn't seem to test the m_privilegeExecutionEnabled stuff, i.e. using dolphin, the paste action is disabled if the dir isn't owned by me.

See D7563 (assistance would be appreciated if you have any idea how to make that work).

dfaure requested changes to this revision.May 8 2020, 9:26 AM

Good idea overall.

src/core/copyjob.cpp
430

Here you kept a comment that said "we want to check", but the check already happened.
I'd say just remove the two lines of comments.
The code is clearer without them.

433

That is not a path, for remote URLs. I suggest using dest.toDisplayString(QUrl::PreferLocalFiles) and just inlining that in the setErrorText call.

451

I think this check is too early.

UDS_LOCAL_PATH is also set by kio_desktop and kio_remote, for instance.
It's the main point of that entry: to map URLs from kioslaves-that-wrap-the-local-file-system back to local paths.

AFAICS the old code would use UDS_LOCAL_PATH also for non-local-file URLs.

452

I completely fail to see the relation between this comment and the code.

"If a dir is already in kcoredirlister" is only relevant when calling KCoreDirLister::cachedItemForUrl, but that's not done here.

455

The check for m_dest.isLocalFile() goes here.

Given line 451, maybe it wasn't local initially, and it is now.

456

Now *this* is where the comment about "we want to check..." belongs :-)

The explanation of why we go "up" in case of copyAs, in order not to confuse KDiskFreeSpaceInfo.

460

missing space after =

460

The lambda is called after going back to the event loop, so this will have happened anyway, no?
I don't get it.

On the other hand I'm fine if this is done here, I'm just not sure why the comment says it has to be so.

(Easy solution is to remove that comment, especially given the suggestion below it won't even seem weird to do things in this order)

465

Maybe you can move the if (m_dest.isLocalFile()) block here, and use else.
It'll be clearer that free space check happens in both cases.

466

... and this line is the same in both code paths, so it could be extracted to before the if().

To avoid confusion between dest and m_dest, maybe rename this var to destToCheck ?

This revision now requires changes to proceed.May 8 2020, 9:26 AM
ahmadsamir marked 2 inline comments as done.May 8 2020, 4:25 PM
ahmadsamir added inline comments.
src/core/copyjob.cpp
430

Copy-paste-comment logic mistake.

433

path as in "destination dir", but indeed technically it's not the 'path' part for remote urls. And toDisplayString will remove any passwords (not sure there may be passwords here, but still, safer that way).

451

Excellent points; I was scratching my head trying to figure out what use UDS_LOCAL_PATH is when, while testing with qDebug(), it was _always_ empty, the only time it wasn't empty was in testtrash unit test, some code there set it, used it or something. So I resorted to grep'ing the code and I found the KCoreDirLister::cachedItemForUrl instance (which answers your next comment). So someone who knows all the use-cases of UDS_LOCAL_PATH (you in this case) should/must update the UDS_LOCAL_PATH docs :D

460

"The lambda is called after going back to the event loop, so this will have happened anyway, no?"
Good point (thanks for the info, I didn't think of the event loop POV).

However, right after the connect(), 'return' is called, so as not to call statCurrentSrc() on line 502, I want it called from the lambda _after_ the free space check job finishes. In one of the iterations working on this patch I got a race, where it would sometimes work and error out if there isn't enough free space at a remote dest, but some other times it would start copying; hence the "must" part; I don't recall the exact details though (I've been working on it for a couple of days, I tried so many stuff, the details of the trial and error are mushed all together at this point.... sorry :/)

So, I'll remove the comment or improve it to convey the idea in a clearer way.

461

s/below/below, because return is called right after it, so that statCurrentSrc is called from the lambda/

does that sound better?

465

Looking at it again, removeSubjob can be called before the free disk space check, because it's statCurrentSrc that adds a new subJob (I was worried of calling removeSubjob too early).

466

actualDest sounds nicer.

ahmadsamir updated this revision to Diff 82285.May 8 2020, 4:26 PM
ahmadsamir marked 2 inline comments as done.

Address comments

dfaure added inline comments.May 8 2020, 5:18 PM
src/core/copyjob.cpp
430

This is the same as "actualDest" too, so its definition could be moved further up and shared with this too.

(Not that the name is perfect --- when copying ~/file.txt to ~/file2.txt, the actual destination is ~/file2.txt.
Or copying ~/dir1 as ~/dir2, then the actual destination is ~/dir2. So even a name like destDir isn't perfect...)

existingDest maybe. ~ exists. ~/dir2 not yet.

448

It has, but "taking into account" is ambiguous. I first thought it meant "we stat'ed the final dest", while in fact it means we stat'ed the parent existing dest. Maybe no comment is better than an ambiguous comment :-)

461

Yes. But comments should add information, not just describe what the code already says :-)

ahmadsamir marked an inline comment as done.May 8 2020, 6:33 PM
ahmadsamir added inline comments.
src/core/copyjob.cpp
430

This is the same as "actualDest" too, so its definition could be moved further up and shared with this too.

The code handling UDS_LOCAL_PATH is in between them, it may change m_dest.

And "existingDest" it is.

ahmadsamir updated this revision to Diff 82305.May 8 2020, 6:34 PM
ahmadsamir marked an inline comment as done.

"existingDest" is a better name for the var relating to m_asMethod

dfaure requested changes to this revision.May 8 2020, 7:34 PM

Sorry, I have one last comment about a comment :)

src/core/copyjob.cpp
478

This comment is now completely out of context. It used to be about NFS/SMB, now this information has gone and one is left wondering why kind of connections we're talking about (connection to the kioslave???)

This revision now requires changes to proceed.May 8 2020, 7:34 PM
ahmadsamir added inline comments.May 9 2020, 8:15 AM
src/core/copyjob.cpp
478

I meant connection to the remote ulr/host; however e.g Dolphin already reports those connection errors in the status bar when it can't connect, so the comment is redundant now... (I tried to keep the original code/comments in tact, apparently that backfired spectacularly).

ahmadsamir updated this revision to Diff 82334.May 9 2020, 8:16 AM

Remove comment

dfaure accepted this revision.May 9 2020, 9:01 AM
dfaure added inline comments.
src/core/copyjob.cpp
478

I'm assuming the TODO was about "What if I'm using a NFS mount and the connection breaks at the time of KDiskFreeSpaceInfo, i.e. what should we do about error handling".

But I think the current code -- which ignores errors and moves on, both for local and now for remote files, actually makes most sense. This is after all just a preliminary check. The worst that will happen is that there will indeed not be enough room and the copy will fail. But that's better than not trying at all, possibly due to a bug in one of those two classes, or possibly because of intermittent network failures.

This revision is now accepted and ready to land.May 9 2020, 9:01 AM
ahmadsamir added inline comments.May 9 2020, 10:53 AM
src/core/copyjob.cpp
478

That makes sense :)

@dfaure please don't forget to flesh out the UDS_LOCAL_PATH docs https://phabricator.kde.org/D29485#inline-169199

This revision was automatically updated to reflect the committed changes.
dfaure added inline comments.May 9 2020, 11:03 AM
src/core/copyjob.cpp
451

I'm a bit confused. My explanation here points to kio_desktop and kio_remote (and was apparently clear), and the API docs for UDS_LOCAL_PATH say

/// A local file path if the ioslave display files sitting
/// on the local filesystem (but in another hierarchy, e.g. settings:/ or remote:/)

which is basically the same information? What is unclear?

ahmadsamir added inline comments.May 9 2020, 11:07 AM
src/core/copyjob.cpp
451

to map URLs from kioslaves-that-wrap-the-local-file-system back to local paths.

That made much more sense to me, reading the docs again now, I know what it meant, but I did not before.