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

Fix: macos status #287

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix: macos status #287

wants to merge 5 commits into from

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Jul 19, 2021

This needs some refinement, but I'm putting it up early.
Follow up to #273

I encountered problems in my own tests related to detecting that the service is not installed.
Pre patch: https://github.com/djdv/go-filesystem-utils/runs/3106784696?check_suite_focus=true#step:7:36
Post patch: https://github.com/djdv/go-filesystem-utils/runs/3108004664?check_suite_focus=true#step:7:58

Still getting a status related failure further down the test so I'll have to see what's going on there.

As-is, this removes the need to check the error string value to determine if it's a stderr-err or something else.
Replacing that logic with a wrapped error value so we can use errors.Is instead.
And changes how we get data from stdio during run, and how we interpret the response from launchctl.

Edit: This was a simplification but still too complicated, the need for this went away in future commits.

In addition, I recognize how hacky this is, and the variable stuttering. Considering there doesn't seem to be a launchd API this is probably as good as it can get.
For the variable naming, I'm going to look at it again later, but might not be able to change it. We're kind of stuck with stderr already having err as its suffix. Suggestions for any of the names is welcomed.
(Also, this incorporates some code from another commit, I might have messed up one of the branches while trying to separate them. Going to review that and submit some others later)

@djdv
Copy link
Contributor Author

djdv commented Jul 20, 2021

This is fun: https://apple.stackexchange.com/questions/284238/why-i-dont-see-all-launchctl-daemons-agents-with-sudo-launchctl-list/284652

I'm going to test an approach where we ask launchctl about the service, and if it's not found, we'll inspect user IDs. If the effective and real IDs differ, we'll try again, launching the launchctl process as the real ID. This should cause it to actually see the service when executing launchctl list $name.
As-is we get this odd behaviour where service install does not work due to privileges, so you need to do sudo service install.
Then this will work as expected service status but not this sudo service status (reports service is not installed).

@djdv
Copy link
Contributor Author

djdv commented Jul 21, 2021

Status is correctly detecting "Not Installed" and "Stopped" now, but I think I managed to break Start().
Going to have to go over what's wrong, I'm probably doing something out of order in service_unix.go in relation to exec and pipes.
* Or my VM is bugging out

service_darwin.go Show resolved Hide resolved
@djdv
Copy link
Contributor Author

djdv commented Jul 22, 2021

I tested in master and was experiencing the same issue around starting the service. It doesn't error, but the service process doesn't actually start.
This seems to be related to how launchd is intended to work and be used. We "load" the service and the system becomes aware of it, but sees no reason to "start" the process unless the plist tells it to on-load, or some other start condition is triggered.
So after we successfully "load" the service, we go to look for it's PID, the job's information is printed out, but without a PID since the service is "loaded" not "started".

It may be such that we just need to check if the service does not have a start condition, and manually start it ourselves (immediately) after loading it.
The only issue with this is that "start" and "stop" are listed as debugging commands in the launchctl manual. But then again, "load" and "unload" are listed under the same "legacy subcommands" section anyway.
It seems like everything around this is subject to break at Apple's discretion in future releases. Cool.

In addition I'm uncomfortable with this sudo approach. It certainly works, however I'm now aware that it might be possible to just search for services under another ID. So if we do something like sudo launchctl list $serviceName, and don't find it, we can check if we're impersonating a user, get our real user ID, and do launchctl list user/$UID/$serviceName and/or launchctl list gui/$UID/$serviceName.
This might work, I'll have to test it for services flagged as "user services". If it does I'll switch to that rather than invoking sudo launchctl from within Status. I want to avoid calling sudo at all costs even if it's in a controlled manner.

Edit: Thankfully launchctl asuser $UID exists, which should be safer. I missed this earlier.
Trying to use gui/$UID and user/$UID domain targets does not seem to be as consistent as either asuser or sudo was.

service_darwin.go Outdated Show resolved Hide resolved
@djdv
Copy link
Contributor Author

djdv commented Jul 22, 2021

59e574c works fine, but userID management is not our responsibility.
Someone might want this but it shouldn't go into mainline.
This was only written in the first place because of a combination of the problems mentioned above.
Like assuming the process was running, but not able to be seen when exec'd via sudo when in reality it was only loaded (not running), and thus failing our "isRunning" check.
Now Status() should work as expected as long as the process is started by the same user that called Start(), regardless of which user.

This passes tests for me locally, but I need to rebase a collection of patches and run them on the CI. Will flip this over to "ready" afterwards.

service_unix.go Outdated Show resolved Hide resolved
@djdv djdv marked this pull request as ready for review July 22, 2021 16:52
@djdv
Copy link
Contributor Author

djdv commented Jul 22, 2021

Works on my machine seal of approval: https://github.com/djdv/go-filesystem-utils/actions/runs/1056920024

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.

None yet

1 participant