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 ↗(On Diff #30716)

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

michaelh added inline comments.Apr 5 2018, 7:22 AM
src/engine/databasesanitizer.h
41 ↗(On Diff #30716)

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 ↗(On Diff #30716)

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
134 ↗(On Diff #31811)

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

245 ↗(On Diff #31811)

see above

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

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

134 ↗(On Diff #31811)

for the second, return

struct summary {
    quint64 ignored;
    quint64 accessible;
    quint64 inaccessible;
};
183 ↗(On Diff #31811)

You can omit DatabaseSanitizer:: here

src/engine/databasesanitizer.h
41 ↗(On Diff #30716)

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

this is definitely wrong, erasing invalidates iterators.

it = usedDevices.erase(it);

195

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

You filter for IgnoreMounted there anyway.

271

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
271–272 ↗(On Diff #32005)

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
271–272 ↗(On Diff #32005)

added in D11753

This revision was automatically updated to reflect the committed changes.