D21204: Ensure no trailing slash in mountpoint read from fstab file

[Nathaniel Graham]

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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: dhaumann, anthonyfieroni, ngraham, bruns, apol, kde-frameworks-devel, 
LeGast00n, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: dhaumann, anthonyfieroni, ngraham, bruns, apol, kde-frameworks-devel, 
LeGast00n, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Dominik Haumann]

dhaumann added a comment.


  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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: dhaumann, anthonyfieroni, ngraham, bruns, apol, kde-frameworks-devel, 
LeGast00n, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven marked 2 inline comments as done.
meven added a comment.


  Thanks @anthonyfieroni

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: anthonyfieroni, ngraham, bruns, apol, kde-frameworks-devel, LeGast00n, 
michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven updated this revision to Diff 60855.
meven added a comment.


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

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21204?vs=58976&id=60855

BRANCH
  arcpatch-D21204

REVISION DETAIL
  https://phabricator.kde.org/D21204

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

To: meven, bruns, #frameworks
Cc: anthonyfieroni, ngraham, bruns, apol, kde-frameworks-devel, LeGast00n, 
michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> fstabhandling.cpp:231
> +// it will match its eventual mounted device regardless whether or not 
> it ends with a slash
> +for (QString device : fstabDevices) {
> +QString deviceName = device;

get it by const ref.

> fstabhandling.cpp:236
> +} else {
> +deviceName = device + '/';
> +}

deviceName.append('/');

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: anthonyfieroni, ngraham, bruns, apol, kde-frameworks-devel, LeGast00n, 
michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven added a comment.


  ping @bruns

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, LeGast00n, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven added a comment.


  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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, LeGast00n, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven added a comment.


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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, LeGast00n, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven updated this revision to Diff 58976.
meven added a comment.


  Tweak FstabHandling::deviceList instead of editing device udi

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21204?vs=58237&id=58976

BRANCH
  D21204

REVISION DETAIL
  https://phabricator.kde.org/D21204

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

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> bruns wrote in 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

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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> meven wrote in fstabhandling.cpp:142
> I am not familiar with those mounts.
> What is the udi of those mounted filesystem ?

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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven marked 2 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> ngraham wrote in fstabhandling.cpp:136
> So this situation could never happen with FUSE mounts? Or should that case be 
> checked too.

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

> bruns wrote in fstabhandling.cpp:142
> that likely breaks for "server:/" NFS mounts

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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven marked 2 inline comments as done.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven updated this revision to Diff 58237.
meven added a comment.


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

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21204?vs=58190&id=58237

BRANCH
  arcpatch-D21204

REVISION DETAIL
  https://phabricator.kde.org/D21204

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

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fstabhandling.cpp:140
> +
> +// strips last slash so that the device matches the one returned by 
> getmntent when filesystem is mounted
> +// even when fstab mnt_dir ends with a '/' in /etc/fstab

break @ < 80 chars

> fstabhandling.cpp:142
> +// even when fstab mnt_dir ends with a '/' in /etc/fstab
> +if (source.endsWith((QLatin1Char('/' {
> +QString deviceName(source);

that likely breaks for "server:/" NFS mounts

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> fstabhandling.cpp:136
>  {
>  if (fstype.startsWith("fuse.")) {
>  return fstype + mountpoint;

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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: ngraham, bruns, apol, kde-frameworks-devel, michaelh

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven updated this revision to Diff 58190.
meven added a comment.


  Move code change to _k_deviceNameForMountpoint

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21204?vs=58189&id=58190

BRANCH
  arcpatch-D21204

REVISION DETAIL
  https://phabricator.kde.org/D21204

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

To: meven, bruns, #frameworks
Cc: bruns, apol, kde-frameworks-devel, michaelh, ngraham

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven marked 2 inline comments as done.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: bruns, apol, kde-frameworks-devel, michaelh, ngraham

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven updated this revision to Diff 58189.
meven marked 3 inline comments as done.
meven added a comment.


  use chop to strip last /

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21204?vs=58188&id=58189

BRANCH
  arcpatch-D21204

REVISION DETAIL
  https://phabricator.kde.org/D21204

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

To: meven, bruns, #frameworks
Cc: bruns, apol, kde-frameworks-devel, michaelh, ngraham

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven updated this revision to Diff 58188.
meven added a comment.


  Fix code, improved doc, better testing

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21204?vs=58048&id=58188

BRANCH
  arcpatch-D21204

REVISION DETAIL
  https://phabricator.kde.org/D21204

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

To: meven, bruns, #frameworks
Cc: bruns, apol, kde-frameworks-devel, michaelh, ngraham

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Nathaniel Graham]

ngraham added reviewers: bruns, Frameworks.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven, bruns, #frameworks
Cc: bruns, apol, kde-frameworks-devel, michaelh, ngraham

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fstabhandling.cpp:168
>  
> +// strips last slash
> +if (mountpoint.at(mountpoint.length() -1) == '/') {

This is a litlle bit scarce ...

> apol wrote in fstabhandling.cpp:170
> you can call mountpoint.chop(2) for the same effect, which should be easier 
> to read.

and why "2"?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven
Cc: bruns, apol, kde-frameworks-devel, michaelh, ngraham

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> fstabhandling.cpp:169
> +// strips last slash
> +if (mountpoint.at(mountpoint.length() -1) == '/') {
> +mountpoint = mountpoint.left(mountpoint.length() - 2);

mountpoint.endsWith(QLatin1Char('/'))

> fstabhandling.cpp:170
> +if (mountpoint.at(mountpoint.length() -1) == '/') {
> +mountpoint = mountpoint.left(mountpoint.length() - 2);
> +}

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

> fstabhandling.cpp:217
> +// strips last slash
> +if (mountpoint.at(mountpoint.length() -1) == '/') {
> +mountpoint = mountpoint.left(mountpoint.length() - 2);

Same as above.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D21204

To: meven
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D21204: Ensure no trailing slash in mountpoint read from fstab file.

[Méven Car]

meven created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  BUG: 406242
  CCBUG: 390691
  FIXED-IN: 5.59

REPOSITORY
  R245 Solid

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21204

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

To: meven
Cc: kde-frameworks-devel, michaelh, ngraham, bruns