Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bluez5: Remove incorrect variant levels. #214

Merged
merged 3 commits into from
Aug 12, 2024

Conversation

fkleon
Copy link
Contributor

@fkleon fkleon commented Aug 11, 2024

The levels were set too high, causing introspection to return type 'v' for all properties, which is incorrect and not matching the BlueZ API.

Additionally, some DBus clients (such as dbus-next) perform type validation at runtime and refuse to process such mismatching properties.

This removes the variant levels and adds a unit test to ensure the introspection details returned by the mocks match the expected API.

@fkleon
Copy link
Contributor Author

fkleon commented Aug 11, 2024

I verified the expected types by comparing the output of gdbus introspect --system --dest "org.bluez" --object-path "/org/bluez/hci0" --recurse between a real device and a mock device.

Some properties should be defined as readonly to match the API specs, which I don't think this library supports. Since this did not trip up any DBus client library I've tested I didn't look into that further.

@martinpitt
Copy link
Owner

Ugh -- seems I have misunderstood D-Bus properties forever. The D-Bus spec says that all properties have to be wrapped into a variant -- the .Get() and .Set() APIs take/give variants only. Also, dbus docs say that variant_level=1 is wrapped into exactly one variant level, which matches what D-Bus expects.

But apparently somewhere along the line the variant gets added/stripped. So this indeed doesn't just affect bluez5, but all other templates as well.

I'll land this, and then send a follow-up PR for fixing all the others. Thanks for pointing out!

adapter = self.dbus_con.get_object("org.bluez", path)

adapter_prop_types = _introspect_property_types(adapter, "org.bluez.Adapter1")
assert adapter_prop_types == {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be self.assertEqual() for better failures.

Copy link
Contributor Author

@fkleon fkleon Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good point. I'm used to running tests with pytest which produces nice failure messages as it rewrites the assert statements internally. Will change it to self.assertEqual() for consistency.

@martinpitt
Copy link
Owner

Oh, you were quick -- I already fixed that locally and did some other minor cleanups. I'll also push the "fix the rest" commit right here, this really belongs together. I'll take it from here, thanks!

The levels were set too high, causing introspection to return type 'v' for
all properties, which is incorrect and not matching the BlueZ API.

Additionally, some DBus clients (such as dbus-next) perform type validation at
runtime and refuse to process such mismatching properties.

This removes the variant levels and adds a unit test to ensure the
introspection details returned by the mocks match the expected API.
@fkleon
Copy link
Contributor Author

fkleon commented Aug 12, 2024

Ugh -- seems I have misunderstood D-Bus properties forever. The D-Bus spec says that all properties have to be wrapped into a variant -- the .Get() and .Set() APIs take/give variants only. Also, dbus docs say that variant_level=1 is wrapped into exactly one variant level, which matches what D-Bus expects.

But apparently somewhere along the line the variant gets added/stripped. So this indeed doesn't just affect bluez5, but all other templates as well.

I'll land this, and then send a follow-up PR for fixing all the others. Thanks for pointing out!

To be fair, I've looked at those docs multiple times and could never quite make sense of the variant_level and when it's required. It could certainly use some better examples! Other DBus client libraries don't seem to bother explicitly defining the variant level and instead rely on auto-wrapping when required.

I only noticed something was wrong when I used dbusmock with dbus_fast (fork of dbus_next) and couldn't get it to read any properties without type mismatches on the mock objects. Many clients don't seem to bother with typechecking the actual responses against the definition from the introspection, in which case this is silently working.

The previous commit only fixed the bluez template, but this was a
general misunderstanding of how D-Bus properties work. They are defined
as variants, but the API already wraps/unwraps them.
@martinpitt
Copy link
Owner

@fkleon Right, seems I fell into the same trap. It seems the usual client libs magically unwrap the extra variant transparently then, as this never came up before. But I confirmed with busctl introspect.

The rawhide NM hang is unrelated, I'll look at that separately. Thanks!

@martinpitt martinpitt merged commit d989366 into martinpitt:main Aug 12, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants