dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.


> marcingu wrote in thumbnail.cpp:776
> No. The m_thumbnailDirDeviceId is set to 0 and only changed if lstat executes 
> properly. Then it takes value of st_dev. I don't know if st_dev can be 0 
> though and I couldn't find this information in manual either. If it can, we 
> should change it.

I don't see how stat would succeed and st_dev would be 0, but I could be wrong. 
Maybe assert that it's not 0, at least?

> thumbnail.cpp:779
> +        if (lstat(QFile::encodeName(m_thumbBasePath).data(), &baseStat) == 
> -1) {
> +            qCWarning(KIO_THUMBNAIL_LOG) << "Cannot read information about 
> filesystem";
> +            return false;

print out the path in the warning (same on line 786)

> thumbnail.cpp:785
> +    struct stat fileStat;
> +    if(lstat(QFile::encodeName(path).data(), &fileStat) == -1) {
> +        qCWarning(KIO_THUMBNAIL_LOG) << "Cannot read information about 
> filesystem";

missing space after `if`


> thumbnail.h:93
>      qint64 m_maxFileSize;
> +    dev_t m_thumbnailDirDeviceId = 0;
>  };

This will break compilation on Windows, I assume?

On Unix, is the corresponding include present in this file? The lack of context 
makes reviewing difficult indeed. Any chance to move this to gitlab? (see 

  R320 KIO Extras


To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

Reply via email to