D24887: WIP slave command behavior assertion system
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
After much delay some replies but there are yet more questions in
here
INLINE COMMENTS
> commands_p.h:27
> {
> +#pragma message "this is awkward since this causes symbols on the public KIO
> entity"
> +Q_NAMESPACE
parse error
> slavebase.cpp:96
> +
> +#pragma message "pretty sure this isn't safe WRT static init order"
> +/**
why? does another global ctor use it?
It's safe. It's just costly -- happens in each and every process that links to
KIO.
Maybe this should be all in NDEBUG anyhow?
> slavebase.cpp:128
> +// * position lacks any documentation
> +// * trash listdir calls totalsize() (via listroot)
> +// * applications listdir calls totalsize()
In order to get progress information when listing very large directories, the
idea used to be "emit totalSize(number of files) from listDir()".
Then kdelibs4 commit 23ba25b6505dc7
changed the logic for the batching in KIO and this information is no longer
necessary.
I just removed outdated docu from slavebase.h in commit 2261f2045c0a
(the kdelibs4 commit removed most of it).
I'm wondering how ListJob can emit percent though, as comments over there say,
without knowing about the totalsize. Something seems amiss here.
I can see how batching (based on time) doesn't need to know the total number of
items, but progress information surely does...
And in fact AFAICS this information does end up in ListJob, and is used there
to calculate progress information.
So I was wrong in today's commit, and that kdelibs4 commit was wrong in
removing that docu too.
This is useful, and slaves should emit it.
The porting to KJob turned a simple int into something explicitly called
"Bytes" though, so it looks even more like a hack than it did before (see
SimpleJobPrivate::slotTotalSize, also called from ListJob).
ListJob should have its own slot, and use Files instead of Bytes.
kio_file should also emit totalSize, 1) for slow network-mounted directories,
2) so that all this can be unittested.
One question, 3 TODOs, this is going to be a long patch to review ;-)
> slavebase.cpp:131
> +// * settings listdir calls totalsize()
> +// * desktop listdir calls processedsize()
> +// * remote listDir calls totalSize()
Sounds wrong (i.e. but g in kio_desktop), listjob takes care of that.
> slavebase.cpp:137
> +#pragma message "deal with not codified expectations"
> +// * during get mimetype should be emitted before data (req:
> mimetype mustn't be after data?)
> +// * canResume has two variants that should be called at the
> beginning of get/put (req: mustn't be after data/written?)
Right. This allows to pick the app based on mimetype, and it will get all the
data.
> slavebase.cpp:139
> +// * canResume has two variants that should be called at the
> beginning of get/put (req: mustn't be after data/written?)
> +// * listDir should call listEntry --> not really something to warn
> about, a dir may be empty
> +// * stat should call statEntry --> unless error?
right
> slavebase.cpp:140
> +// * listDir should call listEntry --> not really something to warn
> about, a dir may be empty
> +// * stat should call statEntry --> unless error?
> +
right, on error it won't call statEntry
> slavebase.cpp:185
> +#pragma message "deal with commands I don't know expectations for"
> +case CMD_SLAVE_CONNECT:
> +case CMD_SLAVE_HOLD:
That's internal, doesn't get to SlaveBase.
> slavebase.cpp:186
> +case CMD_SLAVE_CONNECT:
> +case CMD_SLAVE_HOLD:
> +case CMD_NONE:
same
> slavebase.cpp:187
> +case CMD_SLAVE_HOLD:
> +case CMD_NONE:
> +case CMD_TESTDIR:
That's a reply (slave->app). No expectations, since it's the other way around.
> slavebase.cpp:188
> +case CMD_NONE:
> +case CMD_TESTDIR:
> +case CMD_SPECIAL:
unused, should be removed in KF6
> slavebase.cpp:189
> +case CMD_TESTDIR:
> +case CMD_SPECIAL:
> +case CMD_REPARSECONFIGURATION:
depends on the subcommand, as you documented in slavebase.h
> slavebase.cpp:190
> +case CMD_SPECIAL:
> +case CMD_REPARSECONFIGURATION:
> +case CMD_META_DATA:
fire-and-forget command app->slave, no expectations.
> slavebase.cpp:191
> +case CMD_REPARSECONFIGURATION:
> +case CMD_META_DATA:
> +case CMD_SUBURL:
handled internally in slavebase.cpp, doesn't get to the SlaveBase subclass
> slavebase.cpp:192
> +case CMD_META_DATA:
> +case CMD_SUBURL:
> +case CMD_MESSAGEBOXANSWER:
Obsolete. There's much cleanup to be done around that stuff in KF6. Unless
someone feels like
D24887: WIP slave command behavior assertion system
sitter added a comment. About the list of unexpected signals in forCommand(): It looks that at least http, ftp, and sftp do call connected() as part of various commands. e.g. all do it during get(). Does that make sense? connected() refers to openConnection() in its documentation, and openConnection() says the slave is operating in connection-oriented mode when called, so if openConnection() puts it into connection-oriented mode then not having had a call to openConnection() means it shouldn't be operating in connection-oriented mode. I have no additional understanding besides what the documentation tells me and so it would seem to me that connected() shouldn't be called anywhere but openConnection(). At the same time the documentation for openConnection() does have the neat qualifier forced ` * Opens the connection (forced)` So I guess my questions are : - what does connection-oriented mean exactly? - what exactly is a forced connection open? - how is a forced open different from a casual open? - should connected() really be called all over the place? - if so, does the client software actually need to do something based on the signal or is it simply a case of "it does no harm, so emitting it unconditionally is easier than not"? Based on our specific requirements here the expectation class needs some rejiggering to differentiate state violations (e.g. openConnection() neither calling connected() nor error()) from unexpected updates (connected() during get()) from useless updates (listEntry() during stat()). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24887 To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24887: WIP slave command behavior assertion system
sitter created this revision. sitter added a reviewer: dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. sitter requested review of this revision. REVISION SUMMARY This implements a simple expectation system covering slave commands. At its heart sits the CommandExpectation, which contains the expectations of a given command. Currently the sole expectation is about signal emission notably a whitelist of signals that any given command may emit during its emission. To assist this there's a new Signal enum which is used to refer to the signals. Signals by default are considered exclusive and repeatedly emitting the same signal will result in assertion failures. Multi-emission signals such as data/written/processedsize are of course excluded from this. All signal implementation in SlaveBase should call VERIFY_CALL to verify the validity of the signal call at call-time (notably: can this signal be called during this Command and may it be called 1 or N times). After the command finished the pre-existing verifyState function has been refitted for the new expectation tech. - slavebase.h now tries to document expectations WRT signals - makes KIO to a Q_NAMESPACE which kinda exposes unintended symbols I guess e.g. staticMetaObject. this does enable us to convert the enum values to strings via QMetaMethod but has no additional purpose beyond that. - m_state/States are no more. m_state was solely used to track whether error/finished was called and then assert expectations. this is replaced by m_receivedSignals which tracks any number of signals - verifyErrorFinishedNotCalled and verfifyFinishedNotCalled have been merged into verifyCall which is now the one-place-stop to get in-call verification (i.e signals call verifyCall and verifyCall makes sure that signal should even have been called) - verifyState has been reworked to be applicable to all expectations todo: - I am not 100% on most of the expectations - do we really want Q_NAMESPACE on KIO? we could replace this with enumToString() functions switching over the values manually. seeing as I am lazy I'd rather use the moc generated data though - conceivably an empty expectation should mean everything is allowed, currently it's meant to mean nothing is allowed as that is rather more handy to make sure the expectations themselves are in fact correct and complete - warning todos - stray debugs TEST PLAN casually use all the slaves REPOSITORY R241 KIO BRANCH assert-file REVISION DETAIL https://phabricator.kde.org/D24887 AFFECTED FILES src/core/CMakeLists.txt src/core/commands_p.h src/core/slavebase.cpp src/core/slavebase.h To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
