Commit 1df61748 authored by Harald Sitter's avatar Harald Sitter 🌈
Browse files

sftp: break large writes into multiple requests

Summary:
servers have arbitrary limits that we should stay below. to ensure this
happens use MAX_XFER_BUF_SIZE as maximum size per request. if we read
data large than that, break it apart into multiple requests

(I am mostly guessing here, the rfc doesn't seem to impose any size
 constraint on the write requests themselves, so probably packet
 constraints apply by default)

BUG: 404890
FIXED-IN: 20.04.2

Test Plan:
- fallocate -l 128M file
- sftp to localhost
- copy file from one dir to another
- compare checksums match

- open xls file (via kio-fuse)
- change
- save
- close
- open again
- changes are still there

Reviewers: ngraham, meven, feverfew

Reviewed By: ngraham, meven, feverfew

Subscribers: meven, feverfew, kde-frameworks-devel, kfm-devel

Tags: #dolphin, #frameworks

Differential Revision: https://phabricator.kde.org/D29634
parent c6379d37
......@@ -55,6 +55,22 @@ using namespace std::experimental::filesystem;
// How big should each data packet be? Definitely not bigger than 64kb or
// you will overflow the 2 byte size variable in a sftp packet.
// TODO: investigate what size we should have and consider changing.
// this seems too large...
// from the RFC:
// The maximum size of a packet is in practise determined by the client
// (the maximum size of read or write requests that it sends, plus a few
// bytes of packet overhead). All servers SHOULD support packets of at
// least 34000 bytes (where the packet size refers to the full length,
// including the header above). This should allow for reads and writes of
// at most 32768 bytes.
// In practise that means we can assume that the server supports 32kb,
// it may be more or it could be less. Since there's not really a system in place to
// figure out the maximum (and at least openssh arbitrarily resets the entire
// session if it finds a packet that is too large
// [https://bugs.kde.org/show_bug.cgi?id=404890]) we ought to be more conservative!
// At the same time there's no bug reports about the 60k requests being too large so
// perhaps all popular servers effectively support at least 64k.
#define MAX_XFER_BUF_SIZE (60 * 1024)
#define KSFTP_ISDIR(sb) (sb->type == SSH_FILEXFER_TYPE_DIRECTORY)
......@@ -1436,14 +1452,13 @@ Result SFTPInternal::write(const QByteArray &data)
Q_ASSERT(mOpenFile != nullptr);
ssize_t bytesWritten = sftp_write(mOpenFile, data.data(), data.size());
if (bytesWritten < 0) {
if (!sftpWrite(mOpenFile, data, nullptr)) {
qCDebug(KIO_SFTP_LOG) << "Could not write to " << mOpenUrl;
close();
return Result::fail(KIO::ERR_CANNOT_WRITE, mOpenUrl.toDisplayString());
}
q->written(bytesWritten);
q->written(data.size());
return Result::pass();
}
......@@ -1805,17 +1820,15 @@ Result SFTPInternal::sftpPut(const QUrl &url, int permissions, JobFlags flags, i
break;
}
ssize_t bytesWritten = sftp_write(file, buffer.data(), buffer.size());
if (bytesWritten < 0) {
qCDebug(KIO_SFTP_LOG) << "Failed to sftp_write" << buffer.size() << "bytes."
<< "- Already written: " << totalBytesSent
<< "- SFTP error:" << sftp_get_error(mSftp)
<< "- SSH error:" << ssh_get_error_code(mSession);
bool success = sftpWrite(file, buffer, [this,&totalBytesSent](int bytes) {
totalBytesSent += bytes;
q->processedSize(totalBytesSent);
});
if (!success) {
qCDebug(KIO_SFTP_LOG) << "totalBytesSent at error:" << totalBytesSent;
errorCode = KIO::ERR_CANNOT_WRITE;
result = -1;
} else {
totalBytesSent += bytesWritten;
q->processedSize(totalBytesSent);
break;
}
} // result
} while (result > 0);
......@@ -1909,6 +1922,38 @@ Result SFTPInternal::sftpPut(const QUrl &url, int permissions, JobFlags flags, i
return Result::pass();
}
bool SFTPInternal::sftpWrite(sftp_file file,
const QByteArray &buffer,
const std::function<void(int bytes)> &onWritten)
{
// TODO: enqueue requests.
// Similarly to reading we may enqueue a number of requests to mitigate the
// network overhead and speed up things. Serial iteration gives super poor
// performance.
// Break up into multiple requests in case a single request would be too large.
// Servers can impose an arbitrary size limit that we don't want to hit.
// https://bugs.kde.org/show_bug.cgi?id=404890
off_t offset = 0;
while (offset < buffer.size()) {
const auto length = qMin<int>(MAX_XFER_BUF_SIZE, buffer.size() - offset);
ssize_t bytesWritten = sftp_write(file, buffer.data() + offset, length);
if (bytesWritten < 0) {
qCDebug(KIO_SFTP_LOG) << "Failed to sftp_write" << length << "bytes."
<< "- Already written (for this call):" << offset
<< "- Return of sftp_write:" << bytesWritten
<< "- SFTP error:" << sftp_get_error(mSftp)
<< "- SSH error:" << ssh_get_error_code(mSession)
<< "- SSH errorString:" << ssh_get_error(mSession);
return false;
} else if (onWritten) {
onWritten(bytesWritten);
}
offset += bytesWritten;
}
return true;
}
Result SFTPInternal::copy(const QUrl &src, const QUrl &dest, int permissions, KIO::JobFlags flags)
{
qCDebug(KIO_SFTP_LOG) << src << " -> " << dest << " , permissions = " << QString::number(permissions)
......
......@@ -223,6 +223,14 @@ private: // private methods
Q_REQUIRED_RESULT Result sftpGet(const QUrl &url, KIO::fileoffset_t offset = -1, int fd = -1);
Q_REQUIRED_RESULT Result sftpPut(const QUrl &url, int permissions, KIO::JobFlags flags, int fd = -1);
/**
* sftp_write wrapper breaking buffer into suitable pieces
* \param onWritten acts as callback, for each written block.
*/
Q_REQUIRED_RESULT bool sftpWrite(sftp_file fd,
const QByteArray &buffer,
const std::function<void(int bytes)> &onWritten);
Q_REQUIRED_RESULT Result sftpCopyGet(const QUrl &url, const QString &src, int permissions, KIO::JobFlags flags);
Q_REQUIRED_RESULT Result sftpCopyPut(const QUrl &url, const QString &dest, int permissions, KIO::JobFlags flags);
};
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment