smb: port to Result system to force serialization of error/finish condition
Needs RevisionPublic

Authored by sitter on Apr 17 2020, 10:39 AM.

Details

Reviewers
dfaure
Summary

the Result system was originally introduced to the FTP slave and now also
makes an appearance in the SMB slave. the system introduces a separation
between logic and fronting API class to more tightly control when state
changing calls (finished()/error()) are made. since these calls may only
be made once during a given command multiple calls are at the very least
indicative of bad code and at worst cause severe state confusion for the
slavebase that it won't be able to recover from, rendering the slave
instance broken.

in the internal class Results are returned whenever an error can appear and
these Results must be handled in some form. the only way to effectively
produce user visible errors is to forward results up the call chain.

this is also introducing scopeguards for file descriptors to simplify
error condition returning (i.e. not having to worry about closing the fd
manually). as a result we now require Qt 5.12 (which we do via KF5.66
anyway, albeit implicitly)

Test Plan
  • copy, lots and lots of copy
  • rename
  • delete

Diff Detail

Repository
R320 KIO Extras
Branch
smb-result
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25368
Build 25386: arc lint + arc unit
sitter created this revision.Apr 17 2020, 10:39 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptApr 17 2020, 10:39 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Apr 17 2020, 10:39 AM
dfaure requested changes to this revision.Apr 18 2020, 3:40 PM
dfaure added inline comments.
smb/kio_smb.cpp
162

The KIO::SimpleJob needs to finish....

kio_file's special() -- which is where it all started -- does emit finish().

I'm pretty sure they should all do.

smb/kio_smb.h
96

The class is simple enough, but I'm wondering if at some point we might want to provide it in KIO [as is, not with a d pointer].

269

parse error?

276

the default value here is confusing and useless, since this member actually gets initialized by the constructor.

This revision now requires changes to proceed.Apr 18 2020, 3:40 PM
meven added a subscriber: meven.Apr 19 2020, 6:13 AM
meven added inline comments.
smb/kio_smb_browse.cpp
184

Empty else clause leftover

smb/kio_smb_file.cpp
169

add {}

meven added inline comments.Apr 19 2020, 6:15 AM
smb/kio_smb.h
96

KF6 ?
Or once we want to use it in KIO ?

dfaure added inline comments.Apr 19 2020, 9:05 AM
smb/kio_smb.h
96

Whenever we port a 3rd slave to it ;-)

sitter added inline comments.Apr 20 2020, 8:20 AM
smb/kio_smb.h
96

Sure, if you think it's solid enough from an API POV.

I was thinking that we should amend the slavebase API for KF6 in general. Instead of having error/finished/opened all functions on an API level should return a Result and the slave loop would emit the relevant signal based on the Result. IOW: what currently happens in the derived SlaveBases actually ought to be KIO-internal.

That would then also allow us to get rid of the two-class split again. And the "fronting" class is actually a much bigger concern than Result to me. The call finalization logic is 100% code copy and so very easy to get wrong (e.g. sftp's special() not finishing when in fact it should).

269

That line only moved, I am not quite sure what it is meant to tell us though. The header is and was quite the mess.

dfaure added inline comments.Apr 25 2020, 12:41 PM
smb/kio_smb.h
96

Excellent idea, but why wait for KF6?

We could implement a subclass of SlaveBase, in KIO, let's call it SlaveBaseV2, which

  • Reimplements all the virtual methods from SlaveBase, in a private section
  • Defines a new of virtual methods, called differently somehow
  • Implements the signal emission based on the return value of those new virtual methods.

This allows for incremental porting of the slaves, instead of a big KF6 breakage.
And the V2 class can be developed in a single slave first, before having to freeze its API.

meven added inline comments.Apr 25 2020, 1:53 PM
smb/kio_smb.h
96

Sounds like a nice idea.

File ioslave would be the best one to work on first IMO, since it is the one the most complete, used and tested, albeit one of the biggest probably.

Let's make a task !

But this diff does not need to wait for this.