Making FileJob behave consistently.
ClosedPublic

Authored by feverfew on Aug 15 2019, 9:08 PM.

Details

Summary

This patch does the following:

  1. Makes sure the close() signal is actually emitted when close() is called.
  2. Documents the FileJob functions more accurately, and ensures the file slave acts similarly to the two other slaves that implement these functions (smb/sftp).
  3. Fixes an issue when purposefully reading 0 bytes.
  4. Fixes a bug where finished() is called after error().
Test Plan

The application I am developing on that depends on FileJob now successfully receives the close() signal when required and does not experience the bug
mentioned when reading 0 bytes.
Existing read/write/seek functionality is not broken.
Tests also pass.

Diff Detail

Repository
R241 KIO
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 15 2019, 9:08 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 15 2019, 9:08 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
feverfew requested review of this revision.Aug 15 2019, 9:08 PM
feverfew edited the summary of this revision. (Show Details)Aug 15 2019, 9:10 PM
feverfew edited the summary of this revision. (Show Details)Aug 15 2019, 9:40 PM
chinmoyr requested changes to this revision.Aug 16 2019, 8:19 AM
chinmoyr added inline comments.
src/core/filejob.cpp
180

Nitpick; spacing gotcha

src/core/filejob.h
48–61

Document the methods in SlaveBase as well.

src/core/slavebase.h
107

@since 5.62

src/core/slaveinterface.h
90

Nitpick; add a ,
Next time someone adds an enum it will change only one line.

src/ioslaves/file/file.cpp
520–528

A loop is required here. The docs don't really specify anything about reading data in one go.

582

missing a finished() here

src/ioslaves/file/file.h
117

Nitpick; why not just closeFile() ?

This revision now requires changes to proceed.Aug 16 2019, 8:19 AM
feverfew updated this revision to Diff 63861.Fri, Aug 16, 1:33 PM

Addressing comments

feverfew added inline comments.Fri, Aug 16, 1:37 PM
src/ioslaves/file/file.cpp
520–528

Exactly. The docs weren't specific at all. smb/sftp would read once, and return the result. the file slave would read until size was reached (or EOD). I decided to just convert file's behaviour to what smb/sftp do and updated the docs accordingly, for ease of use (and so that if new slaves implement KIO::open() they'd know how to implement the functions. Of course, I could do the converse, and change smb/sftp to behave like the file slave when reading/writing, but I can leave that up to debate, if others want to chime in.

feverfew added inline comments.Fri, Aug 16, 3:48 PM
src/ioslaves/file/file.h
117

That's how sftp does it, so for consistency.

feverfew added inline comments.Fri, Aug 16, 5:44 PM
src/ioslaves/file/file.cpp
550

Flushing on every write IMO is a bit expensive, but I don't like the fact that close() can't report errors and so we have silent data loss. Maybe we should implement a flush method in FileJob. It only really makes sense for the file slave, for sftp/smb it will be a noop... Thoughts?

feverfew marked 4 inline comments as done.Fri, Aug 16, 5:47 PM
feverfew added inline comments.
src/core/filejob.h
48–61

I've used @see to help me out. Is that enough? I don't want to duplicate the docs, especially if someone changes it and forgets to change the other...

feverfew updated this revision to Diff 64051.Mon, Aug 19, 3:55 PM

Making comments clearer

dfaure requested changes to this revision.Sat, Aug 24, 10:10 AM

Unittests are missing.

src/core/filejob.h
110–114

emitted -> Emitted

112

Why would someone request reading 0 bytes? That doesn't seem sensible to me.

144

s/;/./

185

(OK -- that's supposed to be a known fact from the base class KJob, but I'm not opposed to saying it again ;)

This revision now requires changes to proceed.Sat, Aug 24, 10:10 AM
feverfew added inline comments.Sun, Aug 25, 8:11 PM
src/core/filejob.h
112

Well it can happen and this is technically more correct than saying that an empty QByteArray() is always EOD. In fact, the one of the motivations for this patch was that for KIOFuse, we do read 0 bytes in case the user truncates to 0, otherwise we'd have to have a workaround if it didn't work (for example, before this patch, we'd get both an empty QByteArray() and the error signal...).

feverfew updated this revision to Diff 64672.Mon, Aug 26, 2:36 PM
  • Fixing typos
  • Adding test for reading of 0 bytes and asserting that the close signal is

actually emitted.

feverfew marked 3 inline comments as done.Mon, Aug 26, 2:36 PM
feverfew updated this revision to Diff 64673.Mon, Aug 26, 2:43 PM
  • Removing flush on write
feverfew marked an inline comment as done.Mon, Aug 26, 2:44 PM
dfaure accepted this revision.Wed, Aug 28, 7:29 AM

I didn't know the code of that unittest. It could really use some modernizing (using job->exec() instead of enterLoop, using lambdas instead of numbered slots...), but that's for a separate commit anyway, this one keeps consistency (in ugliness :-) ).
Thanks for the added test.

src/core/filejob.h
48–61

Makes sense to me.

chinmoyr accepted this revision.Wed, Aug 28, 2:39 PM
This revision is now accepted and ready to land.Wed, Aug 28, 2:39 PM
This revision was automatically updated to reflect the committed changes.