dbus: add SetPropertiesSubscriber method #248

dtnaylor · 2018-01-31T01:52:07Z

This PR adds a SetPropertiesSubscriber method which is similar to
SetSubStateSubscriber but with two important differences:

Each update includes all changed properties, not just SubState
Each set of property values from systemd is reported; no update can be
missed. With SubStateSubscriber, transient states can be missed
because sendSubStateUpdate calls GetUnitPathProperties after receiving
the original signal from systemd; by this point, the unit's state may
have changed again. (This behavior is clearly documented, but for some
use cases it might not be acceptable to miss a state transition.)

lucab · 2018-01-31T15:11:18Z

dbus/dbus.go

+	for i := 0; i < len(path); i++ {
+		c := path[i]
+		if c == '_' {
+			res, err := hex.DecodeString(path[i+1 : i+3])


Before looking ahead, please check that i+3 does not overflow len(path).

lucab · 2018-01-31T15:12:53Z

dbus/dbus.go

 }

+// PathBusUnescape is the inverse of PathBusEscape.
+func PathBusUnescape(path string) string {


Do you need this to be public in order to use it in your consuming application? If not, I'd prefer to keep it private for the moment, we can always export it later if required.

Nope, will make private.

lucab · 2018-01-31T15:20:17Z

dbus/subscription.go

 			case "org.freedesktop.DBus.Properties.PropertiesChanged":
 				if signal.Body[0].(string) == "org.freedesktop.systemd1.Unit" {
 					unitPath = signal.Path
+					c.sendPropertiesUpdate(unitPath, signal.Body[1].(map[string]dbus.Variant))


Please add a len(signal.Body) >= 2 check and a type-check for signal.Body[1].(map[string]dbus.Variant).

lucab · 2018-01-31T15:22:29Z

dbus/subscription.go

+	case c.propertiesSubscriber.updateCh <- update:
+	default:
+		select {
+		case c.propertiesSubscriber.errCh <- errors.New("update channel full!"):


"update channel is full", no need for exclamation marks 😃 .

lucab · 2018-01-31T15:25:24Z

dbus/subscription.go

+		select {
+		case c.propertiesSubscriber.errCh <- errors.New("update channel full!"):
+		default:
+		}


If the linter doesn't complain about a spurious return (I didn't try), perhaps consider adding an explicit return after the inner select block. That way, code eventually added at the bottom is already fine with error handling.

lucab · 2018-01-31T15:29:06Z

Thanks for your PR, I left a few comments. This will probably conflict with #249, thus depending on timings you may need to rebase at some later point.

Can you add at least a simple test to exercise SetPropertiesSubscriber codepath?

dtnaylor · 2018-02-01T00:40:39Z

Ok, those fixups should address your comments. I'm happy to squash and rebase once #249 is in if this looks good.

lucab · 2018-02-01T10:16:23Z

Thanks, I'm fine with this. Just squash and rebase your commits, I'll take care of merging this before #249 and then solve conflicts on my side.

Stale review, dismissing.

This commit adds a SetPropertiesSubscriber method which is similar to SetSubStateSubscriber but with two important differences: * Each update includes all changed properties, not just SubState * Each set of property values from systemd is reported; no update can be missed. With SubStateSubscriber, transient states can be missed because sendSubStateUpdate calls GetUnitPathProperties after receiving the original signal from systemd; by this point, the unit's state may have changed again. (This behavior is clearly documented, but for some use cases it might not be acceptable to miss a state transition.)

dtnaylor · 2018-02-01T17:09:49Z

Great—squashed and rebased.

- https://github.com/coreos/go-systemd/compare/v14..v19 - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method - coreos/go-systemd#251 activation: add support for listeners with names - coreos/go-systemd#296 dbus: Fix API break from godbus - coreos/pkg@v3...v4 - no changes in vendored code - https://github.com/godbus/dbus/compare/v3..v5.0.1 - godbus/dbus#89 introduce MakeVariantWithSignature Signed-off-by: Sebastiaan van Stijn <[email protected]>

- https://github.com/coreos/go-systemd/compare/v14..v19 - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method - coreos/go-systemd#251 activation: add support for listeners with names - coreos/go-systemd#296 dbus: Fix API break from godbus - https://github.com/godbus/dbus/compare/v3..v5.0.1 - godbus/dbus#89 introduce MakeVariantWithSignature Signed-off-by: Sebastiaan van Stijn <[email protected]>

lucab previously requested changes Jan 31, 2018

View reviewed changes

dtnaylor force-pushed the property_subscriber branch 2 times, most recently from 11b65cf to 8bcb5df Compare February 1, 2018 00:39

lucab changed the title ~~Systemd unit property subscriber~~ dbus: add SetPropertiesSubscriber method Feb 1, 2018

lucab added the needs/rebase label Feb 1, 2018

lucab mentioned this pull request Feb 1, 2018

login1: add support for session management #242

Merged

dtnaylor added 2 commits February 1, 2018 09:08

dbus: Add unit path unescape function

216b61a

dtnaylor force-pushed the property_subscriber branch from 8bcb5df to 6579259 Compare February 1, 2018 17:08

lucab approved these changes Feb 1, 2018

View reviewed changes

lucab merged commit 90ece6e into coreos:master Feb 2, 2018

lucab added kind/new-api and removed needs/rebase labels May 7, 2018

lucab modified the milestone: v17 May 7, 2018

thaJeztah mentioned this pull request Mar 28, 2019

Update dependencies opencontainers/runc#2029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

dbus: add SetPropertiesSubscriber method #248

dbus: add SetPropertiesSubscriber method #248

Uh oh!

dtnaylor commented Jan 31, 2018

Uh oh!

lucab Jan 31, 2018

Uh oh!

lucab Jan 31, 2018

Uh oh!

dtnaylor Feb 1, 2018

Uh oh!

lucab Jan 31, 2018

Uh oh!

lucab Jan 31, 2018

Uh oh!

lucab Jan 31, 2018

Uh oh!

lucab commented Jan 31, 2018

Uh oh!

dtnaylor commented Feb 1, 2018

Uh oh!

lucab commented Feb 1, 2018

Uh oh!

dtnaylor commented Feb 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dbus: add SetPropertiesSubscriber method #248

dbus: add SetPropertiesSubscriber method #248

Uh oh!

Conversation

dtnaylor commented Jan 31, 2018

Uh oh!

lucab Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

lucab Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

dtnaylor Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

lucab Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

lucab Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

lucab Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

lucab commented Jan 31, 2018

Uh oh!

dtnaylor commented Feb 1, 2018

Uh oh!

lucab commented Feb 1, 2018

Uh oh!

dtnaylor commented Feb 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants