Ensure mounted nfs filesystems matches their fstab declared counterpart
Needs ReviewPublic

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

Details

Reviewers
bruns
ngraham
Group Reviewers
Frameworks
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.63

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 16456
Build 16474: arc lint + arc unit
meven created this revision.May 14 2019, 7:43 AM
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
171

mountpoint.endsWith(QLatin1Char('/'))

172

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

215

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
170

This is a litlle bit scarce ...

172

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 inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
137–138

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
142

break @ < 80 chars

144

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
137–138

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

144

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
144

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
144

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 inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
233

get it by const ref.

238
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

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.

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)

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 inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
229

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();
    ...
}
236

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

@bruns, is this good to go now?

ngraham accepted this revision.Thu, Sep 12, 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.Thu, Sep 12, 9:07 PM
ngraham edited the summary of this revision. (Show Details)Thu, Sep 12, 9:07 PM
bruns requested changes to this revision.Fri, Sep 13, 12:40 AM
bruns added inline comments.
src/solid/devices/backends/fstab/fstabhandling.cpp
238

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

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

Rebase on master, review feedback

meven marked an inline comment as done.Fri, Sep 13, 8:12 AM
bruns added inline comments.Fri, Sep 13, 1:51 PM
src/solid/devices/backends/fstab/fstabhandling.cpp
240

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