D28488: [Fstab] Ensure uniqueness for all filesystem types

[Méven Car]

meven added a comment.


  A consequence of this is the deviceName is used in fstab device description :
  
  F8240164: Screenshot_20200415_180517.png 

  
  Making planner the need for D28590 

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> I got the impression you liked being lax. Guess only when the shoe is on the 
> other foot.

I do like being lax! I literally gave you a ship it despite your comment being 
literally wrong.
Picking on useless crap such as whether a comment is technically entirely 
correct or not doesn't give anything of value to anyone except the person 
picking, who is really just getting an air of superiority I'm sure. The rest of 
the world meanwhile gets their time wasted. You seem to not appreciate that, 
and I'll clearly not make you see it either. One of many great failures of 
mine, right next to wanting things get done.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:e468daf0d100: [Fstab] Ensure uniqueness for all 
filesystem types (authored by bruns).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28488?vs=79067=79126

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

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> sitter wrote in fstabhandling.cpp:131
> The previous comment was factually correct, it never claimed the same source 
> on the same mount would be differentiated. Your comment on the other hand 
> does and it's wrong. It's doesn't say it's unique for the purposes of 
> presentation or anything. It says mountpoints are unique. But they are not.
> I got the impression you liked nitpicking. Guess only when the shoe is on the 
> other foot.

I got the impression you liked being lax. Guess only when the shoe is on the 
other foot.

REPOSITORY
  R245 Solid

BRANCH
  fix_comment

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> You let the old comment pass without any further remarks, but now you start 
> nitpicking?
> 
> From the acessibility viewpoint of the mount, it is unique.

The previous comment was factually correct, it never claimed the same source on 
the same mount would be differentiated. Your comment on the other hand does and 
it's wrong. It's doesn't say it's unique for the purposes of presentation or 
anything. It says mountpoints are unique. But they are not.
I got the impression you liked nitpicking. Guess only when the shoe is on the 
other foot.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R245 Solid

BRANCH
  fix_comment

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> sitter wrote in fstabhandling.cpp:131
> The comment is still wrong.

You let the old comment pass without any further remarks, but now you start 
nitpicking?

From the acessibility viewpoint of the mount, it is unique.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> The first mount is no longer visible, it is shadowed by the first one. You 
> can not read any files from it or write to it. You can not  unmount it.

The comment is still wrong.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

bruns marked an inline comment as done.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added a comment.


  In D28488#639942 , @broulik wrote:
  
  > Wat. So maybe we need to add a hash of mount options to it? (Though I 
personally would call that pebkac)
  
  
  +1

INLINE COMMENTS

> sitter wrote in fstabhandling.cpp:131
> 
> 
>   λ ajax ~ → sudo mount -t cifs //bear.local/foo /mnt -o user=me  
>   λ ajax ~ → sudo mount -t cifs //bear.local/foo /mnt -o user=me
>   λ ajax ~ → sudo mount --bind /tmp /mnt 
>   λ ajax ~ → sudo mount --bind /tmp /mnt
>   λ ajax ~ → mount|grep /mnt
>   //bear.local/foo on /mnt type cifs 
> (rw,relatime,vers=3.1.1,cache=strict,username=me,uid=0,noforceuid,gid=0,noforcegid,addr=fd00::::8c8d:02ff:fec7:d6a4,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,rsize=4194304,wsize=4194304,bsize=1048576,echo_interval=60,actimeo=1)
>   //bear.local/foo on /mnt type cifs 
> (rw,relatime,vers=3.1.1,cache=strict,username=me,uid=0,noforceuid,gid=0,noforcegid,addr=2a03:c100:f100:4f00:8c8d:02ff:fec7:d6a4,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,rsize=4194304,wsize=4194304,bsize=1048576,echo_interval=60,actimeo=1,user=me)
>   /dev/nvme1n1p1 on /mnt type btrfs 
> (rw,relatime,ssd,space_cache,subvolid=1082,subvol=/@/tmp)
>   /dev/nvme1n1p1 on /mnt type btrfs 
> (rw,relatime,ssd,space_cache,subvolid=1082,subvol=/@/tmp)

The first mount is no longer visible, it is shadowed by the first one. You can 
not read any files from it or write to it. You can not  unmount it.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Kai Uwe Broulik]

broulik added a comment.


  Wat. So maybe we need to add a hash of mount options to it? (Though I 
personally would call that pebkac)

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> Of course they are - you can just mount one fs at a path at any time.

