dbus: Add GetUnitByPID #298
Conversation
dbus/methods.go
Outdated
| @@ -288,6 +288,15 @@ func (c *Conn) listUnitsInternal(f storeFunc) ([]UnitStatus, error) { | |||
| return status, nil | |||
| } | |||
|
|
|||
| // GetUnitByPID returns an array with all currently loaded units. Note that | |||
There was a problem hiding this comment.
There is part of a sentence missing after "Note that".
dbus/methods.go
Outdated
| @@ -288,6 +288,15 @@ func (c *Conn) listUnitsInternal(f storeFunc) ([]UnitStatus, error) { | |||
| return status, nil | |||
| } | |||
|
|
|||
| // GetUnitByPID returns an array with all currently loaded units. Note that | |||
| func (c *Conn) GetUnitByPID(pid int) (dbus.ObjectPath, error) { | |||
There was a problem hiding this comment.
Input parameter should be a uint32 to match DBus signature.
There was a problem hiding this comment.
I agree. I was thinking of using int because its what the go stdlib uses for pids (oddly enough), but I'd prefer to adhere to the dbus signature
dbus/methods_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func assertEqualStr(t *testing.T, shouldBe, target string) { |
There was a problem hiding this comment.
It is ok to inline this and the helper above directly inside the test function.
fixtures/get-unit-pid.sh
Outdated
| @@ -0,0 +1,2 @@ | |||
| /bin/echo ${PID} >get-unit-pid.pid | |||
There was a problem hiding this comment.
This should have a #!/usr/bin/env bash shebang.
fixtures/get-unit-pid.service
Outdated
| Description=get unit pid | ||
|
|
||
| [Service] | ||
| ExecStart=get-unit-pid.sh |
There was a problem hiding this comment.
This should specify the full absolute path to the binary.
|
Thanks for the PR! Code looks mostly ok. If path lookup gets too complex, you can try re-arranging the test so that it just runs a sleeping service. That way you can get the service by name, then lookup its main PID from properties, then use your method with the PID to get the path, and finally verify that it matches with the service property. |
I'm having trouble looking up the pid from properties with |
|
@APTy yes, |
|
Got it, thanks. Do you happen to know if it's only available after certain versions of systemd? I tried using |
|
Ah, I got it - needed to use Working through a final compatibility issue that seems to appear only on |
|
Seems like the call to |
This implements the
org.freedesktop.systemd1.Manager.GetUnitByPIDmethod from https://www.freedesktop.org/wiki/Software/systemd/dbus/The test is a very rough outline - I couldn't get it to run locally. Any tips on that?