D23159: Prevent error() being emitted when purposefully reading 0 bytes
feverfew abandoned this revision. feverfew added inline comments. INLINE COMMENTS > feverfew wrote in 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. 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). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23159 To: feverfew, dfaure, fvogt, chinmoyr Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23159: Prevent error() being emitted when purposefully reading 0 bytes
feverfew added inline comments. INLINE COMMENTS > chinmoyr wrote in 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. 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. > chinmoyr wrote in file.cpp:538 > here `bytes` being negative should be an error..no? bytes is unsigned, so it is not a worry for us. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23159 To: feverfew, dfaure, fvogt, chinmoyr Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23159: Prevent error() being emitted when purposefully reading 0 bytes
chinmoyr added inline comments.
INLINE COMMENTS
> file.cpp:524
> while (true) {
> QByteArray res = mFile->read(bytes);
>
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.
> file.cpp:538
> }
> if (bytes <= 0) {
> break;
here `bytes` being negative should be an error..no?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23159
To: feverfew, dfaure, fvogt, chinmoyr
Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23159: Prevent error() being emitted when purposefully reading 0 bytes
feverfew added inline comments.
INLINE COMMENTS
> apol wrote in 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?
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.
> apol wrote in file.cpp:526
> alternatively this could also check ` && bytes != 0`
Please reread the summary, this would not fix the bug.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23159
To: feverfew, dfaure, fvogt, chinmoyr
Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23159: Prevent error() being emitted when purposefully reading 0 bytes
apol added inline comments.
INLINE COMMENTS
> file.cpp:520
> +// Nothing to do...
> +return data(QByteArray());
> +}
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?
> file.cpp:526
>
> if (!res.isEmpty()) {
> data(res);
alternatively this could also check ` && bytes != 0`
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23159
To: feverfew, dfaure, fvogt, chinmoyr
Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23159: Prevent error() being emitted when purposefully reading 0 bytes
feverfew created this revision. feverfew added reviewers: dfaure, fvogt, chinmoyr. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. feverfew requested review of this revision. REVISION 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. REPOSITORY R241 KIO BRANCH Read0Bug (branched from master) REVISION DETAIL https://phabricator.kde.org/D23159 AFFECTED FILES src/ioslaves/file/file.cpp To: feverfew, dfaure, fvogt, chinmoyr Cc: kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
