Properly convert types for properties

[jreidinger]

ruby-dbus represents data using plain Ruby types, which is natural and perfectly fine when the method and property types are simple.

If we export a property with a more complex type, like (stttt) in the following example (available as
Gist, inspired by dinstaller/dbus/manager.rb) then the result will have a similar but incorrect type.

Given a Properties.Get(Progress) call, implemented like this:

$ svc() { pkill -f complex-property; RUBYLIB=lib ruby examples/service/complex-property.rb & sleep 1; }
$ get() { dbus-send  --print-reply --dest=net.vidner.Scratch /net/vidner/Scratch org.freedesktop.DBus.Properties.Get string:net.vidner.Scratch string:Progress; }

The EXPECTED result should be a variant of struct containing uint64's

$ svc; get
   variant   struct {
         string "working"
         uint64 1
         uint64 0
         uint64 100
         uint64 42
      }

At the time this issue was originally reported, it was:

$ git checkout v0.17.0
$ svc; get
   variant       array [
         variant             string "working"
         variant             int32 1
         variant             int32 0
         variant             int32 100
         variant             int32 42
      ]

PR #98 was a partial fix for Get but it is actually wrong since it doesn't respect the Variant return type:

# git checkout v0.18.0.beta1
$ svc; get
   struct {
      string "working"
      uint64 1
      uint64 0
      uint64 100
      uint64 42
   }
# Notice the missing variant above, which is why busctl fails below
$ busctl --user --verbose get-property net.vidner.Scratch /net/vidner/Scratch net.vidner.Scratch Progress
Failed to parse bus message: Invalid argument

GetAll is still mistyped:

$ dbus-send  --print-reply --dest=net.vidner.Scratch /net/vidner/Scratch org.freedesktop.DBus.Properties.GetAll string:net.vidner.Scratch 
   array [
      dict entry(
         string "Progress"
         variant             array [
               variant                   string "working"
               variant                   int32 1
               variant                   int32 0
               variant                   int32 100
               variant                   int32 42
            ]
      )
   ]

Original issue description by JReidinger

Hi,
basically issue is with properties. If property has signature, then ruby-dbus should convert result same as it does for methods. So something like

def initialize
  @test = ["a", "b", {}]
end

dbus_attr_reader :test, "(ssa{sv})"

should work out of the box.

(mvidner: the original report had a(ssa{sv}) as the signature, off by one level of nesting)

[mvidner]

The current behavior is that the property returned by Get is an array (of variants), not a 3-valued struct.

This is because the Ruby to D-Bus value conversion only looks at the method signature, and Property.Get simply returns a variant.

We should add a special case to look at the signature of the property being retrieved.

[mvidner]

(Which reminds me that the library could use more type checking both at client and service side. I need to make a separate Issue for that.)

[jreidinger]

(Which reminds me that the library could use more type checking both at client and service side. I need to make a separate Issue for that.)

agreed. I can very easily return wrong types and even wrong signatures like a(ss{sv}) which results in error in d-feet.

[mvidner]

Another side issue:

The reported issue is at the service side, but when I want to write a test for it, the client side does not distinguish between arrays and structs either (representing them both as Ruby Arrays).

Instead of writing a low-level test at the message+signature level, I will fix it by returning structs as frozen Ruby Arrays.

[mvidner]

I can very easily return ... even wrong signatures like a(ss{sv}) which results in error in d-feet.

Thanks for the catch. (The {sv} should read a{sv})
Even a clearly invalid signature like ! will silently sit there until the property is accessed. It should be caught at declaration time, like for methods. Also signal signatures are not type checked ☹️ . More TODOs...

[mvidner]

Status:

• Properties.Get works (because it is easy to apply the property signature to the return value of the Get method)
• Properties.GetAll still returns variant-typed values, because the current code cannot apply types deep in a Hash/Dictionary

To fix GetAll, the plan is to introduce a DBus::Value class to serve as unambiguous interface between the Ruby layer and D-Bus protocol.

For a property

dbus_attr_accessor :my_property, "s"

The service-side methods my_property, my_property= will continue to work with ::String (but the writer will accept a Value) but GetAll will wrap the obtained String in a Value so that the message marshaling code will be able to send the reply correctly even though the return signature says the dictionary values are variants. (They will be Variants only on the outer level, to satisfy the signature. Oh and I should fix Get to also have an outermost Variant.)

Introducing Value is not trivial. To design it properly I am overhauling PacketMarshaler and PacketUnmarshaler.

I am adding regression tests to PacketUnmarshaller now, also checking if it conforms to the specification. (PR follows soon)

[mvidner]

Oops, "in order to fix this issue" was interpreted by the GitHub bot as "this issue is fixed". Not so, reopening.

[mahlonsmith]

I think I'm seeing a similar problem, adding to this issue. (Let me know you're prefer this as a separate ticket). Properties defined as uint32 are being sent as int32. In this specific case, bluetooth discovery timeout, as defined here.

adapter[ 'DiscoverableTimeout' ] = 0

/usr/local/lib/ruby/gems/2.7.0/gems/ruby-dbus-0.18.0.beta2/lib/dbus/bus.rb:368:in `block in send_sync_or_async': Invalid signature for 'DiscoverableTimeout'; caused by 3 sender=:1.7 -> dest=:1.35 serial=320 reply_serial=20 path=; interface=; member= error_name=org.freedesktop.DBus.Error.InvalidSignature (DBus::Error)

... and on the dbus-monitor end of things:

method call time=1649120833.192372 sender=:1.36 -> destination=org.bluez serial=20 path=/org/bluez/hci0; interface=org.freedesktop.DBus.Properties; member=Set
   string "org.bluez.Adapter1"
   string "DiscoverableTimeout"
   variant       int32 0
error time=1649120833.192665 sender=:1.7 -> destination=:1.36 error_name=org.freedesktop.DBus.Error.InvalidSignature reply_serial=20
   string "Invalid signature for 'DiscoverableTimeout'"

This is using the beta2 prerelease.

[mahlonsmith]

... and almost immediately after writing that, I found the Variants stuff.

adapter[ 'DiscoverableTimeout' ] = DBus.variant( 'u', 0 )

That'll do in a pinch.

[mvidner]

@mahlonsmith Thanks for the report!

I can reproduce the problem with:

#! /usr/bin/env ruby
# frozen_string_literal: true

require "dbus"

bus = DBus::SystemBus.instance
bz_service = bus["org.bluez"]
bz_object = bz_service["/org/bluez/hci0"]
bz_iface = bz_object["org.bluez.Adapter1"]

PROP_NAME = "DiscoverableTimeout"
# read a property
val = bz_iface[PROP_NAME]
puts "#{PROP_NAME} is #{val}"
# write a property
bz_iface[PROP_NAME] = 0

It is present also in earlier versions, not a regression in 0.18 beta.

It seems the fix will be using the recently introduced Data.make_typed in ProxyObjectInterface#[]=.

[mvidner]

With 0.18.1:

• The original issue is fixed, including the GetAll case.
• continuing with a new issue for methods/signals: Check types of method/signal arguments and return values #118

Unrelated problems discovered along the way, validation of

• object paths of exported objects, DBus::Object.new("/invalid-path") should raise #119
• signatures of exported properties, dbus_reader :foo, "invalid!signature!" should raise at once #120