Differential D21204

Ensure mounted nfs filesystems matches their fstab declared counterpart
ClosedPublic
Actions

Authored by meven on May 14 2019, 7:43 AM.

Details

Reviewers
bruns
ngraham
Group Reviewers
Frameworks
Commits
R245:c97f0b2a3076: Ensure mounted nfs filesystems matches their fstab declared counterpart
Summary

When a nfs fs is declared in /etc/fstab and the filesystem path ends with a / (like 192.168.1.16:/home/meven/ ), once mounted the detected filesystem path does not have the slash.
This causes the mounted drive not to match the umounted filesystem, causing the later bug.

I.e :

$ solid-hardware list
[...]
udi = '/org/kde/fstab/192.168.1.16:/home/meven/' (fstab declared mount)
udi = '/org/kde/fstab/192.168.1.16:/home/meven' (mounted drive as returned by getmntent)

The patch makes the logic in Solid::Backends::Fstab::FstabHandling::deviceList matching mounted and unmounted filesytems unsensitive to their path ending with /

BUG: 406242
CCBUG: 390691
FIXED-IN: 5.66

Test Plan

Locally tested with Linux with a nfs drive set up with a path ending with a slash like
192.168.1.16:/home/meven/ /media/NFS nfs defaults,user,auto,noatime,bg 0 0

After patch
$ solid-hardware list
[...]
udi = '/org/kde/fstab/192.168.1.16:/home/meven' (mounted drive as returned by getmntent)

Only one icon appears for the drive in places panel in Dolphin.

Not tested with getmntinfo / BSD code path, but the patch does not alter the code behavior much and don't foresee any risk there.

Diff Detail

Repository
R245 Solid
Branch
arcpatch-D21204
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13459
Build 13477: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 14 2019, 7:43 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.May 14 2019, 7:43 AM
apol added a subscriber: apol.May 14 2019, 12:33 PM
apol added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
169

mountpoint.endsWith(QLatin1Char('/'))

170

you can call mountpoint.chop(2) for the same effect, which should be easier to read.

213

Same as above.

bruns added a subscriber: bruns.May 14 2019, 1:53 PM
bruns added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
168

This is a litlle bit scarce ...

170

and why "2"?

meven updated this revision to Diff 58188.May 17 2019, 12:34 PM

Fix code, improved doc, better testing

meven updated this revision to Diff 58189.May 17 2019, 12:35 PM
meven marked 3 inline comments as done.

use chop to strip last /

meven marked 2 inline comments as done.May 17 2019, 12:36 PM
meven updated this revision to Diff 58190.May 17 2019, 12:42 PM

Move code change to _k_deviceNameForMountpoint

ngraham added a subscriber: ngraham.May 17 2019, 4:00 PM
ngraham added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
136

So this situation could never happen with FUSE mounts? Or should that case be checked too.

bruns added inline comments.May 17 2019, 5:48 PM
src/solid/devices/backends/fstab/fstabhandling.cpp
140

break @ < 80 chars

142

that likely breaks for "server:/" NFS mounts

meven updated this revision to Diff 58237.May 18 2019, 7:03 AM

Shorten comment line, ensure fuse mount do not end with / either

meven marked 2 inline comments as done.May 18 2019, 7:04 AM
meven added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
136

Good point, I haven't thought much about this case.

142

I am not familiar with those mounts.
What is the udi of those mounted filesystem ?

meven marked 2 inline comments as done.May 18 2019, 7:04 AM
bruns added inline comments.May 25 2019, 4:22 PM
src/solid/devices/backends/fstab/fstabhandling.cpp
142

for NFS the following entries are valid:

  • 192.168.0.1:/some/dir
  • 192.168.0.1:/some/dir/
  • server:/other
  • server:/

while server: (without slash) is invalid.

The first two entries are semantically the same

meven added inline comments.May 28 2019, 7:24 AM
src/solid/devices/backends/fstab/fstabhandling.cpp
142

What is the udi of of a mounted nfs that has an umounted udi like: server:/ server:/other and server:/other/ ?

meven updated this revision to Diff 58976.Jun 1 2019, 9:45 AM

Tweak FstabHandling::deviceList instead of editing device udi

meven edited the summary of this revision. (Show Details)Jun 1 2019, 9:45 AM
meven added a comment.Jun 4 2019, 12:06 PM

I have tested the new patch.
It is a bit a workaround but does not edit any udi.
What do you think ?

meven added a comment.Jun 15 2019, 7:08 AM

How do you like this alternative @bruns ?
The upside is that is fixes the bug and does not change stored data and keeps compatibility.
The downside is that it is a little hackish.

anthonyfieroni added a subscriber: anthonyfieroni.Jun 29 2019, 11:35 AM
anthonyfieroni added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
231

get it by const ref.

236
deviceName.append('/');
meven updated this revision to Diff 60855.Jun 30 2019, 10:24 AM

Use a const ref in the loop use append to add a trailing /

meven marked 2 inline comments as done.Jun 30 2019, 10:25 AM

Thanks @anthonyfieroni

dhaumann added a subscriber: dhaumann.Jun 30 2019, 10:07 PM

