Cross posted here: https://bugs.launchpad.net/ubuntu/+source/glib2.0/+bug/1691908

I can confirm the issue. This really annoying.

@Thorned Rose Please change importance to major. Thanks

(In reply to Khurshid Alam from comment #3)
> @Thorned Rose Please change importance to major. Thanks

Bugzilla fields are not for users to change: they are for maintainers and release team.

Sounds like that unix volume monitor is used instead of udisks2 volume monitor for some reason...

The unix volume monitor needs probably some love, see also:
https://mail.gnome.org/archives/gvfs-list/2017-June/msg00000.html

What is output from "gio mount -l" with glib 2.52.2?

> What is output from "gio mount -l" with glib 2.52.2?

I am puzzled by my own output:

$gio mount -l
-----------------------------------------------
Drive(0): Hitachi HTS543225L9A300
  Type: GProxyDrive (GProxyVolumeMonitorUDisks2)
  Volume(0): Games
    Type: GProxyVolume (GProxyVolumeMonitorUDisks2)
  Volume(1): Data
    Type: GProxyVolume (GProxyVolumeMonitorUDisks2)
    Mount(0): Data -> file:///home/pcuser/Documents
      Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Drive(1): TSSTcorp CDDVDW TS-L633M
  Type: GProxyDrive (GProxyVolumeMonitorUDisks2)
Mount(0): Data -> file:///media/pcuser/Data
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(1): Data -> file:///home/pcuser/Videos
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(2): Data -> file:///home/pcuser/Pictures
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(3): Data -> file:///home/pcuser/Music
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(4): Data -> file:///home/pcuser/Downloads
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)


Notice how /home/pcuser/Documents acts as volume and /media/pcuser/Data act as Mount !! How is this even possible?  It should be opposite.

However it seems file paths are ok. Clicking Documents from nautilus sidebar correctly opens /media/pcuser/Data/Documents and not /media/pcuser/Data which is puzzling.

I think the @Thorned Rose also had same problem until he uses udisk rule to hide the external drive. In my case Data is just another ext4 partition on my hard-drive.

glib 2.52.2

gio mount -l
Drive(0): KINGSTON SV300S37A120G
  Type: GProxyDrive (GProxyVolumeMonitorUDisks2)
Drive(1): WDC WD10EZEX-00WN4A0
  Type: GProxyDrive (GProxyVolumeMonitorUDisks2)

----------------

glib2  2.52.2+9+g3245eba16-1

gio mount -l
Drive(0): KINGSTON SV300S37A120G
  Type: GProxyDrive (GProxyVolumeMonitorUDisks2)
Drive(1): WDC WD10EZEX-00WN4A0
  Type: GProxyDrive (GProxyVolumeMonitorUDisks2)
  Volume(0): Farsight
    Type: GProxyVolume (GProxyVolumeMonitorUDisks2)
    Mount(0): Farsight -> file:///home/rose/Documents
      Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(0): Videos -> file:///home/rose/Videos
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(1): Pictures -> file:///home/rose/Pictures
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(2): Music -> file:///home/rose/Music
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)
Mount(3): Downloads -> file:///home/rose/Downloads
  Type: GProxyMount (GProxyVolumeMonitorUDisks2)

------------------

