databasesanitizer: Use flags for filtering
ClosedPublic

Authored by michaelh on Mar 27 2018, 1:35 PM.

Details

Summary

This is in preparation for the upcoming 'clean' and 'repair' commands.
It also helps to keep the API stable.

Depends on D11452

Test Plan
$ baloodb devices --missing-only --mounted-only
Listing database contents...
+ device:2053 indexed-items:115 fstype:ext4 fsname:/dev/sda5 mount:/home
+ device:2049 indexed-items:19 fstype:ext4 fsname:/dev/sda1 mount:/
+ device:22 indexed-items:2 fstype:tmpfs fsname:tmpfs mount:/run
Found 3 matching in 3 devices
Elapsed: 3.447 secs

Diff Detail

Repository
R293 Baloo
Branch
sanitize-enums (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
michaelh created this revision.Mar 27 2018, 1:35 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMar 27 2018, 1:35 PM
michaelh requested review of this revision.Mar 27 2018, 1:35 PM
michaelh edited the summary of this revision. (Show Details)
bruns added a subscriber: bruns.Apr 4 2018, 11:32 PM
bruns added inline comments.
src/engine/databasesanitizer.h
41

Is this correct? IgnoreMounted = (1 << 1) = 2 == IgnoreUnavailable?

michaelh added inline comments.Apr 5 2018, 7:22 AM
src/engine/databasesanitizer.h
41

That's a sneaky question! :-)

michaelh updated this revision to Diff 31365.Apr 5 2018, 8:35 AM
  • Correct typo(?)
bruns added inline comments.Apr 9 2018, 3:03 PM
src/engine/databasesanitizer.cpp
100 ↗(On Diff #31365)

As you are not iterating over sorted keys, just directly iterate on the map.

104 ↗(On Diff #31365)

Why are you fetching the url a second time here? url == map[key].second.

225 ↗(On Diff #31365)

You filter a second time here (first time in createList).

I would propose to use the same format independent of the Ignore(Un)Available.

237 ↗(On Diff #31365)

This needs improvement - if you only print the inaccessible ones, the count is off.
Better explicitly mention number of Total, Accessible, Inaccessible.

src/engine/databasesanitizer.h
41

Hm still awkward:

IgnoreNone = 0,
IgnoreAvailable = 1,
IgnoreUnavailable = 2,
IgnoreMounted = IgnoreAvailable << 4,       <--  (1 << 4) == 16
IgnoreUnmounted = IgnoreUnavailable << 4    <--  (2 << 4) == 32
michaelh updated this revision to Diff 31807.Apr 10 2018, 12:53 PM
michaelh marked 2 inline comments as done.
  • Adapt to upstream changes
  • Correct enum
  • Apply suggested changes
michaelh updated this revision to Diff 31811.Apr 10 2018, 12:58 PM
  • Improve printList summary
michaelh marked 4 inline comments as done.Apr 10 2018, 1:00 PM
michaelh added inline comments.Apr 10 2018, 1:04 PM
src/engine/databasesanitizer.cpp
124 ↗(On Diff #31365)

I'm not content with this solution. Still, it's the best I could do without bloating the code too much.

237 ↗(On Diff #31365)

see above

bruns added inline comments.Apr 10 2018, 4:14 PM
src/engine/databasesanitizer.cpp
111–112 ↗(On Diff #31365)

I think this becomes a little bit clearer if you move it to the top of the map iteration and use a continue; to process the next item, e.g.:

quint64 id= it.key();
deviceId = idToDeviceId(id);  // idutils.h

if (!includeIds.isEmpty() && !includeIds.contains(deviceId)) {
    continue;
} else if (excludeIds.contains(info.deviceId)) {
    continue;
} else if (urlFilter && !urlFilter->match(it.value()).hasMatch())
    continue;
}

checkedFiles += 1;
FileInfo info; 
....
if (info.accessible)
   accessibleCount++

if ((info.accessible && !(accessFilter & IgnoreAvailable)) ||
    (!info.accessible && !(accessFilter & IgnoreUnavailable))) {
    result.append(info);
}

ignoredCount = map.size() - checkedFiles
inaccessibleCount = checkedFiles - accessibleCount

124 ↗(On Diff #31365)

for the second, return

struct summary {
    quint64 ignored;
    quint64 accessible;
    quint64 inaccessible;
};
165 ↗(On Diff #31365)

You can omit DatabaseSanitizer:: here

src/engine/databasesanitizer.h
41

Thats probably not what you want - IgnoreUnmounted (mounted-only) implicitly includes IgnoreUnavailable, so you will exclude listing unavailable files even on mounted filesystems.

michaelh planned changes to this revision.Apr 12 2018, 3:26 PM
michaelh updated this revision to Diff 32005.Apr 12 2018, 7:46 PM
  • Hopefully get the enum right this time
  • Use struct for summary
bruns added inline comments.Apr 12 2018, 8:51 PM
src/engine/experimental/databasesanitizer.cpp
32 ↗(On Diff #32005)

double QDebug

michaelh updated this revision to Diff 32014.EditedApr 12 2018, 9:03 PM

Single QDebug

bruns accepted this revision.Apr 12 2018, 9:07 PM
This revision is now accepted and ready to land.Apr 12 2018, 9:07 PM
bruns requested changes to this revision.Apr 13 2018, 3:25 AM
bruns added inline comments.
src/engine/experimental/databasesanitizer.cpp
193 ↗(On Diff #32014)

this is definitely wrong, erasing invalidates iterators.

it = usedDevices.erase(it);

195 ↗(On Diff #32014)

you can move the block with the iteration over usedDevices and filtering into printDevices ...

You filter for IgnoreMounted there anyway.

271 ↗(On Diff #32014)

Filtering again?

This revision now requires changes to proceed.Apr 13 2018, 3:25 AM
michaelh updated this revision to Diff 32104.Apr 14 2018, 11:06 AM
  • Adapt to upstream changes
michaelh updated this revision to Diff 32147.Apr 14 2018, 8:29 PM
  • Ignore tmpfs devices
bruns accepted this revision.Apr 15 2018, 2:42 AM

Save the missing comment for "tmpfs", looks fine, thanks!

src/engine/experimental/databasesanitizer.cpp
250 ↗(On Diff #32147)

This line definitely needs a comment why this is here

This revision is now accepted and ready to land.Apr 15 2018, 2:42 AM
michaelh added inline comments.Apr 15 2018, 11:43 AM
src/engine/experimental/databasesanitizer.cpp
250 ↗(On Diff #32147)

added in D11753

This revision was automatically updated to reflect the committed changes.