Changeset View
Changeset View
Standalone View
Standalone View
src/ioslaves/file/file.cpp
| Show First 20 Lines • Show All 509 Lines • ▼ Show 20 Line(s) | 464 | { | |||
|---|---|---|---|---|---|
| 510 | opened(); | 510 | opened(); | ||
| 511 | } | 511 | } | ||
| 512 | 512 | | |||
| 513 | void FileProtocol::read(KIO::filesize_t bytes) | 513 | void FileProtocol::read(KIO::filesize_t bytes) | ||
| 514 | { | 514 | { | ||
| 515 | // qDebug() << "File::open -- read"; | 515 | // qDebug() << "File::open -- read"; | ||
| 516 | Q_ASSERT(mFile && mFile->isOpen()); | 516 | Q_ASSERT(mFile && mFile->isOpen()); | ||
| 517 | 517 | | |||
| 518 | if (bytes == 0) { | ||||
| 519 | // Nothing to do... | ||||
| 520 | return data(QByteArray()); | ||||
apol: this should be
```
{
Q_EMIT data({});`
return;
}
```
I'm not sure that we'd need an… | |||||
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. feverfew: I('ve not seen any IOSlave use a Q_EMIT macro, any reason why it's necessary here (what's the… | |||||
| 521 | } | ||||
| 522 | | ||||
| 518 | while (true) { | 523 | while (true) { | ||
| 519 | QByteArray res = mFile->read(bytes); | 524 | 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. chinmoyr: This doesn't support error reporting. Why not refactor this loop using read(char *, qint64)? I… | |||||
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. feverfew: I think that would be appropriate for a separate revision which I'm doing. This revision is… | |||||
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). feverfew: Actually I've realised using the other function would allow me to solve this problem much more… | |||||
| 520 | 525 | | |||
| 521 | if (!res.isEmpty()) { | 526 | if (!res.isEmpty()) { | ||
apol: alternatively this could also check ` && bytes != 0` | |||||
feverfew: Please reread the summary, this would not fix the bug. | |||||
| 522 | data(res); | 527 | data(res); | ||
| 523 | bytes -= res.size(); | 528 | bytes -= res.size(); | ||
| 524 | } else { | 529 | } else { | ||
| 525 | // empty array designates eof | 530 | // empty array designates eof | ||
| 526 | data(QByteArray()); | 531 | data(QByteArray()); | ||
| 527 | if (!mFile->atEnd()) { | 532 | if (!mFile->atEnd()) { | ||
| 528 | error(KIO::ERR_CANNOT_READ, mFile->fileName()); | 533 | error(KIO::ERR_CANNOT_READ, mFile->fileName()); | ||
| 529 | close(); | 534 | close(); | ||
| 530 | } | 535 | } | ||
| 531 | break; | 536 | break; | ||
| 532 | } | 537 | } | ||
| 533 | if (bytes <= 0) { | 538 | if (bytes <= 0) { | ||
chinmoyr: here `bytes` being negative should be an error..no? | |||||
feverfew: bytes is unsigned, so it is not a worry for us. | |||||
| 534 | break; | 539 | break; | ||
| 535 | } | 540 | } | ||
| 536 | } | 541 | } | ||
| 537 | } | 542 | } | ||
| 538 | 543 | | |||
| 539 | void FileProtocol::write(const QByteArray &data) | 544 | void FileProtocol::write(const QByteArray &data) | ||
| 540 | { | 545 | { | ||
| 541 | // qDebug() << "File::open -- write"; | 546 | // qDebug() << "File::open -- write"; | ||
| ▲ Show 20 Lines • Show All 1014 Lines • Show Last 20 Lines | |||||
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?