Commit ef0b0dfa authored by Ahmad Samir's avatar Ahmad Samir
Browse files

Find the base mount point of a partition that has bind mounts

With bind mounts on Linux, UDisks returns a 'MountPoints' property that could
include several paths, try to get the actual mount point of the partition
and ignore bind mounts.

Inspired by the approach used by GNOME's gio, use libmount to parse
"/proc/self/mountinfo" and check the path returned by mnt_fs_get_root(), for
the base partition this should return "/", for bind mounts it returns a dir
e.g. /mnt/dirA/.

This fixes a bug where clicking a Device icon in the Places panel could open
the path of one of the bind mounts instead of the actual/base partition mount
point, which is confusing to say the least.

For more details:
!37

https://gitlab.gnome.org/GNOME/glib/-/issues/1271#note_352412
https://gitlab.gnome.org/GNOME/glib/-/commit/e1fa5ffb91e74376394fe17612015d44fec82366
https://github.com/storaged-project/udisks/issues/478

BUG: 349617
FIXED-IN: 5.84
parent 0e98339e
Pipeline #67259 passed with stage
in 2 minutes and 33 seconds
......@@ -65,6 +65,10 @@ set_package_properties(PList PROPERTIES
PURPOSE "Needed to build the iOS device support backend"
)
# Used by the UDisks backend on Linux
find_package(LibMount)
set_package_properties(LibMount PROPERTIES
TYPE OPTIONAL)
include(ECMPoQmTools)
......@@ -111,6 +115,7 @@ if (CMAKE_SYSTEM_NAME MATCHES Linux)
set(UDEV_FOUND TRUE) # for config-solid.h
add_device_backend(udev)
endif()
set(HAVE_LIBMOUNT ${LibMount_FOUND})
add_device_backend(udisks2)
add_device_backend(fstab)
add_device_backend(upower)
......
if (LibMount_FOUND)
set(backend_libs LibMount::LibMount)
endif()
set(backend_sources
udisksmanager.cpp
udisksdevice.cpp
......
......@@ -16,6 +16,11 @@
#include <QGuiApplication>
#include <QWindow>
#include <config-solid.h>
#if HAVE_LIBMOUNT
#include <libmount/libmount.h>
#endif
using namespace Solid::Backends::UDisks2;
StorageAccess::StorageAccess(Device *device)
......@@ -70,17 +75,50 @@ bool StorageAccess::isEncrypted() const
return isLuksDevice() || m_device->isEncryptedCleartext();
}
QString StorageAccess::filePath() const
static QString baseMountPoint(const QByteArray &dev)
{
QByteArrayList mntPoints;
QString mountPoint;
#if HAVE_LIBMOUNT
// UDisks "MountPoints" property contains multiple paths, this happens with
// devices with bind mounts; try finding the "base" mount point
if (struct libmnt_table *table = mnt_new_table()) {
// This parses "/proc/self/mountinfo" by default
if (mnt_table_parse_mtab(table, nullptr) == 0) {
// BACKWARD because the fs's we're interested in, /dev/sdXY, are typically at the end
struct libmnt_iter *itr = mnt_new_iter(MNT_ITER_BACKWARD);
struct libmnt_fs *fs;
const QByteArray devicePath = dev.endsWith('\x00') ? dev.chopped(1) : dev;
while (mnt_table_next_fs(table, itr, &fs) == 0) {
if (mnt_fs_get_srcpath(fs) == devicePath
// Base mount point will have "/" as root fs
&& (strcmp(mnt_fs_get_root(fs), "/") == 0)) {
mountPoint = QFile::decodeName(mnt_fs_get_target(fs));
break;
}
}
mnt_free_iter(itr);
}
mnt_free_table(table);
}
#endif
return mountPoint;
}
QString StorageAccess::filePath() const
{
if (isLuksDevice()) { // encrypted (and unlocked) device
const QString path = clearTextPath();
if (path.isEmpty() || path == "/") {
return QString();
}
Device holderDevice(path);
mntPoints = qdbus_cast<QByteArrayList>(holderDevice.prop("MountPoints"));
const auto mntPoints = qdbus_cast<QByteArrayList>(holderDevice.prop("MountPoints"));
if (!mntPoints.isEmpty()) {
return QFile::decodeName(mntPoints.first()); // FIXME Solid doesn't support multiple mount points
} else {
......@@ -88,13 +126,20 @@ QString StorageAccess::filePath() const
}
}
mntPoints = qdbus_cast<QByteArrayList>(m_device->prop("MountPoints"));
const auto mntPoints = qdbus_cast<QByteArrayList>(m_device->prop("MountPoints"));
if (mntPoints.isEmpty()) {
return {};
}
if (!mntPoints.isEmpty()) {
return QFile::decodeName(mntPoints.first()); // FIXME Solid doesn't support multiple mount points
} else {
return QString();
const QString potentialMountPoint = QFile::decodeName(mntPoints.first());
if (mntPoints.size() == 1) {
return potentialMountPoint;
}
// Device has bind mounts?
const QString basePoint = baseMountPoint(m_device->prop("Device").toByteArray());
return !basePoint.isEmpty() ? basePoint : potentialMountPoint;
}
bool StorageAccess::isIgnored() const
......
......@@ -16,4 +16,6 @@
#cmakedefine01 HAVE_GETMNTINFO
#cmakedefine01 HAVE_SETMNTENT
#cmakedefine01 HAVE_LIBMOUNT
#cmakedefine01 GETMNTINFO_USES_STATVFS
  • J. G.
    @ji started a thread
    Last updated by Ahmad Samir
    • Hello Ahmad. This change resulted in crashes of Dolphin and Plasmashell for some users of Slackware-current. Slackware doesn't use systemd, and /var/run is a bind mount of /run.

      The crash occurs because mnt_fs_get_root(fs) in line 97 of solid/devices/backends/udisks2/udisksstorageaccess.cpp returns NULL under some as of yet unknown circumstances (doesn't affect all Slackware-current users trying to mount removable media or other partitions).

      Someone made this patch that seems to correct the issue. Do you think the patch is reasonable?

      diff -urN solid-5.84.0.orig/src/solid/devices/backends/udisks2/udisksstorageaccess.cpp solid-5.84.0/src/solid/devices/backends/udisks2/udisksstorageaccess.cpp
      --- solid-5.84.0.orig/src/solid/devices/backends/udisks2/udisksstorageaccess.cpp	2021-07-03 15:21:37.000000000 +0300
      +++ solid-5.84.0/src/solid/devices/backends/udisks2/udisksstorageaccess.cpp	2021-07-20 18:21:03.578070205 +0300
      @@ -80,6 +80,8 @@
           QString mountPoint;
       
       #if HAVE_LIBMOUNT
      +    const char *root = NULL;
      +
           // UDisks "MountPoints" property contains multiple paths, this happens with
           // devices with bind mounts; try finding the "base" mount point
           if (struct libmnt_table *table = mnt_new_table()) {
      @@ -93,8 +95,10 @@
       
                   while (mnt_table_next_fs(table, itr, &fs) == 0) {
                       if (mnt_fs_get_srcpath(fs) == devicePath
      +                    // Ensure that is get a valid string for root
      +                    && (root = mnt_fs_get_root(fs))
                           // Base mount point will have "/" as root fs
      -                    && (strcmp(mnt_fs_get_root(fs), "/") == 0)) {
      +                    && (strcmp(root, "/") == 0)) {
                           mountPoint = QFile::decodeName(mnt_fs_get_target(fs));
                           break;
                       }

      Here' the link to the discussion in the Slackware support forum if you want more context, though there's a lot of stabbing in the dark.

      Thanks for reading!

    • Thanks for the report and sorry for the trouble :)

      Reading the libmount docs again, it indeed says that mnt_fs_get_root() can return NULL, so I've replaced strcmp with qstrcmp; could you please test !48 (merged) ?

    Please register or sign in to reply
  • mentioned in commit e5964d13

    Toggle commit list
  • mentioned in merge request !48 (merged)

    Toggle commit list
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment