D28745: Skip caching thumbnails on encrypted filesystems

marcingu abandoned this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: sitter, dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, badbunny, waitquietly,

D28745: Skip caching thumbnails on encrypted filesystems

ngraham added a comment. Cool, thanks! Can you go to Add Action > Abandon? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: sitter, dfaure, thiago, bruns, meven, ngraham,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Here's the new merge request: https://invent.kde.org/network/kio-extras/-/merge_requests/75 REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: sitter, dfaure,

D28745: Skip caching thumbnails on encrypted filesystems

ngraham added a comment. OK great! Thanks for your patience through this very long process. :) REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: sitter, dfaure, thiago, bruns, meven,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#677439 , @ngraham wrote: > Is this unblocked now that https://invent.kde.org/frameworks/solid/-/merge_requests/19 has been merged? I need to make some small changes. I'll create new merge request on

D28745: Skip caching thumbnails on encrypted filesystems

ngraham added a comment. Is this unblocked now that https://invent.kde.org/frameworks/solid/-/merge_requests/19 has been merged? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc:

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. I think I have the Solid part done, but as I don't know this code well, I'd be grateful if someone more advanced on the subject checked it. https://invent.kde.org/frameworks/solid/-/merge_requests/19 Unfortunately, I wasn't able to test it on LUKS device,

D28745: Skip caching thumbnails on encrypted filesystems

sitter added a comment. > Could this be delegated onto someone who knows Solid or should I try figuring it out by myself? Alas, I'm not sure there is anyone who feels responsible enough for solid, so you'll probably have to give it a go yourself. REPOSITORY R320 KIO Extras REVISION

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676566 , @sitter wrote: > In D28745#676452 , @marcingu wrote: > > > !PING. > > I need help from someone with good understanding of Solid to continue. > > >

D28745: Skip caching thumbnails on encrypted filesystems

sitter added a comment. In D28745#676452 , @marcingu wrote: > !PING. > I need help from someone with good understanding of Solid to continue. > > I'm don't know how to determinate if StorageAccess device is encrypted or not. I wanted to

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. !PING. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. !PING. How do we check if file access is encrypted using Solid? Do we need new property/method in `StorageAccess`? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. !PING. I need help from someone with good understanding of Solid to continue. I'm don't know how to determinate if StorageAccess device is encrypted or not. I wanted to use `StorageVolume::usage`, but it's not available for all types of devices and doesn't

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676321 , @bruns wrote: > In D28745#676320 , @marcingu wrote: > > > In D28745#676317 , @bruns wrote: > > > > > In

D28745: Skip caching thumbnails on encrypted filesystems

thiago added a comment. BTW, have you checked if the thumbnails are still generated for non-removable but encrypted filesystems? My whole system is encrypted (except for /boot), so it would be a performance loss if no thumbnails were ever cached. REPOSITORY R320 KIO Extras REVISION

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. In D28745#676320 , @marcingu wrote: > In D28745#676317 , @bruns wrote: > > > In D28745#676313 , @marcingu wrote: > > > > > Ping!

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676317 , @bruns wrote: > In D28745#676313 , @marcingu wrote: > > > Ping! > > I'm remanding about question early, because I could do much more work if I get to

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. In D28745#676313 , @marcingu wrote: > Ping! > I'm remanding about question early, because I could do much more work if I get to do it on weekend. > > Question: > This code won't save thumbnail for file on any

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ping! I'm remanding about question early, because I could do much more work if I get to do it on weekend. Question: This code won't save thumbnail for file on any device that isn't `StorageVolume` or is `StorageVolume` with `usage` `UsageType::Encrypded`.

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#676303 , @bruns wrote: > Second, I have asked for a full context diff, or even better moving this to invent.kde.org, but @marcingu keeps ignoring this. Sorry about that. It got lost under all

D28745: Skip caching thumbnails on encrypted filesystems

bruns requested changes to this revision. bruns added a comment. This revision now requires changes to proceed. @marcingu - have you even verified this works? - I am quite sure it does not, neither for fuse.encfs, fuse.cryfs (as used by Vaults), nor for any LUKS encrypted devices.

D28745: Skip caching thumbnails on encrypted filesystems

ngraham added a comment. @bruns and @meven? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

dfaure accepted this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83358. marcingu added a comment. Renaming "deviceIdUnset" to "s_deviceIdUnset" REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83357=83358 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED

