Repository: godbus/dbus
Pull request #217 (2020-10-31 23:06:41)

prop: fix set property due to direct assignment of dbus variant
use an addressable helper value and store/demarshal the dbus variant properly
into value before assigning

https://github.com/godbus/dbus/issues/216

jsouthworth: Would you mind providing some unit tests that demonstrate the appropriate semantics for this (to ensure we don't break it again in the future)?

I don't follow exactly how this is meant to function which means I don't really understand how it is currently broken (I've not used properties in my applications).

Thanks!
idleroamer: I have added a test which demonstrates the broken functionality, but I don't think that it runs part of travis.
I am a total novice in golang so please let me know how to proceed.
jsouthworth: Looks good to me. Thanks!

Patch:
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
index 51250e4..d3c4625 100644
--- a/.github/workflows/go.yml
+++ b/.github/workflows/go.yml
@@ -33,7 +33,7 @@ jobs:
         fi
 
     - name: Build
-      run: go build -v .
+      run: go build -v ./...
 
     - name: Test
-      run: go test -v .
+      run: go test -v ./...
diff --git a/_examples/prop.go b/_examples/prop.go
index 7053e2a..45115eb 100644
--- a/_examples/prop.go
+++ b/_examples/prop.go
@@ -16,6 +16,11 @@ func (f foo) Foo() (string, *dbus.Error) {
 	return string(f), nil
 }
 
+type Foo struct {
+	Id    int
+	Value string
+}
+
 func main() {
 	conn, err := dbus.ConnectSessionBus()
 	if err != nil {
@@ -43,6 +48,17 @@ func main() {
 					return nil
 				},
 			},
+			"FooStruct": {
+				Foo{Id: 1, Value: "First"},
+				true,
+				prop.EmitTrue,
+				func(c *prop.Change) *dbus.Error {
+					var foo Foo
+					dbus.Store([]interface{}{c.Value}, &foo)
+					fmt.Println(c.Name, "changed to", foo)
+					return nil
+				},
+			},
 		},
 	}
 	f := foo("Bar")
diff --git a/prop/prop.go b/prop/prop.go
index 1297cee..cb7b3b1 100644
--- a/prop/prop.go
+++ b/prop/prop.go
@@ -3,6 +3,7 @@
 package prop
 
 import (
+	"reflect"
 	"sync"
 
 	"github.com/godbus/dbus/v5"
@@ -228,7 +229,18 @@ func (p *Properties) Introspection(iface string) []introspect.Property {
 // must already be locked.
 func (p *Properties) set(iface, property string, v interface{}) error {
 	prop := p.m[iface][property]
-	prop.Value = v
+	if reflect.ValueOf(prop.Value).Kind() != reflect.Ptr {
+		prop.Value = reflect.New(reflect.TypeOf(prop.Value)).Interface()
+	}
+	err := dbus.Store([]interface{}{v}, prop.Value)
+	if err != nil {
+		return err
+	}
+	return p.emitChange(iface, property)
+}
+
+func (p *Properties) emitChange(iface, property string) error {
+	prop := p.m[iface][property]
 	switch prop.Emit {
 	case EmitFalse:
 		return nil // do nothing
@@ -237,7 +249,7 @@ func (p *Properties) set(iface, property string, v interface{}) error {
 			iface, map[string]dbus.Variant{}, []string{property})
 	case EmitTrue:
 		return p.conn.Emit(p.path, "org.freedesktop.DBus.Properties.PropertiesChanged",
-			iface, map[string]dbus.Variant{property: dbus.MakeVariant(v)},
+			iface, map[string]dbus.Variant{property: dbus.MakeVariant(prop.Value)},
 			[]string{})
 	default:
 		panic("invalid value for EmitType")
@@ -279,7 +291,9 @@ func (p *Properties) Set(iface, property string, newv dbus.Variant) *dbus.Error
 func (p *Properties) SetMust(iface, property string, v interface{}) {
 	p.mut.Lock()
 	defer p.mut.Unlock() // unlock in case of panic
-	if err := p.set(iface, property, v); err != nil {
+	prop := p.m[iface][property]
+	prop.Value = v
+	if err := p.emitChange(iface, property); err != nil {
 		panic(err)
 	}
 }


Test patch:
diff --git a/prop/prop_test.go b/prop/prop_test.go
new file mode 100644
index 0000000..b7ee047
--- /dev/null
+++ b/prop/prop_test.go
@@ -0,0 +1,102 @@
+package prop
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/godbus/dbus/v5"
+)
+
+type Foo struct {
+	Id    int
+	Value string
+}
+
+func comparePropValue(obj dbus.BusObject, name string, want interface{}, t *testing.T) {
+	r, err := obj.GetProperty("org.guelfey.DBus.Test." + name)
+	if err != nil {
+		t.Fatal(err)
+	}
+	haveValue := reflect.New(reflect.TypeOf(want)).Interface()
+	dbus.Store([]interface{}{r.Value()}, haveValue)
+	have := reflect.ValueOf(haveValue).Elem().Interface()
+	if !reflect.DeepEqual(have, want) {
+		t.Errorf("struct comparison failed: got '%v', want '%v'", have, want)
+	}
+}
+
+func TestValidateStructsAsProp(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()
+
+	foo := Foo{Id: 1, Value: "First"}
+	fooPtr := &Foo{Id: 1, Value: "1st"}
+	foos := make([]Foo, 2)
+	foos[0] = Foo{Id: 1, Value: "Ones"}
+	foos[1] = Foo{Id: 2, Value: "Twos"}
+
+	propsSpec := map[string]map[string]*Prop{
+		"org.guelfey.DBus.Test": {
+			"FooStruct": {
+				foo,
+				true,
+				EmitTrue,
+				nil,
+			},
+			"FooStructPtr": {
+				&fooPtr,
+				true,
+				EmitTrue,
+				nil,
+			},
+			"SliceOfFoos": {
+				foos,
+				true,
+				EmitTrue,
+				nil,
+			},
+		},
+	}
+	props := New(srv, "/org/guelfey/DBus/Test", propsSpec)
+
+	obj := cli.Object(srv.Names()[0], "/org/guelfey/DBus/Test")
+	comparePropValue(obj, "FooStruct", foo, t)
+	comparePropValue(obj, "FooStructPtr", *fooPtr, t)
+	comparePropValue(obj, "SliceOfFoos", foos, t)
+
+	yoo := Foo{Id: 2, Value: "Second"}
+	yooPtr := &Foo{Id: 2, Value: "2nd"}
+	yoos := make([]Foo, 2)
+	yoos[0] = Foo{Id: 3, Value: "Threes"}
+	yoos[1] = Foo{Id: 4, Value: "Fours"}
+	obj.SetProperty("org.guelfey.DBus.Test.FooStruct", dbus.MakeVariant(yoo))
+	obj.SetProperty("org.guelfey.DBus.Test.FooStructPtr", dbus.MakeVariant(yooPtr))
+	obj.SetProperty("org.guelfey.DBus.Test.SliceOfFoos", dbus.MakeVariant(yoos))
+	comparePropValue(obj, "FooStruct", yoo, t)
+	comparePropValue(obj, "FooStructPtr", *yooPtr, t)
+	comparePropValue(obj, "SliceOfFoos", yoos, t)
+
+	props.SetMust("org.guelfey.DBus.Test", "SliceOfFoos", foos)
+	comparePropValue(obj, "SliceOfFoos", foos, t)
+
+	zoo := Foo{Id: 3, Value: "Third"}
+	zooPtr := &Foo{Id: 3, Value: "3th"}
+	zoos := make([]Foo, 2)
+	zoos[0] = Foo{Id: 5, Value: "Sevens"}
+	zoos[1] = Foo{Id: 6, Value: "Sixes"}
+	obj.SetProperty("org.guelfey.DBus.Test.FooStruct", dbus.MakeVariant(zoo))
+	obj.SetProperty("org.guelfey.DBus.Test.FooStructPtr", dbus.MakeVariant(zooPtr))
+	obj.SetProperty("org.guelfey.DBus.Test.SliceOfFoos", dbus.MakeVariant(zoos))
+	comparePropValue(obj, "FooStruct", zoo, t)
+	comparePropValue(obj, "FooStructPtr", *zooPtr, t)
+	comparePropValue(obj, "SliceOfFoos", zoos, t)
+}