Ensure mounted nfs filesystems matches their fstab declared counterpart
ClosedPublic

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

Details

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 11890
Build 11908: 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
177

mountpoint.endsWith(QLatin1Char('/'))

178

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

221

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
176

This is a litlle bit scarce ...

178

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

get it by const ref.

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

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

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

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
248

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
254

This comment is stating the obvious ...

257

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
This revision was automatically updated to reflect the committed changes.