** ("Farsight" is my WDC WD10EZEX-00WN4A0 HDD's label)

------------------

My current fstab:

# /dev/sda3
UUID=fa2ce9bd-bc9c-43de-901c-c0e3eab790e0	/         	ext4      	rw,relatime,data=ordered	0 1

# /dev/sda1
UUID=B1D5-6223      	/boot     	vfat      	rw,relatime,fmask=0022,dmask=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro	0 2

# /dev/sda2
UUID=ebed628d-7c27-42e1-b5f2-56f8ab459c3e	none      	swap      	defaults  	0 0

# /dev/sdb
UUID=1e3dcee6-a93f-430b-9209-92443cbfacd4	/mnt/Farsight	ext4	defaults	0 2

# Binds
/mnt/Farsight/Documents	/home/rose/Documents	none	bind,x-gvfs-hide	0 0
/mnt/Farsight/Downloads	/home/rose/Downloads	none	bind,x-gvfs-hide	0 0
/mnt/Farsight/Music	/home/rose/Music	none	bind,x-gvfs-hide	0 0
/mnt/Farsight/Pictures	/home/rose/Pictures	none	bind,x-gvfs-hide	0 0
/mnt/Farsight/Videos	/home/rose/Videos	none	bind,x-gvfs-hide	0 0

Created attachment 354268 [details]
Nautilus v3.24.1 with glib2 v2.52.2+9+g3245eba16

Adding a screenshot of Nautilus v3.24.1 with glib2 v2.52.2+9+g3245eba16

Thanks for the outputs. So, the output comes from udisks2 volume monitor, hmm.

This may be caused by changes from Bug 781867, I don't see anything else relevant. Can't you test it with reverted patches?

It is expected that you see mounts from your home directory (or should we ignore bind mounts?), but not with x-gvfs-hide. Something has to be different on Arch Linux, because I can't reproduce it on Fedora. x-gvfs-hide works correctly in my case. 

However, I just see the first bind mount with a wrong name (without x-gvfs-hide) and this is obviously wrong...

Just a note that the source of the problem (at least for the names) is probably the following. See output from "mount":
/dev/sdc1 /media/test ext4 rw,relatime,data=ordered 0 0
/dev/sdc1 /home/oholy/BIND ext4 rw,relatime,data=ordered 0 0
/dev/sdc1 /home/oholy/BIND2 ext4 rw,relatime,data=ordered 0 0

and from "udisksctl dump":
  org.freedesktop.UDisks2.Filesystem:
    MountPoints:        /home/oholy/BIND
                        /home/oholy/BIND2
                        /media/test

for the following fstab:   
/dev/sdc1         /media/test       ext4 defaults,nofail 0 0
/media/test/BIND  /home/oholy/BIND  none bind,nofail     0 0
/media/test/BIND2 /home/oholy/BIND2 none bind,nofail     0 0

It is not possible to determine that /home/oholy/BIND points to some subfolder of /dev/sdc1 from the "mount" and "udisksctl" outputs. It looks like just a device with multiple mountpoints... is such output really expected for bind mounts?

> This may be caused by changes from Bug 781867, I don't see anything else relevant. Can't you test it with reverted patches?

You mean 53ed180 and 83c1b88? Ok. I will try.


>It is expected that you see mounts from your home directory (or should we ignore bind mounts?), but not with x-gvfs-hide.

We ignore bind mounts. It was fixed long ago in b0d5ce1 (https://bugzilla.gnome.org/show_bug.cgi?id=625552)


> Something has to be different on Arch Linux, because I can't reproduce it on Fedora. x-gvfs-hide works correctly in my case. 

If I do not use bind, x-gvfs-hide works perfectly. And it doesn't have to be fstab bind, I can reproduce it with mount --bind

1. Disable all fatab bind in /etc/fstab with x-gvfs-hide set for /dev/sa6

gio mount -l doesn't list /dev/sda6. It doesn't appear on nautilus. Everything is ok. 


2. Now add an arbitrary bind:

$sudo mount --bind /media/Data/MM /home/pcuser/MM

Output of "gio mount -l":
 Drive(0): Hitachi HTS543225L9A300
  Volume(1): Data
    Type: GProxyVolume (GProxyVolumeMonitorUDisks2)
    Mount(0): Data -> file:///home/pcuser/MM

And Gnome-Disks shows that /dev/sda6 is mounted at /home/pcuser/MM instead of /media/Data


3. Bind another folder:

$sudo mount --bind /media/Data/Public /home/pcuser/Public

Output of "gio mount -l":
 Drive(0): Hitachi HTS543225L9A300
  Volume(1): Data
    Type: GProxyVolume (GProxyVolumeMonitorUDisks2)
    Mount(0): Data -> file:///home/pcuser/MM

    Mount(1): Data -> file:///home/pcuser/Public
      Type: GProxyMount (GProxyVolumeMonitorUDisks2)

4. Remove /home/pcuser/MM

$sudo umount /home/pcuser/MM

And Gnome-Disks shows that /dev/sda6 is now mounted at /home/pcuser/Public instead of /media/Data

> It is not possible to determine that /home/oholy/BIND points to some subfolder of /dev/sdc1 from the "mount" and "udisksctl" outputs. It looks like just a device with multiple mountpoints... is such output really expected for bind mounts?

AFAIK, no it shouldn't. But these also happens with glib-2.52.0. Through it shows wrong mount point of /dev/sda1 (with bind), it doesn't expose mount binds with 2.52.0.

I think this is a separate issue with mount. I will ask in util-linux mailing list.

This bug it's present in Fedora 26 actually in the final freeze state (In reply to Ondrej Holy from comment #9)
> It is expected that you see mounts from your home directory (or should we
> ignore bind mounts?), but not with x-gvfs-hide. Something has to be
> different on Arch Linux, because I can't reproduce it on Fedora. x-gvfs-hide
> works correctly in my case.

I discussed this with Karel Zak, util-linux maintainer, and he confirmed that "mount" output from Comment 10 is correct and expected. The root of mounted filesystem can be obtained from /proc/self/mountinfo or using findmnt for example. I think we need something like g_unix_mount_get_fs_root, so we can handle the bind mounts correctly in volume monitors. I will try to prototype something.

But this still doesn't deal with the ignored x-gvfs-hide. I will check with Fedora 26...

In code present some regression(glib 2.54.0).
On nautilus desktop showed many system mount points.

$ LANG=c gio mount -l
Volume(0): Filesystem root
  Type: GUnixVolume
  Mount(0): Filesystem root -> file:///
    Type: GUnixMount
Volume(1): boot
  Type: GUnixVolume
  Mount(0): boot -> file:///boot
    Type: GUnixMount
Volume(2): dev
  Type: GUnixVolume
  Mount(0): dev -> file:///dev
    Type: GUnixMount
Volume(3): hugepages
  Type: GUnixVolume
  Mount(0): hugepages -> file:///dev/hugepages
    Type: GUnixMount
Volume(4): mqueue
  Type: GUnixVolume
  Mount(0): mqueue -> file:///dev/mqueue
    Type: GUnixMount
Volume(5): pts
  Type: GUnixVolume
  Mount(0): pts -> file:///dev/pts
    Type: GUnixMount
Volume(6): shm
  Type: GUnixVolume
  Mount(0): shm -> file:///dev/shm
    Type: GUnixMount
Volume(7): home
  Type: GUnixVolume
  Mount(0): home -> file:///home
    Type: GUnixMount
Volume(8): data1
  Type: GUnixVolume
  Mount(0): data1 -> file:///media/data1
    Type: GUnixMount
Volume(9): data2
  Type: GUnixVolume
  Mount(0): data2 -> file:///media/data2
    Type: GUnixMount
Volume(10): data3
  Type: GUnixVolume
  Mount(0): data3 -> file:///media/data3
    Type: GUnixMount
Volume(11): proc
  Type: GUnixVolume
  Mount(0): proc -> file:///proc
    Type: GUnixMount
Volume(12): binfmt_misc
  Type: GUnixVolume
  Mount(0): binfmt_misc -> file:///proc/sys/fs/binfmt_misc
    Type: GUnixMount
Volume(13): run
  Type: GUnixVolume
  Mount(0): run -> file:///run
    Type: GUnixMount
Volume(14): 1000
  Type: GUnixVolume
  Mount(0): 1000 -> file:///run/user/1000
    Type: GUnixMount
Volume(15): 42
  Type: GUnixVolume
  Mount(0): 42 -> file:///run/user/42
    Type: GUnixMount
Volume(16): sys
  Type: GUnixVolume
  Mount(0): sys -> file:///sys
    Type: GUnixMount
Volume(17): cgroup
  Type: GUnixVolume
  Mount(0): cgroup -> file:///sys/fs/cgroup
    Type: GUnixMount
Volume(18): blkio
  Type: GUnixVolume
  Mount(0): blkio -> file:///sys/fs/cgroup/blkio
    Type: GUnixMount
Volume(19): cpu,cpuacct
  Type: GUnixVolume
  Mount(0): cpu,cpuacct -> file:///sys/fs/cgroup/cpu,cpuacct
    Type: GUnixMount
Volume(20): cpuset
  Type: GUnixVolume
  Mount(0): cpuset -> file:///sys/fs/cgroup/cpuset
    Type: GUnixMount
Volume(21): devices
  Type: GUnixVolume
  Mount(0): devices -> file:///sys/fs/cgroup/devices
    Type: GUnixMount
Volume(22): freezer
  Type: GUnixVolume
  Mount(0): freezer -> file:///sys/fs/cgroup/freezer
    Type: GUnixMount
Volume(23): hugetlb
  Type: GUnixVolume
  Mount(0): hugetlb -> file:///sys/fs/cgroup/hugetlb
    Type: GUnixMount
Volume(24): memory
  Type: GUnixVolume
  Mount(0): memory -> file:///sys/fs/cgroup/memory
    Type: GUnixMount
Volume(25): net_cls,net_prio
  Type: GUnixVolume
  Mount(0): net_cls,net_prio -> file:///sys/fs/cgroup/net_cls,net_prio
    Type: GUnixMount
Volume(26): perf_event
  Type: GUnixVolume
  Mount(0): perf_event -> file:///sys/fs/cgroup/perf_event
    Type: GUnixMount
Volume(27): pids
  Type: GUnixVolume
  Mount(0): pids -> file:///sys/fs/cgroup/pids
    Type: GUnixMount
Volume(28): systemd
  Type: GUnixVolume
  Mount(0): systemd -> file:///sys/fs/cgroup/systemd
    Type: GUnixMount
Volume(29): pstore
  Type: GUnixVolume
  Mount(0): pstore -> file:///sys/fs/pstore
    Type: GUnixMount
Volume(30): selinux
  Type: GUnixVolume
  Mount(0): selinux -> file:///sys/fs/selinux
    Type: GUnixMount
Volume(31): config
  Type: GUnixVolume
  Mount(0): config -> file:///sys/kernel/config
    Type: GUnixMount
Volume(32): debug
  Type: GUnixVolume
  Mount(0): debug -> file:///sys/kernel/debug
    Type: GUnixMount
Volume(33): security
  Type: GUnixVolume
  Mount(0): security -> file:///sys/kernel/security
    Type: GUnixMount
Volume(34): tmp
  Type: GUnixVolume
  Mount(0): tmp -> file:///tmp
    Type: GUnixMount

I think missing check for system type mount point.

I put next code in gio/gunixmounts.c:907(in _g_get_unix_mount_points function):

>if (guess_system_internal(mount_path, mount_fstype, ""))
>        continue;

This fix problem:

$ LANG=c gio mount -l

Volume(0): data1
  Type: GUnixVolume
  Mount(0): foto -> file:///media/data1
    Type: GUnixMount
Volume(1): data2
  Type: GUnixVolume
  Mount(0): music -> file:///media/data2
    Type: GUnixMount
Volume(2): data3
  Type: GUnixVolume
  Mount(0): video -> file:///media/data3
    Type: GUnixMount

But maybe present better solution?

(In reply to vitalik_p from comment #15)
> In code present some regression(glib 2.54.0).
> On nautilus desktop showed many system mount points.
> 
> $ LANG=c gio mount -l
> ...

This is irrelevant to this bug report. Can you please file a new bug report about it?

This is output from unix volume monitor, which should not be shown by default. You don't see udisks2 volume monitor output for some reason...

> I think missing check for system type mount point.
> 
> I put next code in gio/gunixmounts.c:907(in _g_get_unix_mount_points
> function):
> 
> >if (guess_system_internal(mount_path, mount_fstype, ""))
> >        continue;

This is a wrong place, g_unix_mounts_get/g_unix_mount_points_get should return unfiltered list, but it would be nice to improve the unix volume monitor output in a similar manner - gunixvolumemonitor.c. Will you prepare patch for it?

> This is irrelevant to this bug report. Can you please file a new bug report about it?

I just inform about this. I not have time for this.

> You don't see udisks2 volume monitor output for some reason...

udisks2 is not installed in my system.

> This is a wrong place, g_unix_mounts_get/g_unix_mount_points_get should return unfiltered list, but it would be nice to improve the unix volume monitor output in a similar manner - gunixvolumemonitor.c. Will you prepare patch for it?

I do not know the code well enough to do this.
I can check you patch if need.

This issue affects debian/ubuntu/arch linux and derivatives. And probably many more distros.

I can't believe it hasn't been fixed yet.

A quick fix is to recompile glib with --disable-libmount

You have to turn off libmount by default on linux.. the opposite of this..
https://bugzilla.gnome.org/show_bug.cgi?id=771438

Created attachment 369277 [details] [review]
Honor the x-gvfs-hide mount option when using libmount

libmount is by default enabled in Linux in all major distributions. With libmount, mnt_context_get_mtab() returns ALL mounted points, including bind mounts and mounts with the x-gvfs-hide option set in fstab. The corresponding function in glib that loads Unix mount points (_g_get_unix_mounts[1]) process all the mounted points, adding them all to the list of mount points. This list is later filtered, but I didn't follow the code to that point; this patch filters the list in _g_get_unix_mounts() if the "x-gvfs-hide" option is part of the mount point fstab options.

It works for me with glib 2.52.3.


[1] https://gitlab.gnome.org/GNOME/glib/blob/master/gio/gunixmounts.c#L465

Review of attachment 369277 [details] [review]:

::: glib-2.52.3/gio/gunixmounts.c
@@ +411,2 @@
       mount_options = mnt_fs_strdup_options (fs);
+      if (g_strstr_len (mount_options, -1, "x-gvfs-hide") != NULL) 

This will crash if `mount_options == NULL`, and does not look for the separators in the mount options string, so will incorrectly return true if `x-gvfs-hide` is a substring of a longer option.

Created attachment 369358 [details] [review]
Honor the x-gvfs-hide mount option when using libmount

New version of the patch: Verifies that mount_options is not NULL and calls mnt_match_options(), which does the right thing and only matches a mount option if it's exactly as the parameter.

Review of attachment 369358 [details] [review]:

Unfortunately, this bug report mingles GUnixVolumeMonitor results and GVfsUDisks2VolumeMonitor results. But anyway, this approach is wrong. x-gvfs-hide means that it should not be shown in volume monitors, but it should still be listed in g_unix_mounts_get. So, x-gvfs-hide check should go in gunixvolumemonitor.c if it should be supported by GUnixVolumeMonitor also.

But I've just figured out, why x-gvfs-hide doesn't work for bind mounts and GUDisks2VolumeMonitor. It is because g_unix_mounts_get returns bind mounts, however, g_unix_mount_points_get does not:
https://gitlab.gnome.org/GNOME/glib/blob/master/gio/gunixmounts.c#L992

GVfsUDisks2VolumeMonitor look up for mount options over g_unix_mount_point_get_options, because g_unix_mount_get_options isn't provided:
https://bugzilla.gnome.org/show_bug.cgi?id=668132

It seems that bind mounts are correctly hidden if we remove that check from g_unix_mount_points_get.

Created attachment 369379 [details] [review]
gunixmounts: Return bind mounts from g_unix_mount_points_get also

Bind mount are currently returned for g_unix_mounts_get, but not from
g_unix_mount_points_get. This breaks reading of mount options in
GVfsUDisks2VolumeMonitor. Return bind mounts from g_unix_mount_points_get
also.

Or bind mounts have to be removed from g_unix_mount_get output also. However, it makes sense to list bind mount, because it fixes bugs like the following:
https://bugzilla.gnome.org/show_bug.cgi?id=604015

On the other hand, bind mount handling is broken in GVfsUDisks2VolumeMonitor currently and needs some new API to fix this situation, see Comment 14.

Review of attachment 369379 [details] [review]:

That code was originally introduced in commit 44f4309e3b by Mario. Ondrej, can you justify that other things won’t break if it’s removed? I don’t quite understand the big picture here.

Mario, any thoughts?

Similar check has been introduced in https://gitlab.gnome.org/GNOME/glib/commit/b0d5ce1 

As from the bug report (https://bugzilla.gnome.org/show_bug.cgi?id=625552) it was done to hide bind mount in nautilus and fixed in glib-2.35.x. 

Which is why i think it works when we compile glib without libmounts.

(In reply to Philip Withnall from comment #26)
> Review of attachment 369379 [details] [review] [review]:
> 
> That code was originally introduced in commit 44f4309e3b by Mario. Ondrej,
> can you justify that other things won’t break if it’s removed? I don’t quite
> understand the big picture here.

That patch is rather just a prototype, which can help to improve bind mount handling in GVfs a bit, see Bug 794154, but still, proper handling needs some new API (Comment 14, we need to find a way to distinguish bind mounts from regular mounts).

I can't probably justify that other things won't break in that case, sorry, but we have to do something because the current situation is broken (with libmount-based implementation):

g_unix_mount_points_get excludes bind mount
g_unix_mounts_get includes bind mounts

I am pretty convinced that both functions should do the same (at least because GVfs relies on g_unix_mount_point_get_options because of Bug 668132).

I would suggest excluding bind mounts from both functions in stable branches (as it is with other implementations), because I don't think that attachment 369379 [details] [review] is safe enough for stable and because we can't do much in GVfs without some new APIs and attachment 369358 [details] [review] seems wrong to me for the reasons I mentioned in my review.

For master, we have to make a decision whether the bind mounts should be propagated from g_unix_mount... functions. If yes, we have to provide the missing APIs for proper handling. If no, we have to find another way to fix issues like Bug 604015 (I think there were more issues caused by bind mount, but I can't find them right now).

But I am afraid of that I don't have enough insight into bind mounts to make such decision. Maybe Karel Zak, or Vojtech Trefny can have some comments whether we should propagate bind mounts from our APIs?

By the way, there is also a relevant bug for UDisks:
https://github.com/storaged-project/udisks/issues/478

There is nothing like "bind mounts", the bind is *operation* how to create standard note in kernel VFS. There is nowhere in the kernel information how the node (mountpoint) has been created.

It means it is impossible to exclude stuff created by "bind" from /proc/self/mountinfo.

 mount /dev/sda1 /foo
 mount /dev/sda1 /bar

is the same as:

 mount /dev/sda1 /foo
 mount --bind /foo /bar

The difference is only that by bind operation you don't have to mount FS root, but you can use any sub-directory as FS root (mount --bind /foo/subdir /bar).

All you can do is to de-duplicate list of mounted filesystem by source (device).


Again, "bind mount" is another operation to attach FS to kernel VFS, it is not state of any mountpoint.

(In reply to Philip Withnall from comment #27)
> Mario, any thoughts?

Sorry for the delay. It's incredible how slow I am keeping up with things lately... :-(

Anyway, not that I have a lot of insight to add, since I added that code quite a while ago but, as Khurshid pointed out in [1], what I remember is that such code was heavily based at the time in the non-libmount code, from where that snippet was taken from.

Now, it is possible that with libmount this is no longer needed (or incorrect), in which case it would make sense to remove that. But at this point I'd seriously trust Ondrej's and Karel's judment much more than mine, since I'm not very up-to-date with these bits.

Sorry for not being really helpful. I wanted to reply at least, though

[1] https://bugzilla.gnome.org/show_bug.cgi?id=782814#c28

Ok, there is nothing like "bind mounts", but we have mounts as a result of "bind operations", which points usually to some sub-directories of other mounts, but there isn't any way to detect this over GIO API, nor UDisks, which causes problems to our volume monitors and potentially to other services (I suppose it may probably cause e.g. redundant popups from gsd-housekeeping, or wrong filesystem size from gnome-control-center info). Given the fact that those mounts just duplicate content of other mounts (they are more or less equal to symlinks), I don't see much reasons to list them from our APIs, apart from gvfsd-trash which will consequently ignore trash folders on them (as it does with non-libmount-based implementation anyway).

So, at least for stable branches, I would suggest ignoring those mounts as with non-libmount-based implementation (this affecting btrfs subvolumes also). For master, I would suggest to implement g_unix_mount_get_fs_root and g_unix_mount_get_options and mark those mounts as system internal by default. Though not sure what to do with attachment 369379 [details] [review], g_unix_mount_get_options and g_unix_mount_get_fs_root should be enough to handle those in GVfs and I am not sure who else is using g_unix_mount_points and how...

I am just not sure also that the same trick as in non-libmount-implementation can be used to filter them out (whether the sort order of mounts is same):
https://gitlab.gnome.org/GNOME/glib/blob/master/gio/gunixmounts.c#L567. What about something like the following instead?
if (g_strcmp0 (mnt_fs_get_root (fs), "/") != 0)
  continue; // bind mount or btrfs subvolume

Karel, will that work?

Created attachment 370685 [details] [review]
gunixmounts: Ignore mounts which don't point to fs root

Ignore mounts which don't point to the root of a filesystem (i.e.
created by bind operation, or btrfs subvolumes). Those mounts just
duplicate content of other mounts. What is worse, an API to handle
such mounts properly is missing, which causes issues to our volume
monitors among others, because random mountpoint is exposed for one
device (i.e. one volume).

The previous mntent-based g_unix_mounts_get implementation did
more-or-less the same. The original intention was to avoid being
fooled by mounts created over bind operation, however, there is not
a way to detect mounts created over bind operation reliably. So, it
simply ignored mounts with device path that was repeated. It always
took just the first one with the same device path, it relied on that
the first one is the most important, basically, the one which points
to the root.

The solution proposed by this patch should be a bit more clever and
always chooses mounts which point to the root, regardless its order.
Although, there is a small difference. The mounts, which are created
over bind operation, but points to the filesystem root won't be
ignored. However, this is not a problem for our volume monitors at
least.

I would suggest this patch to be used for all stable branches, where libmount is enabled by default at least, in order to be consistent as much as possible with mntent-based implementation. 

Maybe for future, we can change this to not totally skip those mounts, but just mark them as system internal, which would help to fix issues like Bug 604015.

(In reply to Ondrej Holy from comment #32)
> if (g_strcmp0 (mnt_fs_get_root (fs), "/") != 0)
>   continue; // bind mount or btrfs subvolume

The problem is that you can filter out unique mountpoints.

mount /dev/sda2 /foo
mount --bind /mnt/subdir /bar
umount /foo

Now, fs-root for /bar is /subdir, but /dev/sda2 is mounted on /bar only. It means the content is no duplicated anywhere.

The solution is to de-duplicate the list by source (e.g. /dev/sda2).
For example for fstrim --all we use:

static int uniq_fs_target_cmp(
                struct libmnt_table *tb __attribute__((__unused__)),
                struct libmnt_fs *a,
                struct libmnt_fs *b)
{       
        return !mnt_fs_streq_target(a, mnt_fs_get_target(b));
}       
        
static int uniq_fs_source_cmp(
                struct libmnt_table *tb __attribute__((__unused__)),
                struct libmnt_fs *a,
                struct libmnt_fs *b)
{       
        if (mnt_fs_is_pseudofs(a) || mnt_fs_is_netfs(a) ||
            mnt_fs_is_pseudofs(b) || mnt_fs_is_netfs(b))
                return 1;
        
        return !mnt_fs_streq_srcpath(a, mnt_fs_get_srcpath(b));
}

and now de-duplicate the table:

        tab = mnt_new_table_from_file(_PATH_PROC_MOUNTINFO);
        if (!tab)
                err(MNT_EX_FAIL, _("failed to parse %s"), _PATH_PROC_MOUNTINFO); 
        
        /* de-duplicate by mountpoints */
        mnt_table_uniq_fs(tab, 0, uniq_fs_target_cmp);
        
        /* de-duplicate by source */
        mnt_table_uniq_fs(tab, MNT_UNIQ_FORWARD, uniq_fs_source_cmp);
        
        while (mnt_table_next_fs(tab, itr, &fs) == 0) {
              ...
        }


This solution should be good enough, although it does not care about NFS binds where is no /subdir as FS-root in mountinfo, but /subdir is appended to source path. Maybe also for some btrfs setups it will not perfects, but...

I tried the patch from comment 33. And I can confirm it works now. At-least gvfs-hide part.

Thanks for replies!

(In reply to Karel Zak from comment #35)
> ... 
>         
>         /* de-duplicate by mountpoints */
>         mnt_table_uniq_fs(tab, 0, uniq_fs_target_cmp);

This is not related to this bug, but it is probably a good idea to use it.
       
>         /* de-duplicate by source */
>         mnt_table_uniq_fs(tab, MNT_UNIQ_FORWARD, uniq_fs_source_cmp);

I am convinced that this is exactly the same as we have with mntent-based implementation. I wanted to be more clever with attachment 370685 [details] [review] and do not show any mounts created over bind operation, because their handling is not optimal anyway... but maybe this would be more certain for stable.

Just a note that this still filter out some unique mountpoints:

mount /dev/sda2 /foo
mount --bind /foo/subdir /bar
mount --bind /foo/subdir2 /bar2 
umount /foo

(But we can't propagate both of them anyway, because that's exactly what we are not able to handle in monitors without additional API.)

Or choose the wrong one:

mount /dev/sda2 /foo
mount --bind /foo/subdir /bar
umount /foo
mount /dev/sda2 /foo

(But this is the same as with mntent-based implementation.)

I am going to propose some patches for master soon...

Created attachment 371467 [details] [review]
gunixmounts: Filter out mounts with device path that was repeated

libmount-based implementation doesn't filter out mounts with device
path that was repeated as it is done with mntent-based implementation.
It causes problems to our volume monitors which are not able to handle
multiple mounts for one device path properly without additional API.
Let's filter out the same mounts as are filtered out with mntent-based
implementation.

This is intended only for stable branches to prevent current issues.

Created attachment 371468 [details] [review]
gio: Add g_unix_mount_get_root_path

Currently, there isn't API to determine root path for mounts created
over bind operation (or btrfs subvolumes). This causes issues to our
volume monitors if there is multiple mounts for one device, which can
happen with libmount-based implementation currently. Let's propagate
root path from libmount over g_unix_mount_get_root_path, so we can
handle this somehow in our volume monitors.

GLIB_AVAILABLE_IN_ has to be updated accordingly before push.

Created attachment 371469 [details] [review]
gunixmounts: Mark mounts as system internal instead of filtering out (master)

mntent-based implementation filter out mounts with device path that was
repeated. Consequently, it is not possible to show such mounts in UI even
with x-gvfs-show, because they are not returned from g_unix_mounts_get.
libmount-based implementation currently doesn't filter out any mounts
which causes issues to our volume monitors. Let's rather mark mounts
which don't point into fs root as system_internal. This approach won't be
affected by mount order as is mntent-based implementation. It will mark
more mounts as system_internal than it is filtered out with mntend-based
implementation, but there will be always possibility to show them in UI
over x-gvfs-show, which was not possible with mntend-based. We can
probably introduce some improvements later to not mark unique mounts as
system internal even if they don't point into fs root...

Do you think that this is a good idea? Or should we use attachment 371467 [details] [review] also for master and maybe just improve the filtering to not skip unique mountpoints. However, this patch allows us to fix Bug 604015... but maybe we just disable g_file_trash on mounts where g_unix_mount_at returns NULL, it would fix trashing for GNOME at least.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1271.