D28745: Skip caching thumbnails on encrypted filesystems

dfaure added inline comments. INLINE COMMENTS > thumbnail.cpp:741 > +bool allowCache; > +allowCache = sharesFilesystemWithThumbRoot(filePath); > +if (!allowCache) { join with previous line (this isn't C) ;) > thumbnail.h:95 > +#ifndef Q_OS_WIN >

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83357. marcingu added a comment. Moving allowCache check, so it's done only if thumbnail was created. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83356=83357 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83356. marcingu added a comment. Setting canonical path as value of hadFirstThumbnail REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83355=83356 REVISION DETAIL https://phabricator.kde.org/D28745

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83355. marcingu added a comment. Removing unnecessary includes REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83354=83355 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

bruns added inline comments. INLINE COMMENTS > thumbnail.cpp:62 > +#include > +#include > What are all these includes for? > thumbnail.cpp:563 > > -if (validThumbnails > 0 && hadFirstThumbnail == dir.filePath()) { > +if (validThumbnails > 0 && hadFirstThumbnail ==

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 9 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83354. marcingu added a comment. Using special value instead of 0 as unset m_thumbnailDirDeviceId. Changing ifdef checks for Q_OS_WIN for consistency. Moving ifdev check into sharesFilesystemWithThumbRoot, to reduce number of preprocessor directives.

D28745: Skip caching thumbnails on encrypted filesystems

thiago added inline comments. INLINE COMMENTS > marcingu wrote in thumbnail.cpp:776 > "-1 isn't exactly a great value for an unsigned int :-)" > > I believe "-1" guaranties unsigned to be set to its maximum value, due to how > type conversion works, but I could be wrong. > > Do you know if

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.cpp:776 > -1 isn't exactly a great value for an unsigned int :-) > > Or rather, it's not great enough (in the "greater than" sense) :-) > > But yeah, 0x is a good "not set yet" value for a dev_t, to be safe >

D28745: Skip caching thumbnails on encrypted filesystems

dfaure added inline comments. INLINE COMMENTS > marcingu wrote in thumbnail.cpp:776 > How about I set it to -1 at start and check for that instead? I checked > definitions and it looks like dev_st is unsigned, but it shouldn't be a > problem. -1 isn't exactly a great value for an unsigned int

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.cpp:776 > I agree with your second sentence. I never said otherwise. It will work, *if* > indeed we are both right that st_dev would never be 0 when stat() succeeds. > My suggestion is that we make sure our assumption

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 3 inline comments as done. marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.h:93 > Which check? This is a member variable, and I'm not sure the dev_t type will > be known at all on Windows. > But if you're not sure either, let's wait until CI tells us.

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83353. marcingu added a comment. Skipping usage of POSIX functions and types on Windows REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83351=83353 REVISION DETAIL https://phabricator.kde.org/D28745

D28745: Skip caching thumbnails on encrypted filesystems

dfaure added inline comments. INLINE COMMENTS > marcingu wrote in thumbnail.cpp:776 > If st_dev cannot be 0 on successful call of lstat, I don't think there's need > to change anything. m_thumbnailDirDeviceId will remain zero only if lstat > returns error and as long it's zero

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.cpp:776 > 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? If st_dev cannot be 0 on successful call of lstat, I don't think there's need to

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:781 > error check here for `!lstat(QFile::encodeName(path).data(), )` is > important, file might have moved for instance. Please elaborate, I don't see what the problem is. REPOSITORY R320 KIO Extras REVISION

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83351. marcingu added a comment. Adding path to error logs REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=83350=83351 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added inline comments. INLINE COMMENTS > dfaure wrote in thumbnail.h:93 > 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

D28745: Skip caching thumbnails on encrypted filesystems

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > 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

D28745: Skip caching thumbnails on encrypted filesystems

ngraham added a subscriber: dfaure. ngraham added a comment. @dfaure, what do you think here? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: dfaure, thiago, bruns, meven, ngraham,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:776 > `!= -1`, Add a warning with errno should it fail. No. The m_thumbnailDirDeviceId is set to 0 and only changed if lstat executes properly. Then it takes value of

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 83350. marcingu added a comment. Using solid for getting Device REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=81255=83350 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. I made merge request for storageAccessFromPath: https://invent.kde.org/frameworks/solid/-/merge_requests/8 REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: thiago,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#675709 , @bruns wrote: > In D28745#675698 , @marcingu wrote: > > > > How often do I have to repeat the thumbnailer has to use the canonical path anyway? Please

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. In D28745#675698 , @marcingu wrote: > > How often do I have to repeat the thumbnailer has to use the canonical path anyway? Please use that. > > `storageAccessFromPath` converts given path into canonical. I figured it

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. > How often do I have to repeat the thumbnailer has to use the canonical path anyway? Please use that. `storageAccessFromPath` converts given path into canonical. I figured it should do so anyway, so I'm not doing it again here. REPOSITORY R320 KIO Extras

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. In D28745#675685 , @marcingu wrote: > Solid::Device device = Solid::Device::storageAccessFromPath(filePath); > if (device.is()) { > allowCache = device.as()->usage() !=

D28745: Skip caching thumbnails on encrypted filesystems

thiago added a comment. > for (Device device: list) { > StorageAccess *storageAccess = device.as(); > if (canonPath.startsWith(storageAccess->filePath()) && storageAccess->filePath().size() > match_length) { > match_length = storageAccess->filePath().size(); >

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ok, so far I have implemented `Solid::Device::storageAccessFromPath` by talking all StorageAccess devices, going though all of them and and returning proper one. code: Solid::Device Solid::Device::storageAccessFromPath(const QString ) { // TODO

D28745: Skip caching thumbnails on encrypted filesystems

meven added a comment. In D28745#675033 , @marcingu wrote: > Ok, so, what I want to do now is to create static method `findByPath` which is going to return Solid::StorageVolume instance (is there a case in which we can expect something

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ok, so, what I want to do now is to create static method `findByPath` which is going to return Solid::StorageVolume instance (is there a case in which we can expect something different than StorageVolume?). Should it be `StorageVolume

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. In D28745#674863 , @meven wrote: > In D28745#674827 , @bruns wrote: > > > In D28745#674711 , @meven wrote: > > > > > > > > >

D28745: Skip caching thumbnails on encrypted filesystems

meven added a comment. In D28745#674827 , @bruns wrote: > In D28745#674711 , @meven wrote: > > > > > > > > > Solid does not provide straight `folder => StorageVolume` yet, but I think

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. And can you please use arc to upload the patch - it is nearly impossible to do a review with the missing context REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: thiago,

D28745: Skip caching thumbnails on encrypted filesystems

bruns requested changes to this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov,

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. In D28745#674711 , @meven wrote: > > Solid does not provide straight `folder => StorageVolume` yet, but I think Solid could have such a utility feature added. > Something like

D28745: Skip caching thumbnails on encrypted filesystems

meven added a comment. In D28745#674294 , @marcingu wrote: > I tried to research Solid using api.kde.org (https://api.kde.org/frameworks/solid/html/classSolid_1_1Device.html,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ping! I'm not able to continue without help of someone who knows Solid::Device well. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: thiago, bruns, meven, ngraham,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. I tried to research Solid using api.kde.org (https://api.kde.org/frameworks/solid/html/classSolid_1_1Device.html, https://api.kde.org/frameworks/solid/html/classSolid_1_1StorageVolume.html) and looking for usages of both Solid::Device and Solid::StorageVolume in

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. In D28745#667225 , @thiago wrote: > Use Solid and ask it if the device is on an encrypted device. If Solid does not have such an API, add it. I fully second that. REPOSITORY R320 KIO Extras REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

thiago added a comment. Use Solid and ask it if the device is on an encrypted device. If Solid does not have such an API, add it. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: thiago,

D28745: Skip caching thumbnails on encrypted filesystems

bruns added a comment. I think the patch as is has some structural problems. I think it would be significantly cleaner when: - the "cacheAllowed" checking code is moved into a separate function - the global `allowCache` variable is removed, and the value is passed into

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:738 > There is an error with the previous line, one of the two should be > allowDirCached I think. > Here you always ignore result of `sharesFilesystemWithThumbRoot` Could

D28745: Skip caching thumbnails on encrypted filesystems

thiago added inline comments. INLINE COMMENTS > thumbnail.cpp:743 > +const auto mount = mountsList.findByPath(filePath); > +allowCache = !(mount->mountType() == > QLatin1String("fuse.cryfs") || mount->mountType() == > QLatin1String("fuse.encfs")); > +

D28745: Skip caching thumbnails on encrypted filesystems

meven requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel,

D28745: Skip caching thumbnails on encrypted filesystems

meven added inline comments. INLINE COMMENTS > thumbnail.cpp:738 > +#else > +allowCache = false; > +#endif There is an error with the previous line, one of the two should be allowDirCached I think. Here you always ignore result of `sharesFilesystemWithThumbRoot` >

D28745: Skip caching thumbnails on encrypted filesystems

ngraham accepted this revision. ngraham added a comment. @bruns? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. PING! Is current code fine or should we get rid of KMountPoint completely somehow? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added inline comments. INLINE COMMENTS > meven wrote in thumbnail.cpp:738 > Can't you move this out of the loop ? Since `m_thumbBasePath` does not change. I've moved this check into new method, so it should make more sense now. REPOSITORY

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, aprcela,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 81255. marcingu added a comment. Moving check for sharing filesystem with thumbroot into new method. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=81167=81255 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

meven added inline comments. INLINE COMMENTS > thumbnail.cpp:738 > +if (!m_thumbnailDirDeviceId) { > +struct stat baseStat; > +if (!lstat(QFile::encodeName(m_thumbBasePath).data(), > )) { Can't you move this out of the loop ? Since

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added a comment. Unless there's way to get rid of KMountPoint completely, this should reduce number of calls to minimum. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 81167. marcingu added a comment. Limiting usage of KMountPoint and lstat to max once per directory. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=80526=81167 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, fprice,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 80526. marcingu added a comment. Review fixes REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=80458=80526 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES thumbnail/thumbnail.cpp

D28745: Skip caching thumbnails on encrypted filesystems

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > thumbnail.cpp:731 > +struct stat baseStat, fileStat; > +if (!lstat(m_thumbBasePath.toStdString().c_str(), ) && >

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked 2 inline comments as done. marcingu added a comment. I improved check if file is on the same filesystem as thumbnails cache, but don't know if we can get rid of KMountPoint completely. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To:

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 80458. marcingu added a comment. Checking if file is on the same filesystem as thumbnails cache directory using lstsat REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=80021=80458 REVISION DETAIL

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. Ok, Thanks! I'll check it this weekend. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns Cc: bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik,

D28745: Skip caching thumbnails on encrypted filesystems

bruns added inline comments. INLINE COMMENTS > thumbnail.cpp:726 > > +const auto mountsList = KMountPoint::currentMountPoints(); > +const auto mount = mountsList.findByPath(filePath); Calling currentMountPoints() for every created file is definitely a bad idea ...

D28745: Skip caching thumbnails on encrypted filesystems

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > thumbnail.cpp:729 > +const auto thumbRootMount = > mountsList.findByPath(m_thumbBasePath); > +//If file is on the same filesystem as

D28745: Skip caching thumbnails on encrypted filesystems

meven accepted this revision. meven added a comment. In D28745#649011 , @marcingu wrote: > In D28745#648036 , @meven wrote: > > > This is gonna have an hefty toll on perf as it will add a `getmntent`

D28745: Skip caching thumbnails on encrypted filesystems

marcingu added a comment. In D28745#648036 , @meven wrote: > This is gonna have an hefty toll on perf as it will add a `getmntent` syscall to every thumbnail generation. > Using `Solid::Device::listFromType` would leverage Solid always

D28745: Skip caching thumbnails on encrypted filesystems

meven added a comment. This is gonna have an hefty toll on perf as it will add a `getmntent` syscall to every thumbnail generation. Using `Solid::Device::listFromType` would leverage Solid always up-to-date (using events rather thane sysalls) device cache. I am not sure in the end this

D28745: Skip caching thumbnails on encrypted filesystems

marcingu marked an inline comment as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu, ivan, broulik, #dolphin, ngraham Cc: ngraham, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, fprice, LeGast00n, cblack,

D28745: Skip caching thumbnails on encrypted filesystems

marcingu updated this revision to Diff 80021. marcingu added a comment. Removing extra whitespaces REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28745?vs=79986=80021 REVISION DETAIL https://phabricator.kde.org/D28745 AFFECTED FILES

D28745: Skip caching thumbnails on encrypted filesystems

ngraham retitled this revision from "Skipping catching of thumbnails on encrypted filesystems" to "Skip caching thumbnails on encrypted filesystems". ngraham edited the summary of this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28745 To: marcingu,