Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dbus: Add GetUnitByPID #298

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

dbus: Add GetUnitByPID #298

wants to merge 10 commits into from

Conversation

APTy
Copy link

@APTy APTy commented Dec 13, 2018

This implements the org.freedesktop.systemd1.Manager.GetUnitByPID method 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?

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is part of a sentence missing after "Note that".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input parameter should be a uint32 to match DBus signature.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

func assertEqualStr(t *testing.T, shouldBe, target string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is ok to inline this and the helper above directly inside the test function.

@@ -0,0 +1,2 @@
/bin/echo ${PID} >get-unit-pid.pid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a #!/usr/bin/env bash shebang.

Description=get unit pid

[Service]
ExecStart=get-unit-pid.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should specify the full absolute path to the binary.

@lucab
Copy link
Contributor

lucab commented Dec 14, 2018

Thanks for the PR! Code looks mostly ok.
I think that test is failing because you didn't specify an absolute path for the binary.

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.

@APTy
Copy link
Author

APTy commented Dec 14, 2018

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 GetUnitProperties or GetUnitProperty. Is it "MainPID" that I should be looking for?

@lucab
Copy link
Contributor

lucab commented Dec 15, 2018

@APTy yes, MainPID tells you the pid of the top-level process.

@APTy
Copy link
Author

APTy commented Dec 15, 2018

Got it, thanks. Do you happen to know if it's only available after certain versions of systemd? I tried using GetUnitProperty to look it up (and could find other properties like Id) but couldn't find MainPID. I can follow up with some repro steps but just wanted to check first

@APTy
Copy link
Author

APTy commented Dec 17, 2018

Ah, I got it - needed to use GetServiceProperty not GetUnitProperty, since MainPID is defined on the org.freedesktop.systemd1.Service interface, not org.freedesktop.systemd1.Unit.

Working through a final compatibility issue that seems to appear only on ubuntu:18.04 that results in:

    expected
        "/org/freedesktop/systemd1/unit/_2d_2eslice"
    to equal
        "/org/freedesktop/systemd1/unit/get_2dunit_2dpid_2eservice"

@APTy
Copy link
Author

APTy commented Dec 17, 2018

Seems like the call to GetUnitByPID() on 18.04 is returning the object path to the root slice configuration unit -.slice (http://manpages.ubuntu.com/manpages/bionic/man5/systemd.slice.5.html), looking into this more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants