Avoid calling finished() after error()"
ClosedPublic

Authored by barchiesi on Jul 14 2019, 10:23 PM.

Details

Summary

By not checking the return of runJob(), the slave might call finished() after already having called error() in handleError().
See T7995 for futher information.

Diff Detail

Repository
R219 KIO GDrive
Branch
arcpatch-D22462
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14072
Build 14090: arc lint + arc unit
barchiesi requested review of this revision.Jul 14 2019, 10:23 PM
barchiesi created this revision.
barchiesi added a project: KIO GDrive.
barchiesi added a subscriber: KIO GDrive.
dvratil requested changes to this revision.Jul 15 2019, 3:13 PM
dvratil added a subscriber: dvratil.

The change around runJob() looks good, but all the changes in logging should go in separately since they are unrelated to the main part of the change.

src/kaccountsmanager.cpp
177 ↗(On Diff #61763)

Looks like a rather unrelated change, it should go in in a separate review (and commit). The refreshToken should get truncated as well, I think.

This revision now requires changes to proceed.Jul 15 2019, 3:13 PM
barchiesi updated this revision to Diff 61815.Jul 15 2019, 6:00 PM

Removed debug output changes.

barchiesi retitled this revision from Avoid calling finished() after error()" and add some debug output. to Avoid calling finished() after error()".Jul 15 2019, 6:00 PM
elvisangelaccio requested changes to this revision.Jul 15 2019, 8:29 PM
elvisangelaccio added inline comments.
src/kio_gdrive.cpp
473

Please move this comment in the runJob() doxygen comment in kio_gdrive.h, rather than duplicating it here n times.

This revision now requires changes to proceed.Jul 15 2019, 8:29 PM
barchiesi updated this revision to Diff 61951.Jul 18 2019, 7:29 AM

Added doxygen for runJob() method.

src/kio_gdrive.cpp
148

This message is a bit cryptic. How about "fileSystemFreeSpace is not supported for gdrive root urls?"

elvisangelaccio accepted this revision.Jul 21 2019, 8:03 PM

Please push to master. Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Jul 21 2019, 8:25 PM
This revision was automatically updated to reflect the committed changes.