Differential D21204 Diff 58048 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​
151#if HAVE_SETMNTENT 151​#if HAVE_SETMNTENT
152 152​
153 FILE *fstab; 153​ FILE *fstab;
154 if ((fstab = setmntent(FSTAB, "r")) == nullptr) { 154​ if ((fstab = setmntent(FSTAB, "r")) == nullptr) {
155 return; 155​ return;
156 } 156​ }
157 157​
158 struct mntent *fe; 158​ struct mntent *fe;
159 while ((fe = getmntent(fstab)) != nullptr) { 159​ 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​ 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​ // strips last slash
brunsUnsubmitted
Done

This is a litlle bit scarce ...

bruns: This is a litlle bit scarce ...
169​ if (mountpoint.at(mountpoint.length() -1) == '/') {
apolUnsubmitted
Done

mountpoint.endsWith(QLatin1Char('/'))

apol: mountpoint.endsWith(QLatin1Char('/'))
170​ mountpoint = mountpoint.left(mountpoint.length() - 2);
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​ }
168 globalFstabCache->m_fstabCache.insert(device, mountpoint); 172​ globalFstabCache->m_fstabCache.insert(device, mountpoint);
169 globalFstabCache->m_fstabFstypeCache.insert(device, fstype); 173​ globalFstabCache->m_fstabFstypeCache.insert(device, fstype);
170 while (!options.isEmpty()) { 174​ while (!options.isEmpty()) {
171 globalFstabCache->m_fstabOptionsCache.insert(device, options.takeFirst()); 175​ globalFstabCache->m_fstabOptionsCache.insert(device, options.takeFirst());
172 } 176​ }
173 } 177​ }
174 } 178​ }
175 179​
Show All 26 Lines205#else
202 if (items.count() < 4) { 206​ if (items.count() < 4) {
203 continue; 207​ continue;
204 } 208​ }
205#endif 209​#endif
206 //prevent accessing a blocking directory 210​ //prevent accessing a blocking directory
207 if (_k_isFstabNetworkFileSystem(items.at(2), items.at(0)) || 211​ if (_k_isFstabNetworkFileSystem(items.at(2), items.at(0)) ||
208 _k_isFstabSupportedLocalFileSystem(items.at(2))) { 212​ _k_isFstabSupportedLocalFileSystem(items.at(2))) {
209 const QString device = items.at(0); 213​ const QString device = items.at(0);
210 const QString mountpoint = items.at(1); 214​ QString mountpoint = items.at(1);
211 215​
216​ // strips last slash
217​ if (mountpoint.at(mountpoint.length() -1) == '/') {
apolUnsubmitted
Done

Same as above.

apol: Same as above.
218​ mountpoint = mountpoint.left(mountpoint.length() - 2);
219​ }
212 globalFstabCache->m_fstabCache.insert(device, mountpoint); 220​ globalFstabCache->m_fstabCache.insert(device, mountpoint);
213 } 221​ }
214 } 222​ }
215 223​
216 fstab.close(); 224​ fstab.close();
217#endif 225​#endif
218 globalFstabCache->m_fstabCacheValid = true; 226​ globalFstabCache->m_fstabCacheValid = true;
219} 227​}
220 228​
221QStringList Solid::Backends::Fstab::FstabHandling::deviceList() 229​QStringList Solid::Backends::Fstab::FstabHandling::deviceList()
222{ 230​{
223 _k_updateFstabMountPointsCache(); 231​ _k_updateFstabMountPointsCache();
224 _k_updateMtabMountPointsCache(); 232​ _k_updateMtabMountPointsCache();
225 233​
226 QStringList devices = globalFstabCache->m_fstabCache.keys(); 234​ QStringList devices = globalFstabCache->m_fstabCache.keys();
227 devices += globalFstabCache->m_mtabCache.keys(); 235​ devices += globalFstabCache->m_mtabCache.keys();
228 devices.removeDuplicates(); 236​ devices.removeDuplicates();
229 return devices; 237​ return devices;
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 =…
230} 238​}
231 239​
232QStringList Solid::Backends::Fstab::FstabHandling::mountPoints(const QString &device) 240​QStringList Solid::Backends::Fstab::FstabHandling::mountPoints(const QString &device)
233{ 241​{
anthonyfieroniUnsubmitted
Done

get it by const ref.

anthonyfieroni: get it by const ref.
234 _k_updateFstabMountPointsCache(); 242​ _k_updateFstabMountPointsCache();
235 _k_updateMtabMountPointsCache(); 243​ _k_updateMtabMountPointsCache();
236 244​
broulikUnsubmitted
Done

Excess parentheses

broulik: Excess parentheses
237 QStringList mountpoints = globalFstabCache->m_fstabCache.values(device); 245​ QStringList mountpoints = globalFstabCache->m_fstabCache.values(device);
238 mountpoints += globalFstabCache->m_mtabCache.values(device); 246​ mountpoints += globalFstabCache->m_mtabCache.values(device);
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…
239 mountpoints.removeDuplicates(); 247​ mountpoints.removeDuplicates();
240 return mountpoints; 248​ return mountpoints;
brunsUnsubmitted
Done

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

bruns: No need to put this in `.. else {` after `continue`
241} 249​}
242 250​
243QStringList Solid::Backends::Fstab::FstabHandling::options(const QString &device) 251​QStringList Solid::Backends::Fstab::FstabHandling::options(const QString &device)
244{ 252​{
245 _k_updateFstabMountPointsCache(); 253​ _k_updateFstabMountPointsCache();
246 254​
brunsUnsubmitted
Not Done

This comment is stating the obvious ...

bruns: This comment is stating the obvious ...
247 QStringList options = globalFstabCache->m_fstabOptionsCache.values(device); 255​ QStringList options = globalFstabCache->m_fstabOptionsCache.values(device);
248 return options; 256​ return options;
249} 257​}
brunsUnsubmitted
Not Done

dito

bruns: dito
250 258​
251QString Solid::Backends::Fstab::FstabHandling::fstype(const QString &device) 259​QString Solid::Backends::Fstab::FstabHandling::fstype(const QString &device)
252{ 260​{
253 _k_updateFstabMountPointsCache(); 261​ _k_updateFstabMountPointsCache();
254 262​
255 return globalFstabCache->m_fstabFstypeCache.value(device); 263​ return globalFstabCache->m_fstabFstypeCache.value(device);
256} 264​}
257 265​
▲ Show 20 Lines • Show All 96 Lines • Show Last 20 Lines