D8254: Make file ioslave backend on removeables are user-frendly on linux

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171202/b95d8df6/attachment-0001.html>