fix error message when running as non-root

[N-R-K]

currently trying to stop a service as non-root prints:

* ERROR: ${service} stopped by something else

but this is plain wrong, the service isn't stopped and the real reason for failure is due to permission problem. same issue with starting a service.

[N-R-K]

Rough draft, fixes the immediate issue but I didn't analyze it properly to make sure if the logic is wholly correct or not.

[N-R-K]

Noticed this, we might be feeding -1 into rc_waitpid which looks very suspicious to me. More investigation is needed.

[navi-desu]

i swear i had a fix for this somewhere already, though i guess i never merged it

thanks for the mr!

[N-R-K]

One more issue still remaining:

[openrc errmsg-fix]~> rc-service ${service} stop
 * ${service}: failed to acquire lock: Permission denied
[openrc errmsg-fix]~> doas rc-service ${service} stop
 * WARNING: ${service} is already stopped
[openrc errmsg-fix]~> rc-service ${service} status
 * status: stopped

It's kind of a troll to complain to the user about invalid permissions, misleading them to retry with root only to then tell them the service is already stopped - which we could've known without needing root perm as shown in the status call.

[navi-desu]

One more issue still remaining:

[openrc errmsg-fix]~> rc-service ${service} stop
 * ${service}: failed to acquire lock: Permission denied
[openrc errmsg-fix]~> doas rc-service ${service} stop
 * WARNING: ${service} is already stopped
[openrc errmsg-fix]~> rc-service ${service} status
 * status: stopped

It's kind of a troll to complain to the user about invalid permissions, misleading them to retry with root only to then tell them the service is already stopped - which we could've known without needing root perm as shown in the status call.

i think we can just check if *state == desired_state on the != EWOULDBLOCK case -- will try out something in a moment

maybe i'd be nice to see if there's a proper error for permission as well, and check that, so that other errors don't get mixed with "we have no perms but it's stopped so print 'already stopped'"

[N-R-K]

Is there a reason not to just do this? Seems clean:

@@ -795,6 +795,11 @@ svc_stop_check(RC_SERVICE *state)
        if (in_background && !(*state & (RC_SERVICE_STARTED | RC_SERVICE_INACTIVE)))
                exit(EXIT_FAILURE);
 
+       if (*state & RC_SERVICE_STOPPED) {
+               ewarn("WARNING: %s is already stopped", applet);
+               return 1;
+       }
+
        if (exclusive_fd == -1)
                exclusive_fd = svc_lock(applet, !deps);
        if (exclusive_fd == -1) {
@@ -806,11 +811,6 @@ svc_stop_check(RC_SERVICE *state)
        }
        fcntl(exclusive_fd, F_SETFD, fcntl(exclusive_fd, F_GETFD, 0) | FD_CLOEXEC);
 
-       if (*state & RC_SERVICE_STOPPED) {
-               ewarn("WARNING: %s is already stopped", applet);
-               return 1;
-       }
-
        rc_service_mark(applet, RC_SERVICE_STOPPING);

[navi-desu]

Is there a reason not to just do this? Seems clean:

@@ -795,6 +795,11 @@ svc_stop_check(RC_SERVICE *state)
        if (in_background && !(*state & (RC_SERVICE_STARTED | RC_SERVICE_INACTIVE)))
                exit(EXIT_FAILURE);
 
+       if (*state & RC_SERVICE_STOPPED) {
+               ewarn("WARNING: %s is already stopped", applet);
+               return 1;
+       }
+
        if (exclusive_fd == -1)
                exclusive_fd = svc_lock(applet, !deps);
        if (exclusive_fd == -1) {
@@ -806,11 +811,6 @@ svc_stop_check(RC_SERVICE *state)
        }
        fcntl(exclusive_fd, F_SETFD, fcntl(exclusive_fd, F_GETFD, 0) | FD_CLOEXEC);
 
-       if (*state & RC_SERVICE_STOPPED) {
-               ewarn("WARNING: %s is already stopped", applet);
-               return 1;
-       }
-
        rc_service_mark(applet, RC_SERVICE_STOPPING);

the only issue i see really is a possible race, if another process is either STOPPED > STARTING or STOPPING > STOPPED, realistically either case would basically make it throw a wrong error, by checking state without having a lock

however we're already checking for FAILED/STARTED/INACTIVE before getting the lock anyway, so we might as well just do this

[N-R-K]

however we're already checking for FAILED/STARTED/INACTIVE before getting the lock anyway, so we might as well just do this

My thought as well. And also, the state variable never gets updated after acquiring the lock either, so doing it after wasn't really solving the race anyways.

[navi-desu]

thanks!