Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
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
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117044/#review58143 --- 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? - Frank Reininghaus 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
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
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?
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;
}
- Thomas
---
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
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117044/#review57500 --- Note that this change won't make it automatically into Plasma Next, it needs forward-porting. - Sebastian Kügler 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
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117044/#review57443 --- This review has been submitted with commit 6246e99b43f3d1a9e15d563fbb5e173ed50ba5e5 by Tomáš Trnka to branch KDE/4.13. - Commit Hook On March 25, 2014, 7:17 a.m., Tomáš Trnka wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117044/ --- (Updated March 25, 2014, 7:17 a.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
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
--- 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.) Status -- This change has been marked as submitted. 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
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117044/#review55479 --- Ship it! Looks ok. There are probably other names for automounted partition types, though... like subfs at least, if I believe kmountpoint.cpp. Another solution is the one from kfileitem.cpp isDirectoryMounted() - identifying unmounted dirs by the fact that they have a size of 0. - David Faure On March 25, 2014, 7:17 a.m., Tomáš Trnka wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117044/ --- (Updated March 25, 2014, 7:17 a.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
Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117044/ --- 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
