KIO-SFTP backport #279

This PR is a backport of a version of the SFTP ioslave based on libssh.

This was based on the KDE2 port I mentioned in issue #276 and the actual KDE5 version of kio-sftp.

~~Basic testing suggests it mostly works, but there are some issues. This list is not exhaustive:~~

What works

Browsing directories
Opening most files via TDEParts
File previews
Moving/renaming files
Creating directories
Copying files (local & remote)
Symlinks

What does not work

~~Opening symlinks~~ fixed.
~~Putting files, either by copying a local file or by creating one (results in a crash)~~ fixed.
URLs with no usernames (hangs?)

Needs testing

Public key authentication

Other issues

~~Random hangs~~ probably fixed.
~~The password dialog pops up too many times.~~ fixed.

Great, good work. I was planning to work on this myself at some point, but you have been quicker :-)
If you work on solving the remaining issues, then I will have a look. Unless you prefer I first take a look at the existing code.

Btw, good to mentioned in the commit where we copy the backported code from and that it is available under GPL2 license.

I'll finish with the remaining issues probably tomorrow.

no worries, ping me when this is ready for review and I will do so.

@MicheleC Looks like I finished much earlier than I expected. The issues were trivial after all.

I can say that the IOSlave seems fully functional at this point with the features I tested.

Special attention in testing should be paid to public key authentication (it's the first that the code tries anyway) because I'm not sure whether it works. I tried to imitate the code of the latest (KDE5) version for this auth method so it could work fine. Password authentication works normally.

Upd.: nevermind the TDEParts issue, I must have missed Slavek's TDEIO patch.

Please amend as per comment

Seems to work fine, I could connect to a server on another computer on a local network and use the remote files with ease.

URLs with no usernames (hangs)

Seems to work fine here. The urls I tried did not have a username: a dialog opened asking for user/password and that is all that was needed.

Good job!

Seems to work fine here. The urls I tried did not have a username: a dialog opened asking for user/password and that is all that was needed.

Might be a distribution issue then.

I use a static IP as url (like sftp://aaa.bbb.ccc.ddd), not sure if this makes any difference.
I suggest we merge it and ask those users who had problem to test out the new package. If the username-less URL becomes a problem, we will fix it as a separate issue. In any case existing sftp does not work in recent distro apparently, so no much to lose in it.

I agree. Anything to do before I squash and merge?

I plan to squash it all to one commit:
Replaced tdeio-sftp with backported version

Before you merge, one more thing.
I will comment in the code.

Seems to work fine here. The urls I tried did not have a username: a dialog opened asking for user/password and that is all that was needed.

Actually the dialog just showed up, but it took a minute or two for it to open, while with username in URL it connects immendately.

Before you merge, one more thing.
I will comment in the code.

Sorry, ignore previous comment. I was confused about something, but it is fine.

Squash into one commit sounds good. Make sure the message retains a link to the original source that we imported, again to state that it is GPL2 licensed.

Seems to work fine here. The urls I tried did not have a username: a dialog opened asking for user/password and that is all that was needed.

Actually the dialog just showed up, but it took a minute or two for it to open, while with username in URL it connects immendately.

Interesting. I noticed a 2-3 secs delay here, on local network.... maybe something you can try debugging, see if you can understand while it takes longer without specifying the user (unless it is the server side being slow...)

Squashed commit ready. Please have a look at the commit message.

Interesting. I noticed a 2-3 secs delay here, on local network.... maybe something you can try debugging

I think I'll look into it after the merge.

Also, should this be backported or is it only for R14.1.0?

Interesting. I noticed a 2-3 secs delay here, on local network.... maybe something you can try debugging
I think I'll look into it after the merge.

for reference, trying to connect here (sftp://itcsubmit.wustl.edu) shows the dialog in a couple of seconds.

Also, should this be backported or is it only for R14.1.0?

That is a good question. The code is quite different, so in theory it should go into R14.1.0 only. But given that tdeio_sftp does not work in R14.0.12 with recent distros, IMO it makes sense to backport this to R14.0.x too. @SlavekB what do you think?

Looks good

for reference, trying to connect here (sftp://itcsubmit.wustl.edu) shows the dialog in a couple of seconds.

Indeed, so it might be dependent on the SSH/SFTP server after all.

Also, should this be backported or is it only for R14.1.0?

That is a good question. The code is quite different, so in theory it should go into R14.1.0 only. But given that tdeio_sftp does not work in R14.0.12 with recent distros, IMO it makes sense to backport this to R14.0.x too. @SlavekB what do you think?

It does not make any change API/ABI, so there nothing that would fundamentally block the backport to R14.0.x. It is clear that it is definitely solving the problem, so it seems to be a good idea to do backport.

Regarding squash into single commit: There could be useful to leave the initial backport as one commit and subsequent fixes and improvements as another commit. But I have no objection if you want to leave it as one commit.

I did not test functionality, but there are some unexpected changes that relate to translations. It seems that something should be done with it.

It does not make any change API/ABI, so there nothing that would fundamentally block the backport to R14.0.x. It is clear that it is definitely solving the problem, so it seems to be a good idea to do backport.

Sounds good 👍

@SlavekB Translations and ioslave description should be OK now. I split it all into two commits too.

@MicheleC Identation on line 73 fixed ;-)

I have now looked at the versions of libssh in older Debian and it seems that 0.7.x does not contain CMake files => there will probably be better to detect library using pkg-config instead of find_package.

I have now looked at the versions of libssh in older Debian and it seems that 0.7.x does not contain CMake files => there will probably be better to detect library using pkg-config instead of find_package.

Like this?

  pkg_search_module( LIBSSH libssh )
  if( NOT LIBSSH_FOUND )
    tde_message_fatal( "LibSSH is required, but was not found on your system" )
  endif( )

Changing the way of detection libssh is good. Unfortunately, it turned out that there would be necessary other modifications to ensure compatibility with libssh on all supported distributions (Xenial - 0.6.3, Stretch - 0.7.3).

I think about two options:

Pay efforts to solve compatibility with libssh < 0.8.x – ie to 0.6.x to allow use of libssh from older distributions. As a sample there can be sftp from kio-extras 15.12.3 from Ubuntu Xenial.
For Stretch require libssh 0.8.x from Stretch-backports and for Xenial to do our own backport. Both we would add to our build-dependencies.

What is your opinion?

If it does not require too much effort, I think option 1 would be preferable.

Will do.

@SlavekB can you please test the latest changes?

There seems to be the last two calls that need to treat for the libssh API < 0.7.90. See comments below.

To move forward, I have made changes related to compatibility for older versions of libssh, which I mentioned in the previous revise. Therefore, it is now ready to do rebase and we can go to merging.

Tested builds with older and also with current libssh and everything looks ready for merge.

Merged and backported for R14.0.13.

Great, well done @blu.256!

 extern "C"
 {
   int KDE_EXPORT kdemain( int argc, char **argv )
   int kdemain( int argc, char **argv )

   SOURCE sftp.protocol
   DESTINATION ${SERVICES_INSTALL_DIR}
   PO_DIR tdeioslave-desktops
   #PO_DIR tdeioslave-desktops

 moving=true
 Icon=ftp
 Description=A tdeioslave for sftp
 Description=A new tdeioslave for sftp

   /* get the hash */
   ssh_key serverKey;
 #if LIBSSH_VERSION_INT < SSH_VERSION_INT(0, 7, 0)
   if (ssh_get_publickey(mSession, &serverKey) < 0) {

                          <<  mHost << "? " << mConnected << endl;
       /* write the known_hosts file */
       kdDebug(TDEIO_SFTP_DB) << "Adding server to known_hosts file." << endl;
       if (ssh_session_update_known_hosts(mSession) != SSH_OK) {

				`##### create translation templates ##############`

				`tde_l10n_create_template( "tdeio_sftp" )`

KIO-SFTP backport #279

What works

What does not work

Needs testing

Other issues

Reviewers