Differential D21204 Diff 60855 src/solid/devices/backends/fstab/fstabhandling.cpp

Changeset View

Standalone View

View Options

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

Show First 20 Lines • Show All 127 Lines • ▼ Show 20 Line(s)126 if (fstype == "fuse.encfs" ||
128 return true; 128​ return true;
129 } 129​ }
130 return false; 130​ return false;
131} 131​}
132 132​
133QString _k_deviceNameForMountpoint(const QString &source, const QString &fstype, 133​QString _k_deviceNameForMountpoint(const QString &source, const QString &fstype,
134 const QString &mountpoint) 134​ const QString &mountpoint)
135{ 135​{
136 if (fstype.startsWith("fuse.")) { 136​ if (fstype.startsWith("fuse.")) {
ngrahamUnsubmitted
Done

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

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

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

meven: Good point, I haven't thought much about this case.
137 return fstype + mountpoint; 137​ return fstype + mountpoint;
138 } 138​ }
139 return source; 139​ return source;
140} 140​}
brunsUnsubmitted
Done

break @ < 80 chars

bruns: break @ < 80 chars
141 141​
142void Solid::Backends::Fstab::FstabHandling::_k_updateFstabMountPointsCache() 142​void Solid::Backends::Fstab::FstabHandling::_k_updateFstabMountPointsCache()
brunsUnsubmitted
Done

that likely breaks for "server:/" NFS mounts

bruns: that likely breaks for "server:/" NFS mounts
mevenAuthorUnsubmitted
Done

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

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

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

bruns: for NFS the following entries are valid: - 192.168.0.1:/some/dir - 192.168.0.1:/some/dir/…
mevenAuthorUnsubmitted
Done

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

