D23344: assert slave command finality
bcooksley removed a subscriber: fsitter. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23344 To: sitter, dfaure Cc: bcooksley, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns, fsitter
D23344: assert slave command finality
fsitter added a comment. You think sending your minions to insult me and then disabling my account will solve the issue? what will you do next? Disable registration so no one points out your hypocrisy? Which overlord made the decision to disable my account and for what? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23344 To: sitter, dfaure Cc: fsitter, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
fsitter added a comment. You think sending your minions to insult me and then disabling my account will solve the issue? what will you do next? Disable registration so no one points out your hypocrisy? Which overlord made the decision to disable my account and for what? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23344 To: sitter, dfaure Cc: fsitter, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
dfaure added a comment. LOL, so even the joke (about you writing buggy code) was buggy, good one ;) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23344 To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
This revision was automatically updated to reflect the committed changes. Closed by commit R241:830c56744f2b: assert slave command finality (authored by sitter). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23344?vs=64826=64928 REVISION DETAIL https://phabricator.kde.org/D23344 AFFECTED FILES CMakeLists.txt src/core/config-kiocore.h.cmake src/core/slavebase.cpp To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
sitter added a comment. Shoes! I meant shoes! REPOSITORY R241 KIO BRANCH assert REVISION DETAIL https://phabricator.kde.org/D23344 To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. You put on shows? On TV? :-) REPOSITORY R241 KIO BRANCH assert REVISION DETAIL https://phabricator.kde.org/D23344 To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
sitter updated this revision to Diff 64826. sitter added a comment. some days one has to wonder how I manage to put on shows. fix warning branch as per David's comments to actually do nothing when the cond was matched REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23344?vs=64744=64826 BRANCH assert REVISION DETAIL https://phabricator.kde.org/D23344 AFFECTED FILES CMakeLists.txt src/core/config-kiocore.h.cmake src/core/slavebase.cpp To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> slavebase.cpp:67
> +#else
> +#define KIO_STATE_ASSERT(cond, where, what) qCWarning(KIO_CORE) << what
> +#endif
Don't you mean if(!cond) { qCWarning... } ? Otherwise the warning will be
shown every time, no matter what the condition says.
Or more precisely,
do { if (!(cond)) qCWarning(KIO_CORE) << what; } while (false)
for the usual brace nesting problem.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23344
To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
sitter updated this revision to Diff 64744. sitter added a comment. - wrap in custom assert defines that either assert or qwarn based on a cmake option - new cmake option KIO_ASSERT_SLAVE_STATES enables the asserts. the option is only on by default when run on jenkins - fix a bunch of typos the long term plan here is still to always enable the assertions (conditional on build type anyway), to not break everyones systems the cmake option allows a few brave souls to opt into assertion. also by running jenkins slaves with assertions we'll have additional chances of finding bugs there REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23344?vs=64294=64744 BRANCH assert REVISION DETAIL https://phabricator.kde.org/D23344 AFFECTED FILES CMakeLists.txt src/core/config-kiocore.h.cmake src/core/slavebase.cpp To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
sitter added a comment. Yeah, it's certainly a very aggressive change. I also think we need this to be outside kdeinit5, otherwise unit tests won't crash since they generally should fork directly. That being said, as a first step maybe we can have this enabled on the CI and see if existing unit tests start failing. Additionally here's two "transient" options I'd propose: - only enable the asserts for Debug builds **from git**. that way if stuff crashes a lot, the user/developer can expect a quick turn around on the fix - give an env var or cmake option that can opt out of the assertions but still run in debug mode? (the cmake option seems a bit pointless, if you run a Debug build you should get debuggy behavior IMHO). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23344 To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
dfaure added a comment.
I see those warnings often, so I'm a bit worried that this will make
kioslaves quite crashy for KDE developers for a while, including those unable
to fix those issues.
At least we need to fix known issues first, like kio_ftp.
How about we both (and ZaWertun) run with this for a while (and fix things)
before merging it upstream?
Note that we could also just export QT_FATAL_WARNINGS=1 before kdeinit5 :-)
INLINE COMMENTS
> slavebase.cpp:202
> +// Force the command into finished state. We'll not reach this for
> Debug builds
> +// that fail the assertion. For Releas builds we'll have made sure
> that the
> +// command is actually finished after the verfication regardless of
> what
typo: Release
> slavebase.cpp:203
> +// that fail the assertion. For Releas builds we'll have made sure
> that the
> +// command is actually finished after the verfication regardless of
> what
> +// the slave did.
typo: verification
> slavebase.cpp:480
> + Q_FUNC_INFO,
> + qUtf8Printable(QStringLiteral("error() was called, but it's
> not supposed to! Please fix the %2 KIO slave")
> + .arg(QCoreApplication::applicationName(;
should be %1, not %2
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23344
To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23344: assert slave command finality
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 changes existing qwarnings in state verification logic to be assertive in order to crash misbehaving slaves so git users can report bugs about them and we can fix them. release builds are not affected and will continue to not crash albeit without warnings. I am not sure it's worth the effort to assert and qwarn. additionally when no finality was expected we no longer verify this after the fact but instead at the time finished/error gets called. this, combined with the assert aborting, has the advantage that we'll see the actual call chain that resulted in the unexpected finality call. it's still not ideal since we'd also want to know the previous caller but unfortunately we have no way to get that reliable (e.g. backtrace() is woefully incapable of resolving symbols from the slave - probably because it is a module). lastly, force a misbehaving slave into finished state when it should have been finished or error'd but didn't. this prevents slaves getting stuck ad infinitum on account of them not finshing. TEST PLAN misehaving ftp slave now crashes REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D23344 AFFECTED FILES src/core/slavebase.cpp To: sitter, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
