Fixing implementation of FileJob interface in smb/sftp slaves
ClosedPublic

Authored by feverfew on Aug 16 2019, 5:02 PM.

Details

Summary

This patch does the following:

  1. Fixing instances of finished() being called after error() in smb slave.
  1. Changing behaviour of the sftp/smb slave to conform to what is stated in D23194. In particular, the sftp slave should not call finished() in the open()/read()/write()/seek() methods.
Test Plan

It compiles...

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.
feverfew created this revision.Aug 16 2019, 5:02 PM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptAug 16 2019, 5:02 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
feverfew requested review of this revision.Aug 16 2019, 5:02 PM
feverfew retitled this revision from Fixing finished() called after error() in smb slave. Do not call finished() in open/read/write/seek operations to Fixing implementation of FileJob interface in smb/sftp slaves.Aug 16 2019, 5:07 PM
feverfew edited the summary of this revision. (Show Details)
feverfew edited the test plan for this revision. (Show Details)
feverfew added reviewers: chinmoyr, fvogt.
feverfew updated this revision to Diff 63876.Aug 16 2019, 5:11 PM

Unnecessary min version upgraded

Not knowing the background here at a glance I would argue that SlaveBase in KIO should be getting state verification on all of this,.

Not knowing the background here at a glance I would argue that SlaveBase in KIO should be getting state verification on all of this,.

Sorry, I'm not sure what you mean by "state verification" in this case and how it relates to SlaveBase.

SlaveBase (which the slaves derive from) runs a command loop, out of this command loop come the actual calls to the API functions. So, in said command loop we can verify which state the slave is in before and more importantly after any API call. Specifically about the defects you are repairing here I'd guess that none of the commands running on an open connection must result in the finished state. We already do this for (some) of the other commands.

dfaure accepted this revision.Aug 28 2019, 7:40 AM

Well I guess these fixes came to be exactly *because* SlaveBase warns about finished() being called when it shouldn't :-)

This revision is now accepted and ready to land.Aug 28 2019, 7:40 AM

OK, I'm wrong, it doesn't check that at the moment. But IIRC your slavebase patch does that meanwhile ;)

@dfaure this is actually a different beast altogether, the current state validation is only run on the API for all slaves, seek/read/write are run in a completely different loop which I see isn't being verified at all. I'll add it in a separate diff once the assertion is in. Should be fairly trivial. As I understand it open/read/write/seek must only ever call error() or their specific "send" function (opened()/data()/written()/position()) but never finished. Close meanwhile must reach finality (finished or error).

This comment was removed by bcooksley.
This comment was removed by bcooksley.
This comment was removed by bcooksley.
bcooksley removed a subscriber: fsitter.
This revision was automatically updated to reflect the committed changes.