Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo

[Dawit Alemayehu]

> On May 19, 2014, 12:05 p.m., Frank Reininghaus wrote:
> > We are seeing quite a few bug reports about a severe regression between KDE SC 4.13.0 and KDE SC 4.13.1:
> > 
> > https://bugs.kde.org/show_bug.cgi?id=334776
> > 
> > According to the reporter of https://bugs.kde.org/show_bug.cgi?id=334988 the regression has been caused by this commit.
> > 
> > I'm surprised that this was committed to KDE/4.13 at all - the review request was for 'master', and it does not really fix a serious bug. Please make sure that such patches, which fix little annoyances but have the potential to cause serious trouble, get a lot of testing in betas and RCs before they are shipped. Testing on a single system is definitely not enough!
> > 
> > I propose to revert this patch - maybe a better solution which prevents automounting can be found for master/frameworks, but IMHO we should not take any further risks in KDE/4.13. Any objections?
> 
> Thomas Lübking wrote:
>     The patch does somehow not what it promises. 
>     
>     KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath( path );
>     
>     "KMountPoint::currentMountPoints()" promises to return mtab, ie. already used mountpoints, so if there's "mp", it's mounted, iow. it looks like (i've just looked at this for the first time) as if it only shortcuts for already mounted autofs mounts, but will still statvfs on unmounted autofs mounts (mounting them implicitly)
>     
>     Ie. what the patch probably should do was to:
>     
>     if (!mp) { // unmounted, avoid automounts
>        KMountPoint::Ptr pmp = KMountPoint::possibleMountPoints().findByPath(path);
>        if (pmp && pmp->mountType() == QLatin1String("autofs"))
>           return info;
>     }

I think the submitter took David's suggestion and changed the original implementation without understanding the consequences. Thomas' suggested patch would probably be a better starting point. 

David: I could not find a isDirectoryMounted() method in kfileitem.cpp to see what you meant by looking at that implementation. 

In the meantime I think this patch should be reverted because it caused a regression.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117044/#review58143
-----------------------------------------------------------


On May 6, 2014, 8:01 p.m., Tomáš Trnka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117044/
> -----------------------------------------------------------
> 
> (Updated May 6, 2014, 8:01 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
> 
> Previously, all unmounted autofs mountpoints were always mounted
> by krunner/plasma-desktop on startup, defeating the purpose of
> automounting. Let's ignore the unmounted filesystems instead when
> gathering free space stats, just like the "df" utility does.
> 
> Everything still gets mounted properly on first real access.
> 
> The change itself is pretty trivial and I would regard it as a bugfix
> (and thus eligible for 4.13), but I'm posting it for review to see
> what you kdelibs people think.
> 
> 
> Diffs
> -----
> 
>   kio/kfile/kdiskfreespaceinfo.cpp f11eb0998f0e718e9b366f8c26da30586bfa44ef 
> 
> Diff: https://git.reviewboard.kde.org/r/117044/diff/
> 
> 
> Testing
> -------
> 
> I'm using this patch on kdelibs since 4.11 and I have noted no problems
> in connection with ~4 automounted filesystems.
> 
> 
> Thanks,
> 
> Tomáš Trnka
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140524/5b8e2116/attachment.htm>