> 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 > >
