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

supervise-daemon: implement output_logger and error_logger. #656

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

Higgs1
Copy link
Contributor

@Higgs1 Higgs1 commented Sep 25, 2023

Implements #644. Allows redirecting process stdout and stderr to another process, just like is already possible with start-stop-daemon. Also addresses #341 and #118.

Almost all of the code is copied directly from s-s-d (#127). I tried to follow your coding style as best I could, I read several previously accepted PRs, and I tried to keep the diff to a minimum.

This works correctly on my machine (Alpine, x86_64) and is currently hosting several of my public services. The stdout and stderr output is successfully redirected to syslog on another machine, which is then emailed to me sometimes.

I didn't copy over the short command arg variants (-3 and -4) because -3 conflicts with supervise-daemon's "--reexec" which doesn't exist in s-s-d. I can edit if you prefer something else.

As an example, here's a snippet from my /etc/init.d file(s):

#!/sbin/openrc-run

supervisor=supervise-daemon
output_logger="logger -et '${RC_SVCNAME}'"
error_logger="logger -et '${RC_SVCNAME}' -p3"

depend() {
	use logger
}

This forwards stdout and stderr to syslog. -p3 sets the priority level to error.

@Higgs1 Higgs1 mentioned this pull request Sep 25, 2023
@tomalok
Copy link

tomalok commented Sep 25, 2023

Implements #644. Allows redirecting process stdin and stdout to another process, just like is already possible with start-stop-daemon. Also addresses #341 and #118.

pretty sure you mean stderr and stdout here, and elsewhere.

@Higgs1
Copy link
Contributor Author

Higgs1 commented Sep 25, 2023

Thank you! I think I've fixed it now. That was such an embarrassing mistake.

Allows redirecting process stdin and stdout to another process,
just like is already possible with start-stop-daemon.

Also added --stdout-logger and --stderr-logger to the man page.
@@ -1,5 +1,5 @@
executable('supervise-daemon',
['supervise-daemon.c', misc_c, plugin_c, schedules_c, usage_c, version_h],
['supervise-daemon.c', '../start-stop-daemon/pipes.c', misc_c, plugin_c, schedules_c, usage_c, version_h],
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if would make sense to move ../start-stop-daemon/pipes.c to shared/pipes.c?

Suggested change
['supervise-daemon.c', '../start-stop-daemon/pipes.c', misc_c, plugin_c, schedules_c, usage_c, version_h],
['supervise-daemon.c', pipes_c, misc_c, plugin_c, schedules_c, usage_c, version_h],

Copy link
Contributor

Choose a reason for hiding this comment

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

diff --git a/src/shared/meson.build b/src/shared/meson.build
index b80b242e..04c70fef 100644
--- a/src/shared/meson.build
+++ b/src/shared/meson.build
@@ -10,6 +10,10 @@ schedules_c = files([
   'schedules.c',
   ])
 
+pipes_c = files([
+  'pipes.c',
+  ])
+
 if selinux_dep.found()
   selinux_c = files([
     'selinux.c',
diff --git a/src/start-stop-daemon/pipes.c b/src/shared/pipes.c
similarity index 100%
rename from src/start-stop-daemon/pipes.c
rename to src/shared/pipes.c
diff --git a/src/start-stop-daemon/pipes.h b/src/shared/pipes.h
similarity index 100%
rename from src/start-stop-daemon/pipes.h
rename to src/shared/pipes.h
diff --git a/src/start-stop-daemon/meson.build b/src/start-stop-daemon/meson.build
index d363ff94..6cae791d 100644
--- a/src/start-stop-daemon/meson.build
+++ b/src/start-stop-daemon/meson.build
@@ -1,5 +1,5 @@
 executable('start-stop-daemon',
-  ['start-stop-daemon.c', 'pipes.c', misc_c, schedules_c,
+  ['start-stop-daemon.c', pipes_c, misc_c, schedules_c,
        selinux_c, usage_c, version_h],
   c_args : [cc_audit_flags, cc_branding_flags, cc_pam_flags, cc_cap_flags, cc_selinux_flags],
   link_with: [libeinfo, librc],

With the addition of logger process redirect in supervise-daemon,
pipes.c and pipes.h are now included in both s-s-d and supervise-daemon.
Thus it makes sense to move the source files to the src/shared dir.
@Higgs1
Copy link
Contributor Author

Higgs1 commented Sep 26, 2023

Hi @ncopa , I'm a big fan enjoyer of Alpine Linux.

I was originally trying to keep the PR to a minimum, but I agree that moving it to the "shared" directory makes a lot more sense. This is the first PR I've submitted to a project of uh... this caliber, so I'm still learning all the ins and outs. I've updated it with your suggestion, thank you!

@Higgs1 Higgs1 requested a review from ncopa October 1, 2023 17:39
@williamh williamh merged commit cf9286d into OpenRC:master Oct 3, 2023
5 checks passed
@Higgs1 Higgs1 deleted the supervise-daemon-logger branch October 12, 2023 20:09
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.

4 participants