D29634: sftp: break large writes into multiple requests

This revision was automatically updated to reflect the committed changes. Closed by commit R320:1df6174834bb: sftp: break large writes into multiple requests (authored by sitter). REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29634?vs=82723=82834

D29634: sftp: break large writes into multiple requests

meven added a comment. In D29634#670485 , @feverfew wrote: > In D29634#670419 , @meven wrote: > > > In D29634#670377 , @ngraham wrote: > > > > > Nice

D29634: sftp: break large writes into multiple requests

feverfew added a comment. In D29634#670419 , @meven wrote: > In D29634#670377 , @ngraham wrote: > > > Nice work. > > > > In D29634#670159 , @feverfew

D29634: sftp: break large writes into multiple requests

meven added a comment. In D29634#670377 , @ngraham wrote: > Nice work. > > In D29634#670159 , @feverfew wrote: > > > I imagine something similar should be done for FileJob::write? > > >

D29634: sftp: break large writes into multiple requests

ngraham accepted this revision. ngraham added a comment. Nice work. In D29634#670159 , @feverfew wrote: > I imagine something similar should be done for FileJob::write? Yeah. REPOSITORY R320 KIO Extras BRANCH release/20.04

D29634: sftp: break large writes into multiple requests

feverfew accepted this revision. feverfew added a comment. I imagine something similar should be done for FileJob::write? REPOSITORY R320 KIO Extras BRANCH release/20.04 REVISION DETAIL https://phabricator.kde.org/D29634 To: sitter, ngraham, meven, feverfew Cc: meven, feverfew,

D29634: sftp: break large writes into multiple requests

meven accepted this revision. This revision is now accepted and ready to land. REPOSITORY R320 KIO Extras BRANCH release/20.04 REVISION DETAIL https://phabricator.kde.org/D29634 To: sitter, ngraham, meven Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik,

D29634: sftp: break large writes into multiple requests

sitter edited the test plan for this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D29634 To: sitter, ngraham, meven Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n,

D29634: sftp: break large writes into multiple requests

sitter updated this revision to Diff 82723. sitter added a comment. - rejigger write segmentation into new sftpWrite function used by both the put() and write() - fix length calculation - refine buf size comment REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE

D29634: sftp: break large writes into multiple requests

meven added a comment. Well apart from the length of data needed to be reduced as we progress, all this makes sense. Btw gvfs uses 32768 as MAX_BUFFER_SIZE for sftp https://gitlab.gnome.org/GNOME/gvfs/-/blob/master/daemon/gvfsbackendsftp.c#L94 REPOSITORY R320 KIO Extras REVISION

D29634: sftp: break large writes into multiple requests

sitter added inline comments. INLINE COMMENTS > meven wrote in kio_sftp.cpp:58 > Why not change it now to 32 * 1024 then ? > I guess you tested this value works at least with openssh. > > I guess the best solution would be to ask/figure out the server buffer size. > > What does gvfs, or other

D29634: sftp: break large writes into multiple requests

feverfew added inline comments. INLINE COMMENTS > meven wrote in kio_sftp.cpp:1831-1832 > Maybe we can deduce the server buffer size based on the `bytesWritten` value > : at init `serv_buffer_size =MAX_XFER_BUF_SIZE; ` and then ` if (length > > bytesWritten) { serv_buffer_size = bytesWritten

D29634: sftp: break large writes into multiple requests

meven added inline comments. INLINE COMMENTS > feverfew wrote in kio_sftp.cpp:1831-1832 > AFAICT the size of the buffer never changes so this will easily cause a > buffer overrun if I'm not mistaken? > > Say for example you have a buffer with `buffer.size() == MAX_XFER_BUF_SIZE + > 1`. Then

D29634: sftp: break large writes into multiple requests

meven requested changes to this revision. meven added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kio_sftp.cpp:58 > // you will overflow the 2 byte size variable in a sftp packet. > +// FIXME: this seems too large > +// from the RFC: > +// The maximum

D29634: sftp: break large writes into multiple requests

feverfew added a comment. Seems like something similar should also occur in `FileJob::write`? INLINE COMMENTS > kio_sftp.cpp:1831-1832 > +while (offset < buffer.size()) { > +const auto length = qMin(MAX_XFER_BUF_SIZE, > buffer.size()); > +ssize_t

D29634: sftp: break large writes into multiple requests

sitter created this revision. sitter added a reviewer: ngraham. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. sitter requested review of this revision. REVISION SUMMARY servers have arbitrary limits that we should stay below. to ensure