λ ajax ~ → sudo mount -t cifs //bear.local/foo /mnt -o user=me  
  λ ajax ~ → sudo mount -t cifs //bear.local/foo /mnt -o user=me
  λ ajax ~ → sudo mount --bind /tmp /mnt 
  λ ajax ~ → sudo mount --bind /tmp /mnt
  λ ajax ~ → mount|grep /mnt
  //bear.local/foo on /mnt type cifs 
(rw,relatime,vers=3.1.1,cache=strict,username=me,uid=0,noforceuid,gid=0,noforcegid,addr=fd00::::8c8d:02ff:fec7:d6a4,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,rsize=4194304,wsize=4194304,bsize=1048576,echo_interval=60,actimeo=1)
  //bear.local/foo on /mnt type cifs 
(rw,relatime,vers=3.1.1,cache=strict,username=me,uid=0,noforceuid,gid=0,noforcegid,addr=2a03:c100:f100:4f00:8c8d:02ff:fec7:d6a4,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,rsize=4194304,wsize=4194304,bsize=1048576,echo_interval=60,actimeo=1,user=me)
  /dev/nvme1n1p1 on /mnt type btrfs 
(rw,relatime,ssd,space_cache,subvolid=1082,subvol=/@/tmp)
  /dev/nvme1n1p1 on /mnt type btrfs 
(rw,relatime,ssd,space_cache,subvolid=1082,subvol=/@/tmp)

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

bruns marked an inline comment as done.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> sitter wrote in fstabhandling.cpp:131
> mountpoints are not unique

Of course they are - you can just mount one fs at a path at any time.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter requested changes to this revision.
sitter added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> fstabhandling.cpp:131
> +// for different users. Make sure it is unique by appending the
> +// mountpoint (which is unique).
> +return source + QLatin1Char(':') + mountpoint;

mountpoints are not unique

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Méven Car]

meven accepted this revision.
meven added a comment.
This revision is now accepted and ready to land.


  Fine

REPOSITORY
  R245 Solid

BRANCH
  fix_comment

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

bruns added a comment.


  In D28488#639815 , @meven wrote:
  
  > Good idea, we should fix this once and for all.
  >  Two points though about udi form :
  >
  > 1/ Nfs udi would no be great.
  >  For instance the source for nfs is usually of the form :
  >  192.168.0.1:/export
  >
  > With this we would have 
  >  192.168.0.1:/export:/mountpount
  >
  > I suggest for the nfs source to remove the first ':' in the first place
  >  To get :
  >  192.168.0.1/export:mountpount for nfs
  
  
  Lots of mangling for IMHO no benefit. The UDI is not really visible for the 
user.
  
  > 2/ For fuse mounts this removes the fstype from the udi, which is present 
currently for this case and handy.
  
  It does not ...

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Méven Car]

meven added a reviewer: sitter.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Méven Car]

meven requested changes to this revision.
meven added a comment.
This revision now requires changes to proceed.


  Good idea, we should fix this once and for all.
  Two points though about udi form :
  
  1/ Nfs udi would no be great.
  For instance the source for nfs is usually of the form :
  192.168.0.1:/export
  
  With this we would have 
  192.168.0.1:/export:/mountpount
  
  I suggest for the nfs source to remove the first ':' in the first place
  To get :
  192.168.0.1/export:@mountpount for nfs
  
  2/ For fuse mounts this removes the fstype from the udi, which is present 
currently for this case and handy.
  So perhaps we would want to use the fstype :
  
  nfs/192.168.0.1/export:mountpount for nfs
  cifs//server/export:mountpoint for cifs (perhaps without the double slash)
  fuse.type/source/export:mountpount for fuse
  sftp/username@source/export:mountpoint for sftp
  
  Perhaps event fstype:/ to make the fstype look like a protocol in the udi, 
i.e : `fuse.type:/source/export:mountpount` and 
`sftp:/username@source/export:mountpoint for sftp`
  A complete udi would like like:
  `/orge/kde/fstab/sftp:/username@source/export:mountpoint`

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Kai Uwe Broulik]

broulik accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R245 Solid

BRANCH
  fix_comment

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

To: bruns, #frameworks, meven, broulik
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Kai Uwe Broulik]

broulik added a comment.


  +1

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Stefan Brüns]

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

REVISION SUMMARY
  Commit a99c6136da2d 
 
("Samba: Ensure to differenciate mounts sharing the
  same source") only fixed the multiple mount problem for SMB share. This
  can also happen for all other mounts (remounted local file systems,
  NFS, ...).
  
  Also clarify the comment.

REPOSITORY
  R245 Solid

BRANCH
  fix_comment

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

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

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