Fix SFTP Plugin of KIO for Windows
ClosedPublic

Authored by brute4s99 on Jun 26 2019, 4:56 PM.

Details

Summary

The fixed plugin works on Windows. Right now, KDE Connect uses it to establish an initial connection over SFTP, which can then be offloaded to any sftp:// handling application.

Currently, this can be achieved seamlessly by using WinSCP which is a free and open source SFTP Client.

Test Plan

0. Install WinSCP : https://winscp.net/eng/download.php.

  1. set Craft to use master for kio-extras
craft --set version=master kio-extras
  1. install kio-extras and all deps
craft -i kio-extras
  1. apply this patch
  1. re-build kio-extras
craft --compile --install --qmerge kio-extras
  1. checkout milestone2 branch from invent.kde.org/piyushaggarwal/kdeconnect-kde
  1. build kdeconnect-kde again
craft --compile --install --qmerge kdeconnect-kde
  1. Run kdeconnect-indicator.exe from within CraftRoot/bin/
  1. Right Click on the dark icon in sys tray, go to your phone and click on Browse Device.
  1. Press YES, followed by an OK on password prompt (the correct password comes pre-filled).
  1. Expect WinSCP to take point from there on.

Diff Detail

Repository
R320 KIO Extras
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
brute4s99 marked 6 inline comments as done.Jul 2 2019, 8:02 PM
brute4s99 added inline comments.Jul 2 2019, 8:02 PM
CMakeLists.txt
12

ah, I thought QFileDevice needs it. Reverting this.

brute4s99 marked an inline comment as done.Jul 3 2019, 6:20 AM
brute4s99 added inline comments.
sftp/kio_sftp.cpp
406

For my use case, I could not find any possibility of symlinks, (traversing Android filesystem) so I left it blank in here for any future dev to fix it.

Any reason why you skip the symlinks?
I mean displaying thm should be fine, dolphin probably can also follow them.
Creating them on windows is a bit more problematic.

Any reason why you skip the symlinks?
I mean displaying thm should be fine, dolphin probably can also follow them.
Creating them on windows is a bit more problematic.

For my use case, I could not find any possibility of symlinks, (traversing Android filesystem) so I left it blank in here for any future dev to fix it.

albertvaka added inline comments.Jul 3 2019, 6:25 PM
sftp/kio_sftp.cpp
2254

I think what they mean is that if you remove the ifdef it should just work.

brute4s99 added inline comments.Jul 3 2019, 6:38 PM
sftp/kio_sftp.cpp
2254

ah, gotcha!

brute4s99 added inline comments.Jul 3 2019, 8:50 PM
sftp/kio_sftp.cpp
2254

S_IFLNK is not defined on WIndows

brute4s99 updated this revision to Diff 61225.Jul 5 2019, 3:20 PM

added handling for WIndows. I'll try it on linux and revert on this patch!

brute4s99 added inline comments.Jul 5 2019, 3:21 PM
sftp/kio_sftp.cpp
2254

yes! thanks Hannah! ๐ŸŽ‰

brute4s99 marked 9 inline comments as done.Jul 5 2019, 3:21 PM
albertvaka added inline comments.Jul 5 2019, 7:57 PM
sftp/kio_sftp.cpp
2254

You can keep QT_STAT_LNK on every platform and remove the ifdef. That's the point of the abstraction Qt provides.

For consistency, I would also change the other (S_IFDIR, etc.) to their Qt counterparts.

brute4s99 updated this revision to Diff 61419.Jul 9 2019, 3:01 PM

Updated with QT_ pre-procs for S_IFDIR, S_IFLNK and others.

brute4s99 added inline comments.Jul 9 2019, 3:04 PM
sftp/kio_sftp.cpp
29

this builds without utime.h on my system (Arch Linux with latest Plasma, Qt and other packages), . Please inform if someone else has issues without this header.

vonreth added a comment.EditedJul 9 2019, 6:00 PM

I think you need to set(CMAKE_CXX_STANDARD 17) to make this compile with mingw

vonreth accepted this revision.Jul 9 2019, 6:09 PM

looks good

This revision is now accepted and ready to land.Jul 9 2019, 6:09 PM
dfaure requested changes to this revision.Jul 9 2019, 6:19 PM
dfaure added inline comments.
sftp/kio_sftp.cpp
1940โ€“1941

We're now doing stat() twice, once here, and once in QFileInfo just below.

This could be fixed by using QFileInfo for everything:

const bool bDestExists = info.exists();
1962โ€“1963

WRONG. Here we were testing the result of stat() on the .part file, see old line 1942.

Granted, reusing "buff" didn't make the code very readable....

Create a different QFileInfo instance for the .part file, and use it for bPartExists and for isDir() here.

2041

Why ReadWrite, if we know it doesn't exist?

Does setFileTime() even need open() first? I wouldn't have thought so.

2046

Again??

2058โ€“2059

= info.exists()

This revision now requires changes to proceed.Jul 9 2019, 6:19 PM
brute4s99 updated this revision to Diff 61972.Jul 18 2019, 11:56 AM
brute4s99 marked 7 inline comments as done.

updated wrt new comments. @dfaure please take another look! ๐Ÿ‘€

brute4s99 added inline comments.Jul 18 2019, 11:56 AM
sftp/kio_sftp.cpp
2041