meven: What is the udi of of a mounted nfs that has an umounted udi like: server:/ server:/other and…
143{ 143​{
144 if (globalFstabCache->m_fstabCacheValid) { 144​ if (globalFstabCache->m_fstabCacheValid) {
145 return; 145​ return;
146 } 146​ }
147 147​
148 globalFstabCache->m_fstabCache.clear(); 148​ globalFstabCache->m_fstabCache.clear();
149 globalFstabCache->m_fstabOptionsCache.clear(); 149​ globalFstabCache->m_fstabOptionsCache.clear();
150 150​
Show All 9 Lines159 while ((fe = getmntent(fstab)) != nullptr) {
160 const QString fsname = QFile::decodeName(fe->mnt_fsname); 160​ const QString fsname = QFile::decodeName(fe->mnt_fsname);
161 const QString fstype = QFile::decodeName(fe->mnt_type); 161​ const QString fstype = QFile::decodeName(fe->mnt_type);
162 if (_k_isFstabNetworkFileSystem(fstype, fsname) || 162​ if (_k_isFstabNetworkFileSystem(fstype, fsname) ||
163 _k_isFstabSupportedLocalFileSystem(fstype)) { 163​ _k_isFstabSupportedLocalFileSystem(fstype)) {
164 const QString mountpoint = QFile::decodeName(fe->mnt_dir); 164​ const QString mountpoint = QFile::decodeName(fe->mnt_dir);
165 const QString device = _k_deviceNameForMountpoint(fsname, fstype, mountpoint); 165​ const QString device = _k_deviceNameForMountpoint(fsname, fstype, mountpoint);
166 QStringList options = QFile::decodeName(fe->mnt_opts).split(QLatin1Char(',')); 166​ QStringList options = QFile::decodeName(fe->mnt_opts).split(QLatin1Char(','));
167 167​
168 globalFstabCache->m_fstabCache.insert(device, mountpoint); 168​ globalFstabCache->m_fstabCache.insert(device, mountpoint);
brunsUnsubmitted
Done

This is a litlle bit scarce ...

bruns: This is a litlle bit scarce ...
169 globalFstabCache->m_fstabFstypeCache.insert(device, fstype); 169​ globalFstabCache->m_fstabFstypeCache.insert(device, fstype);
apolUnsubmitted
Done

mountpoint.endsWith(QLatin1Char('/'))

apol: mountpoint.endsWith(QLatin1Char('/'))
170 while (!options.isEmpty()) { 170​ while (!options.isEmpty()) {
apolUnsubmitted
Done

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

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

and why "2"?

bruns: and why "2"?
171 globalFstabCache->m_fstabOptionsCache.insert(device, options.takeFirst()); 171​ globalFstabCache->m_fstabOptionsCache.insert(device, options.takeFirst());
172 } 172​ }
173 } 173​ }
174 } 174​ }
175 175​
176 endmntent(fstab); 176​ endmntent(fstab);
177 177​
178#else 178​#else
Show All 26 Lines
205#endif 205​#endif
206 //prevent accessing a blocking directory 206​ //prevent accessing a blocking directory
207 if (_k_isFstabNetworkFileSystem(items.at(2), items.at(0)) || 207​ if (_k_isFstabNetworkFileSystem(items.at(2), items.at(0)) ||
208 _k_isFstabSupportedLocalFileSystem(items.at(2))) { 208​ _k_isFstabSupportedLocalFileSystem(items.at(2))) {
209 const QString device = items.at(0); 209​ const QString device = items.at(0);
210 const QString mountpoint = items.at(1); 210​ const QString mountpoint = items.at(1);
211 211​
212 globalFstabCache->m_fstabCache.insert(device, mountpoint); 212​ globalFstabCache->m_fstabCache.insert(device, mountpoint);
213 } 213​ }
apolUnsubmitted
Done

Same as above.

apol: Same as above.
214 } 214​ }
215 215​
216 fstab.close(); 216​ fstab.close();
217#endif 217​#endif
218 globalFstabCache->m_fstabCacheValid = true; 218​ globalFstabCache->m_fstabCacheValid = true;
219} 219​}
220 220​
221QStringList Solid::Backends::Fstab::FstabHandling::deviceList() 221​QStringList Solid::Backends::Fstab::FstabHandling::deviceList()
222{ 222​{
223 _k_updateFstabMountPointsCache(); 223​ _k_updateFstabMountPointsCache();
224 _k_updateMtabMountPointsCache(); 224​ _k_updateMtabMountPointsCache();
225 225​
226 QStringList devices = globalFstabCache->m_fstabCache.keys(); 226​ QStringList devices = globalFstabCache->m_mtabCache.keys();
227 devices += globalFstabCache->m_mtabCache.keys(); 227​ const QStringList fstabDevices = globalFstabCache->m_fstabCache.keys();
broulikUnsubmitted
Done

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();
    ...
}
broulik: Don't create a temporary `keys()` list just to iterate it. Use a loop like ``` for (auto it =…
228 devices.removeDuplicates(); 228​
229​ // Ensure that regardless an fstab device ends with a slash
230​ // it will match its eventual mounted device regardless whether or not it ends with a slash
231​ for (const QString &device : fstabDevices) {
anthonyfieroniUnsubmitted
Done

get it by const ref.

anthonyfieroni: get it by const ref.
232​ // one of device or deviceName will have a trailing slash
233​ QString deviceName = device;
234​ if (deviceName.endsWith((QLatin1Char('/')))) {
broulikUnsubmitted
Done

Excess parentheses

broulik: Excess parentheses
235​ deviceName.chop(1);
236​ } else {
anthonyfieroniUnsubmitted
Done
deviceName.append('/');
anthonyfieroni: ``` deviceName.append('/'); ```
brunsUnsubmitted
Done

You should check for devices.contains(deviceName) here and continue, avoids detaching deviceName when not necessary.

bruns: You should check for `devices.contains(deviceName)` here and `continue`, avoids detaching…
237​ deviceName.append(QLatin1Char('/'));
238​ }
brunsUnsubmitted
Done

No need to put this in .. else { after continue

bruns: No need to put this in `.. else {` after `continue`
239​ if (!devices.contains(deviceName) && !devices.contains(device)) {
240​ devices.append(device);
241​ }
242​ }
229 return devices; 243​ return devices;
230} 244​}
brunsUnsubmitted
Not Done

This comment is stating the obvious ...

bruns: This comment is stating the obvious ...
231 245​
232QStringList Solid::Backends::Fstab::FstabHandling::mountPoints(const QString &device) 246​QStringList Solid::Backends::Fstab::FstabHandling::mountPoints(const QString &device)
233{ 247​{
brunsUnsubmitted
Not Done

dito

bruns: dito
234 _k_updateFstabMountPointsCache(); 248​ _k_updateFstabMountPointsCache();
235 _k_updateMtabMountPointsCache(); 249​ _k_updateMtabMountPointsCache();
236 250​
237 QStringList mountpoints = globalFstabCache->m_fstabCache.values(device); 251​ QStringList mountpoints = globalFstabCache->m_fstabCache.values(device);
238 mountpoints += globalFstabCache->m_mtabCache.values(device); 252​ mountpoints += globalFstabCache->m_mtabCache.values(device);
239 mountpoints.removeDuplicates(); 253​ mountpoints.removeDuplicates();
240 return mountpoints; 254​ return mountpoints;
241} 255​}
▲ Show 20 Lines • Show All 112 Lines • Show Last 20 Lines