One more small thing: it's better to replace sCopyFile in call to error with something like dest.path(),
to display server path rather than local source.

Patch was made against version 5.60.0 (rev 8513ca9).
Sorry that I didn't mention it earlier.

In D22528#497803, @sitter wrote:

Looks good to me. The slave indeed must issue an exit state there.

What email address would you like to have associated with the git commit?

zawertun@gmail.com

Thanks!

Please wait.

This is a partial revert of my earlier commit to this file -- not exactly, but it feels like there is a bit of a mess regarding the question of whether error() was already emitted or not.
I made the assumption that statusServerError meant error was already emitted (by earlier function calls) but it now seems this assumption was wrong.
But then it feels like we're playing ping-pong with this code in a way that will never be satisfactory, if various error cases have or have not already emitted error() at this point.
This needs further research and cleanups.

Well, presumably we're better off with this patch in than out, so actually if you want to commit, I (or someone else...) can do the research either way.

Does anyone know which methods are supposed to call error() and which methods are supposed to just set iError and let the caller do it? Right now it seems to be a bit of a mess...

Hm. I've had a quick look and I think the intent was that only the slavebase overridden functions call error or finished. So technically all internal functions that can have an error need to set an iError or return one so that the "public" function can then issue error() as needed. This seems to not very well enforced and probably never was, in fact there's a comment in the .h about this very fact

// ------------------------------------------------------------------------
// All the methods named ftpXyz are lowlevel methods that are not exported.
// The implement functionality used by the public high-level methods. Some
// low-level methods still use error() to emit errors. This behaviour is not
// recommended - please return a boolean status or an error code instead!
// ------------------------------------------------------------------------

What I think may be best to resolve this is to split the class in two.

The "public" class only has overrides of SlaveBase. It's functions are the only ones that may issue error() or finished().
The "private" class has all the supporting functions and has no access to SlaveBase so it cannot issue error or finished.
This should be a very durable solution as even in the future no one will accidentally add another call to error or finished in one of the internal functions since they are in a different object and all error handling must be done via iError or some other system.

In fact, I am thinking this is a case where one could just use exceptions. The private class could throw a simple exception on errors and in the public class we'd need nothing more than a try{}catch in all the public functions.

Something like this https://phabricator.kde.org/P439

I like very much your proposal about using two classes to avoid reintroducing problems in the future.

I like a lot less the use of exceptions, generally speaking... one uncaught exception and it's the end of the world.
But OK I see that in this case, there is a clear layer where all exceptions are caught, so this typical pitfall is avoided. I guess I can make an exception to my rule about exceptions, haha :-)

Bug still present in plasma 5.16.4.

Wrong. Still present in KF frameworks 5.61:

KIO’s FTP connection feature is now more tolerant of broken FTP server implementations (Enes Selim, KDE Frameworks 5.61)

(from https://pointieststick.com/2019/07/28/kde-usability-productivity-week-81/)

@dfaure are we ok to land this for now?

I can't say if this is better in or out, since it's broken in any case. We're playing ping-pong with broken code, but sure, whatever, if you think it's better with this commit than with the earlier equally broken two versions, I'm not objecting.

Is your work on a proper two-classes solution not ready?

In D22528#507373, @dfaure wrote:

Is your work on a proper two-classes solution not ready?

Nope, I haven't made any progress on it beyond what I posted. I briefly looked at a testing setup to make sure stuff doesn't regress but that got me discouraged again :|

Just found another bug on the KDE bugzilla related to this: https://bugs.kde.org/show_bug.cgi?id=410357.

but sure, whatever, if you think it's better with this commit than with the earlier equally broken two versions, I'm not objecting.

@sitter so what do you think, should we land this in the meantime?

Well, we *could* land it, I am also not sure if that is a net-improvement though. As David said we are playing ping-pong with bugs and all of them are symptoms of the larger issue that error handling is done inconsistently, which in turn is why fixing one bug causes another, as has happened here. This will keep on happening until we address the root problem. If you want to help with facilitating that what I really want is a quick and dirty integration test system built for ftp so we can check such high level functionality as "can I overwrite a file".

[spam comment removed by sysadmin]