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

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:444
> 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?

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

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in copyjob.cpp:444
> 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

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?

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:1cac602d9966: [CopyJob] Check free space for remote urls 
before copying and other improvements (authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82334=82356

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

AFFECTED FILES
  src/core/copyjob.cpp

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir added a comment.


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

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:477
> 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.

That makes sense :)

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[David Faure]

dfaure accepted this revision.
dfaure added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> ahmadsamir wrote in copyjob.cpp:477
> 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).

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.

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir updated this revision to Diff 82334.
ahmadsamir added a comment.


  Remove comment

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82305=82334

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:477
> 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???)

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

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> copyjob.cpp:477
> +// Check available free space for remote urls
> +// TODO: find a way to report connection errors to the user
> +KIO::FileSystemFreeSpaceJob *spaceJob = 
> KIO::fileSystemFreeSpace(existingDest);

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

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir updated this revision to Diff 82305.
ahmadsamir marked an inline comment as done.
ahmadsamir added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82285=82305

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir marked an inline comment as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in 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.

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

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> copyjob.cpp:430
> +if (!m_privilegeExecutionEnabled && !isWritable) {
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +q->setError(ERR_WRITE_ACCESS_DENIED);

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.

> copyjob.cpp:448
>  const QString fileName = m_dest.fileName();
> +// The statJob that returned "entry" has taken into account 
> m_asMethod
>  m_dest = QUrl::fromLocalFile(sLocalPath);

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

> ahmadsamir wrote in copyjob.cpp:479
> s/below/below, because return is called right after it, so that 
> statCurrentSrc is called from the lambda/
> 
> does that sound better?

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

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir updated this revision to Diff 82285.
ahmadsamir marked 2 inline comments as done.
ahmadsamir added a comment.


  Address comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29485?vs=82113=82285

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir marked 2 inline comments as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in 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.

Copy-paste-comment logic mistake.

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

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

> dfaure wrote in copyjob.cpp:444
> 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.

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

> dfaure wrote in copyjob.cpp:478
> 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)

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

> copyjob.cpp:479
> +// Must do this here before statCurrentSrc() is called in the lambda 
> connected to
> +// FileSystemFreeSpaceJob below
>  q->removeSubjob(job);

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

does that sound better?

> dfaure wrote in copyjob.cpp:485
> 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.

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

> dfaure wrote in copyjob.cpp:486
> ... 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 ?

actualDest sounds nicer.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Good idea overall.

INLINE COMMENTS

> copyjob.cpp:430
> +if (!m_privilegeExecutionEnabled && !isWritable) {
> +// In copy-as mode, we want to check the directory to which 
> we're copying.
> +// The target file or directory does not exist yet.

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.

> copyjob.cpp:433
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +const QString path = dest.isLocalFile() ? dest.toLocalFile() 
> : dest.toString();
> +q->setError(ERR_WRITE_ACCESS_DENIED);

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

> copyjob.cpp:444
>  
> -const QString sLocalPath = 
> entry.stringValue(KIO::UDSEntry::UDS_LOCAL_PATH);
> -if (!sLocalPath.isEmpty() && kio_resolve_local_urls) {
> -const QString fileName = m_dest.fileName();
> -m_dest = QUrl::fromLocalFile(sLocalPath);
> -if (m_asMethod) {
> -m_dest = addPathToUrl(m_dest, fileName);
> +if (m_dest.isLocalFile()) {
> +// Fast code path, if a dir is already in the kcoredirlister 
> cache and it was e.g.

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.

> copyjob.cpp:445
> +if (m_dest.isLocalFile()) {
> +// Fast code path, if a dir is already in the kcoredirlister 
> cache and it was e.g.
> +// renamed and got UDS_LOCAL_PATH set

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.

> copyjob.cpp:460
>  }
> -qCDebug(KIO_COPYJOB_DEBUG) << "Setting m_dest to the local 
> path:" << sLocalPath;
> -if (isGlobalDest) {
> -m_globalDest = m_dest;
> +
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;

The check for m_dest.isLocalFile() goes here.

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

> copyjob.cpp:461
> +
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +const QString path = dest.toLocalFile();

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.

> copyjob.cpp:465
> +// Check available free space for local urls
> +KDiskFreeSpaceInfo freeSpaceInfo 
> =KDiskFreeSpaceInfo::freeSpaceInfo(path);
> +if (freeSpaceInfo.isValid()) {

missing space after =

> copyjob.cpp:478
>  
> +// Must do this here before statCurrentSrc() is called in the lambda 
> connected to
> +// FileSystemFreeSpaceJob below

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)

> copyjob.cpp:485
> +// TODO: find a way to report connection errors to the user
> +if (!m_dest.isLocalFile()) {
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;

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.

> copyjob.cpp:486
> +if (!m_dest.isLocalFile()) {
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +KIO::FileSystemFreeSpaceJob *spaceJob = 
> KIO::fileSystemFreeSpace(dest);

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

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  In D29485#664977 , @ahmadsamir 
wrote:
  
  > 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).

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Ahmad Samir]

ahmadsamir added a comment.


  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.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, meven, sitter.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION 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

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns