Calling prop.Properties.Set changes the type of value to a pointer

[jeremija]

I stumbled upon this problem while using this library in another project so I tried debugging it by adding the following test case to prop/prop_test.go and it fails:

func TestInt32(t *testing.T) {
	srv, err := dbus.SessionBus()
	if err != nil {
		t.Fatal(err)
	}
	defer srv.Close()

	cli, err := dbus.SessionBus()
	if err != nil {
		t.Fatal(err)
	}
	defer cli.Close()

	propsSpec := map[string]map[string]*Prop{
		"org.guelfey.DBus.Test": {
			"int32": {
				int32(100),
				true,
				EmitTrue,
				nil,
			},
		},
	}
	props := New(srv, "/org/guelfey/DBus/Test", propsSpec)

	obj := cli.Object(srv.Names()[0], "/org/guelfey/DBus/Test")

	comparePropValue(obj, "int32", int32(100), t)
	r := props.GetMust("org.guelfey.DBus.Test", "int32")
	if r != int32(100) {
		t.Errorf("expected r to be int32(100), but was %#v", r)
	}

	if err := props.Set("org.guelfey.DBus.Test", "int32", dbus.MakeVariant(int32(101))); err != nil {
		t.Fatalf("failed to set prop int32 to 101")
	}

	comparePropValue(obj, "int32", int32(101), t)
	r = props.GetMust("org.guelfey.DBus.Test", "int32")
	if r != int32(101) {
		t.Errorf("expected r to be int32(101), but was %#v", r)
	}
}

$ go test ./prop -run TestInt32
msg signal from org.freedesktop.DBus to :1.129 serial 2 path /org/freedesktop/DBus interface org.freedesktop.DBus member NameAcquired
  ":1.129"
deliver?
--- FAIL: TestInt32 (0.00s)
    prop_test.go:68: expected r to be int32(101), but was (*int32)(0xc000192120)
FAIL
FAIL    github.com/godbus/dbus/v5/prop  0.006s
FAIL

The first call to GetMust on the server side returns the int32(100), but after calling Set it starts returning a pointer.

This doesn't happen when I use SetMust, but I'd rather my code not to panic if the connection to DBus fails so I have to use SetMust, but then I have to check the type of value every time which is inconsistent and inconvenient:

	v, err := s.props.Get(dbusInterfaceName, "MyInt32Prop")
	if err != nil {
		return err
	}

	switch t := v.Value().(type) {
	case int32:
		value = t
	case *int32:
		value = *t
 	}

[guelfey]

After some investigation, it seems like this was introduced in #217, which tried to fix that structs in Value weren't handled correctly. The root cause is that for struct conversions, dbus.Store must be used, which needs to be passed a pointer. But if Value isn't a pointer already, we can't get a pointer to it to pass to dbus.Store due to reflection limitations. So the attempted fix in #217 just overwrites Value with a new pointer then, leading to this breakage.

I think the best fix would be to make a deep copy of the initial property map, and then only use pointers in Value internally, converting between them for all method calls.

[jeremija]

Thanks for looking into it!

and then only use pointers in Value internally

Would this mean that the retrieved value from my example would always be int32 or *int32? Asking because I strongly prefer int32: if the property is defined as int32 in DBus, I think it's more user-friendly to return the exact type, rather than a pointer to it.

[guelfey]

Exactly; for purposes of the method calls like GetMust they would be an int32, but internally copied and stored as *int32.