[UDev Backend] Narrow device queried for
ClosedPublic

Authored by broulik on May 24 2019, 9:16 AM.

Details

Summary

By checking for usb subsystem and the identifying property we use in queryDeviceInterface we can significantly reduce the number of wrapper items created.

Test Plan

Previously hotplugengine took 46ms to initialize on plasmashell startup here, now it's down to 2ms.
My phone still shows up as Camera when connected via PTP rather than MTP.

Needs D21555 for cameras to show up properly on application start

Diff Detail

Repository
R245 Solid
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.May 24 2019, 9:16 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 24 2019, 9:16 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
broulik requested review of this revision.May 24 2019, 9:16 AM
bruns added a comment.May 24 2019, 1:12 PM

libgphoto2.rules lists the following devices (USB VID:PID):

  • 1403:0001 Sitronix Digital Photo Frame

which apparently shows up as a generic mass storage (sd[a-z]), but actually is something completely different, see https://github.com/gphoto/libgphoto2/tree/master/camlibs/st2205

  • 1908:xxxx Appotech based Photo frame

https://github.com/gphoto/libgphoto2/tree/master/camlibs/ax203

  • 04b0:xxxx Nikon Coolpix, pre PTP era (~2002)

Thanks, @bruns but what does this mean for this patch? :D

bruns added a comment.May 24 2019, 2:35 PM

sd[a-z] is subsystem "block"
sg[0-9] is susbsystem "scsi_generic"

none of the ID_GPHOTO2==1 devices is a tty or dvb device

src/solid/devices/backends/shared/udevqtclient.h
53

This definitely needs some documentation

  • values in subsystems form a union (ORed)
  • key/values in properties form a union (ORed)
  • both unions are intersected (ANDed)

i.e. in the camera case, (usb || dvb || tty) && (ID_GPHOTO2==1)

Okay, so we can just filter for "usb" then since we don't monitor "block" and "scsi_generic"

This definitely needs some documentation

Agreed

bruns requested changes to this revision.May 25 2019, 3:31 PM

Please also update the summary.

This revision now requires changes to proceed.May 25 2019, 3:31 PM
broulik updated this revision to Diff 58706.May 27 2019, 8:04 AM
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
  • Limit to usb subsystem only
  • Add docs

So, you want me to change that 1 to QVariant() then?

So, you want me to change that 1 to QVariant() then?

I think that is completely sufficient, yes.

In all the other places, the !QVariant::toString().isEmpty() and QVariant::toInt() == 1 should be replaced by just QVariant::isValid()

bruns added inline comments.May 29 2019, 11:22 AM
src/solid/devices/backends/shared/udevqtclient.cpp
223

Nitpick - the existing code uses toLatin1(), and for the ASCII subset it doesn't matter, but I am quite sure properties and values are UTF8 in general.

I have no strong proof, but the udev changelog has:

udev 069
========
A bunch of mostly trivial bugfixes. From now on no node name or
symlink name can contain any character than plain whitelisted ascii
characters or validated utf8 byte-streams. This is needed for the
/dev/disk/by-label/* links, because we import untrusted data and
export it to the filesystem.

broulik added inline comments.May 29 2019, 1:09 PM
src/solid/devices/backends/shared/udevqtclient.cpp
223

This is just copied from the other code. I can fixup all of them separately later if you want.

broulik updated this revision to Diff 58839.May 29 2019, 1:38 PM
broulik retitled this revision from [UDev Backend] Narrow subsystems queried for cameras to [UDev Backend] Narrow device queried for.
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
bruns accepted this revision.May 29 2019, 2:42 PM
bruns added inline comments.
src/solid/devices/backends/shared/udevqtclient.cpp
223

Yes, thats completely fine for me - I was just looking through the code and picking up everything that should be fixed.

This revision is now accepted and ready to land.May 29 2019, 2:42 PM
This revision was automatically updated to reflect the committed changes.
broulik reopened this revision.May 31 2019, 10:47 AM

I just reverted it as it indeed causes problems with initial device lookup. Not just in plasmashell but also in Dolphin. Since Frameworks tagging is tomrrow, better leave it for now and investigate thorougly next week :)

This revision is now accepted and ready to land.May 31 2019, 10:47 AM

For some reason here my udev_enumerate_add_match_property("ID_MEDIA_PLAYER", null) doesn't work. If I check for "1" it works but null doesn't find any devices. Since the systemd code uses fnmatch, we can probably just pass * to it to match anything.

bruns added a comment.Jun 1 2019, 9:11 PM

For some reason here my udev_enumerate_add_match_property("ID_MEDIA_PLAYER", null) doesn't work. If I check for "1" it works but null doesn't find any devices. Since the systemd code uses fnmatch, we can probably just pass * to it to match anything.

nullptr only matches when the udev property also has no value (if (!value && !value_dev) return true;). "*" should work, yes.

The question now is, should we also differnentiate here, i.e. map QVariant() to nullptr, and use "*" for wildcards?

broulik updated this revision to Diff 59052.Jun 3 2019, 7:50 AM
broulik edited the test plan for this revision. (Show Details)
  • Use * wildcard

Ping :)

The question now is, should we also differnentiate here, i.e. map QVariant() to nullptr, and use "*" for wildcards?

What do you mean?

bruns added a comment.Jun 6 2019, 10:09 AM

Ping :)

The question now is, should we also differnentiate here, i.e. map QVariant() to nullptr, and use "*" for wildcards?

What do you mean?

Previously, you used QVariant() as wildcard, now it is "*", which matches the udev syntax.

bruns accepted this revision.Jun 6 2019, 10:09 AM
This revision was automatically updated to reflect the committed changes.