Differential D23159

Prevent error() being emitted when purposefully reading 0 bytes
AbandonedPublic
Actions

Authored by feverfew on Aug 14 2019, 6:15 PM.

Details

Reviewers
dfaure
fvogt
chinmoyr
Summary

Currently when 0 is passed as a parameter, if the file to be read is non-empty both data and error signals
are emitted. This behaviour is incorrect, as there is no error. This patch makes sure that only data is
emitted.

Test Plan

The application I'm developing that hit this bug now no longer hits this issue.

Diff Detail

Repository
R241 KIO
Branch
Read0Bug (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15108
Build 15126: arc lint + arc unit
feverfew created this revision.Aug 14 2019, 6:15 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 14 2019, 6:15 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
feverfew requested review of this revision.Aug 14 2019, 6:15 PM
apol added a subscriber: apol.Aug 14 2019, 11:20 PM
apol added inline comments.
src/ioslaves/file/file.cpp
520

this should be

{
    Q_EMIT data({});`
    return;
}

I'm not sure that we'd need an empty data emitted then. Are you trying to fix a specific bug that triggers this?

526

alternatively this could also check && bytes != 0

feverfew added inline comments.Aug 15 2019, 12:35 AM
src/ioslaves/file/file.cpp
520

I('ve not seen any IOSlave use a Q_EMIT macro, any reason why it's necessary here (what's the change in the behaviour).

Please re-read the summary, carefully, the error is subtle.

526

Please reread the summary, this would not fix the bug.

chinmoyr added inline comments.Aug 15 2019, 6:13 PM
src/ioslaves/file/file.cpp
524

This doesn't support error reporting. Why not refactor this loop using read(char *, qint64)? I believe you can accommodate your changes inside the loop cleanly then.

538

here bytes being negative should be an error..no?

feverfew added inline comments.Aug 15 2019, 6:53 PM
src/ioslaves/file/file.cpp
524

I think that would be appropriate for a separate revision which I'm doing. This revision is only to handle the case outlined in the summary.

538

bytes is unsigned, so it is not a worry for us.

feverfew abandoned this revision.Aug 15 2019, 7:15 PM
feverfew added inline comments.
src/ioslaves/file/file.cpp
524

Actually I've realised using the other function would allow me to solve this problem much more cleanly. I'll abandon this revision and upload a new patch that addresses this problem (and others).

Revision Contents
Changeset List

PathPackages
Msrc/ioslaves/file/file.cpp (5 lines)

Diff 63753

View Options

src/ioslaves/file/file.cpp

Loading...