setFileTime() needs the file to be open.
Source : https://doc.qt.io/qt-5/qfiledevice.html#setFileTime

dfaure requested changes to this revision.Jul 20 2019, 11:24 AM
dfaure added inline comments.
sftp/kio_sftp.cpp
1940โ€“1941

Please remove this variable, since you're not calling QT_STAT anymore.

You'll notice that you're still using buff.st_size, which should become QFileInfo's size() method instead...

1967

Red alert! Red alert! Uninitialized data being used!

2058โ€“2059

Same here

This revision now requires changes to proceed.Jul 20 2019, 11:24 AM
brute4s99 updated this revision to Diff 62159.EditedJul 20 2019, 11:41 PM
brute4s99 marked 4 inline comments as done.

updated

Please revert the changes to qCDebug

pino added a subscriber: pino.Jul 21 2019, 8:43 AM
pino added inline comments.
sftp/kio_sftp.cpp
2043

no need for QStringLiteral here, sending a const char * to debug is perfectly fine; also, the qDebug must be categorized, just like all the other debug outputs

2046โ€“2047

what is this commented code for?

brute4s99 added inline comments.Jul 21 2019, 10:10 AM
sftp/kio_sftp.cpp
2046โ€“2047

it uses buff.st_atime . Since I'm removing use of buff, I'm not sure how to handle this. For now I've commented out setting the file access time instruction for now.

dfaure requested changes to this revision.Jul 21 2019, 10:53 AM
dfaure added inline comments.
sftp/kio_sftp.cpp
266

Use kdebugsettings to enable sftp debug output instead of qCDebug->qDebug (which adds a lot of noise to the review)

2046โ€“2047

Just use partFile.fileTime(QFileInfoFileAccessTime)

2048

You can remove the FileTime() type conversion, just do setFileTime(dt, QFileDevice::FileModificationTime)

This revision now requires changes to proceed.Jul 21 2019, 10:53 AM
brute4s99 updated this revision to Diff 62187.Jul 21 2019, 12:31 PM
brute4s99 marked 6 inline comments as done.

updated

dfaure requested changes to this revision.Jul 21 2019, 6:35 PM

Almost there :-)

sftp/kio_sftp.cpp
406

Yes, QT_STAT_LNK works on Unix too, we use it in many places in KIO
(and it has the value S_IFLNK)

So I'm pretty sure you can remove this ifdef.

You could also replace S_IFREG with QT_STAT_REG and S_IFDIR with QT_STAT_DIR, for consistency.

2027โ€“2028

At this point we were doing another stat() in order to see how big the part file is *after* sftpGet.
You're reusing the old QFileInfo so that won't contain updated data.

You need to call QFileInfo::refresh() first.

2046

Sorry I gave wrong advice.
The old code was preserving the atime just because the utime() API forces us to specify an atime.
But what we want to do here is set the modification time, nothing else.
We don't care about the access time at all.
Please just remove this line.

2254

This is marked as Done, but it's not Done. In fact it matches my own suggestion above, so I agree with Albert ;-)

This revision now requires changes to proceed.Jul 21 2019, 6:35 PM
brute4s99 added inline comments.Jul 21 2019, 6:52 PM
sftp/kio_sftp.cpp
2254

so sorry, it seems I updated a previous version of the diff.

brute4s99 updated this revision to Diff 62237.Jul 21 2019, 8:51 PM
brute4s99 marked 3 inline comments as done.

updated

brute4s99 edited the summary of this revision. (Show Details)Jul 21 2019, 8:52 PM
dfaure requested changes to this revision.Jul 21 2019, 10:40 PM
dfaure added inline comments.
sftp/kio_sftp.cpp
2027โ€“2028

Better not do refresh on success, it slows things down for no purpose.

else {
    partFile.refresh();
    const int size = ...;
    if (partFile.exists() && partFile.size() < size) {
        partFile.remove();
    }
}
2045

coding style: join with previous line

} else {
This revision now requires changes to proceed.Jul 21 2019, 10:40 PM
brute4s99 updated this revision to Diff 62254.Jul 21 2019, 10:44 PM
brute4s99 marked an inline comment as done.

updated!

brute4s99 marked an inline comment as done.Jul 21 2019, 10:44 PM
dfaure accepted this revision.Jul 21 2019, 10:47 PM

Thanks for your persistence :-)

Do you have commit access? Otherwise can I have your name and email, for the git author information?

This revision is now accepted and ready to land.Jul 21 2019, 10:47 PM

yeah I can push! I'm a GSoCer lol

This revision was automatically updated to reflect the committed changes.

should've removed WIP before landing ๐Ÿ˜ฌ

Indeed. I assumed it wasn't in your actual commit log (bad idea, for this exact reason). Phabricator often shows outdated descriptions compared to the commit log (unless people use arc diff --verbatim) so I stopped complaining about the description.... Clearly I should have done it anyway :)

brute4s99 marked 2 inline comments as done.Jul 21 2019, 11:56 PM

I'll not leave it like this from next time, that's for sure. :p

brute4s99 retitled this revision from WIP : Fix SFTP Plugin of KIO for Windows to Fix SFTP Plugin of KIO for Windows.Jul 21 2019, 11:56 PM

:p