dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
I suppose `put()` should do the same, then?
INLINE COMMENTS
> file_unix.cpp:72
> + */
> +static QString blockDeviceByPath(const QString &filename)
> +{
Doesn't KMountPoint::currentMountPoints + findByPath already do this?
> file_unix.cpp:107
> + */
> +// TODO move it to KMountPoint?
> +static int statsDeviceByFile(const QString &filename, int
> &max_hw_single_transfer_chunk, bool &is_removeable)
Probably not, I don't see another user than kio_file for such detailed
information.
> file_unix.cpp:226
> +
> + // set default is non removable and chunk size 512k
> + int block_size = 1024 * 1024 * 1;
... and then the code says 1024*1024 ;)
A comment shouldn't say "what" (the code does that), but "why".
If there's no need to document "why", remove the comment.
> file_unix.cpp:229
> + bool is_removeable = false;
> + statsDeviceByFile(dest, block_size, is_removeable);
> +
make that &block_size and &is_removeable so that when reading this line it's
clear that those are in+out vars.
> file_unix.cpp:232
> + // minimum block size 1M
> + if( block_size < (1024 * 1024 * 1) )
> + block_size = (1024 * 1024 * 1);
use std::clamp, this way you won't even have to repeat the min and max values.
> file_unix.cpp:241
> QFile dest_file(dest);
> - if (!dest_file.open(QIODevice::Truncate | QIODevice::WriteOnly)) {
> + int dest_fd = ::open(dest.toStdString().c_str(), O_CREAT | O_TRUNC |
> (is_removeable? O_SYNC : 0) | O_WRONLY);
> + if (!dest_file.open(dest_fd, QIODevice::Truncate | QIODevice::WriteOnly,
> QFile::AutoCloseHandle)) {
The conversion via std::string is incorrect, encoding wise.
The way to pass a QString (for a filename) to a system call is
QFile::encodeName().
> file_unix.cpp:289
> #endif
> - n = ::read(src_file.handle(), buffer, MAX_IPC_SIZE);
> + n = ::read(src_file.handle(), (void *)&(*buffer), block_size);
>
C casts make me puke ;)
The C++ way would be reinterpret_cast, but I don't think you even need that. It
compiled with a char* before, so just use `buffer.data()`
> file_unix.cpp:324
> #endif
> - if (dest_file.write(buffer, n) != n) {
> + if (dest_file.write(&(*buffer), n) != n) {
> if (dest_file.error() == QFileDevice::ResourceError) { //
> disk full
.data() is more readable
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8254
To: vova7890, #frameworks, dfaure
Cc: dfaure, ngraham, alexeymin, #frameworks