Minor/general comment: given this is a KDE Frameworks change, could you improve the summary of the commit message for this and future commits? Here it just says bug, ccbug, fixedin. Imho a commit log should be self-explaining and self-contained: what is broken exactly, why is it broken, why is the suggested fix correct, what testing did you do, what possible risks does the change have?

Everything is missing here. As consequence, you get poor or no reviews, and no one feels good enough giving a ship-it.

ngraham added a comment.Jun 30 2019, 11:14 PM
In D21204#488719, @dhaumann wrote:

Minor/general comment: given this is a KDE Frameworks change, could you improve the summary of the commit message for this and future commits? Here it just says bug, ccbug, fixedin. Imho a commit log should be self-explaining and self-contained: what is broken exactly, why is it broken, why is the suggested fix correct, what testing did you do, what possible risks does the change have?

Everything is missing here. As consequence, you get poor or no reviews, and no one feels good enough giving a ship-it.

+100; improving your title and description is often a key part of getting more reviewers.

ngraham retitled this revision from Ensure no trailing slash in mountpoint read from fstab file. to Ensure no trailing slash in mountpoint read from fstab file.Jun 30 2019, 11:14 PM
meven retitled this revision from Ensure no trailing slash in mountpoint read from fstab file to Ensure mounted mounted nfs filesystems matches their fstab declared counterpart.Jul 1 2019, 9:30 AM
meven edited the summary of this revision. (Show Details)
meven edited the test plan for this revision. (Show Details)
In D21204#488719, @dhaumann wrote:

Minor/general comment: given this is a KDE Frameworks change, could you improve the summary of the commit message for this and future commits? Here it just says bug, ccbug, fixedin. Imho a commit log should be self-explaining and self-contained: what is broken exactly, why is it broken, why is the suggested fix correct, what testing did you do, what possible risks does the change have?

Everything is missing here. As consequence, you get poor or no reviews, and no one feels good enough giving a ship-it.

Thank you for pointing this out, this is already great feedback.

meven retitled this revision from Ensure mounted mounted nfs filesystems matches their fstab declared counterpart to Ensure mounted nfs filesystems matches their fstab declared counterpart.Jul 1 2019, 9:58 AM
broulik added a subscriber: broulik.Jul 1 2019, 10:05 AM
broulik added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
227 ↗(On Diff #60855)

Don't create a temporary keys() list just to iterate it. Use a loop like

for (auto it = globalFstabCache->m_fstabCache.constBegin(),
     end = globalFstabCache->m_fstabCache.constEnd();
     it != end; ++it) {
    QString deviceName = it.key();
    ...
}
234 ↗(On Diff #60855)

Excess parentheses

meven updated this revision to Diff 60913.Jul 1 2019, 11:16 AM

Use an iterator to loop over globalFstabCache->m_fstabCache

meven marked 2 inline comments as done.Jul 1 2019, 11:52 AM
meven marked 4 inline comments as done.Jul 1 2019, 12:19 PM
meven updated this revision to Diff 62027.Jul 19 2019, 7:19 AM

Remove dead code

ngraham added a comment.Jul 19 2019, 2:54 PM

@bruns, is this good to go now?

ngraham accepted this revision.Sep 12 2019, 9:07 PM

Let's get this in soon. We have plenty of time before Frameworks 5.63.

This revision is now accepted and ready to land.Sep 12 2019, 9:07 PM
ngraham edited the summary of this revision. (Show Details)Sep 12 2019, 9:07 PM
bruns requested changes to this revision.Sep 13 2019, 12:40 AM
bruns added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
236 ↗(On Diff #60855)

You should check for devices.contains(deviceName) here and continue, avoids detaching deviceName when not necessary.

This revision now requires changes to proceed.Sep 13 2019, 12:40 AM
meven updated this revision to Diff 65955.Sep 13 2019, 8:12 AM

Rebase on master, review feedback

meven marked an inline comment as done.Sep 13 2019, 8:12 AM
bruns added inline comments.Sep 13 2019, 1:51 PM
src/solid/devices/backends/fstab/fstabhandling.cpp
238 ↗(On Diff #60855)

No need to put this in .. else { after continue

meven edited the summary of this revision. (Show Details)Oct 15 2019, 1:53 PM
meven updated this revision to Diff 67981.Oct 15 2019, 1:57 PM
meven marked an inline comment as done.

Avoid else after continue;

bruns accepted this revision.Dec 12 2019, 8:27 PM
bruns added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
244 ↗(On Diff #60855)

This comment is stating the obvious ...

247 ↗(On Diff #60855)

dito

This revision is now accepted and ready to land.Dec 12 2019, 8:27 PM
bruns added a comment.Dec 12 2019, 8:28 PM

please remove the 2 comments.

ngraham edited the summary of this revision. (Show Details)Dec 12 2019, 9:10 PM
Closed by commit R245:c97f0b2a3076: Ensure mounted nfs filesystems matches their fstab declared counterpart (authored by meven). · Explain WhyDec 15 2019, 7:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 60913

View Options

src/solid/devices/backends/fstab/fstabhandling.cpp

Loading...