Details

Reviewers

markj
otis
kevans
michaelo
bapt

Summary

I needed to get the pid of a process started by daemon(8) but without using its supervising functionality of --child-pidfile (and therefore without the need of having daemon(8) hang around as another process). This comes handy when you want to switch the user but can't use su(1) (e.g. because the user has a negative listing in /etc/login.access).

This patch adds an option "-x file" to accomplish this (see also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=280487)

It also fixes an internal comment regarding which options enable Supervision mode.

(Note: This is my first time using reviews.freebsd.org so please be lenient towards me :-))

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mail_fbsd2.e4m.org created this revision.Aug 17 2024, 9:20 AM

Herald added a subscriber: imp. · View Herald TranscriptAug 17 2024, 9:20 AM

mail_fbsd2.e4m.org requested review of this revision.Aug 17 2024, 9:20 AM

When I think of it more closely, couldn't it be modified to do something like --no-supervise or --execute-only and use -p to get the command's pid file in child-pidfile? I'm just trying to avoid duplicate functionality.

Hmm, what will happen if one uses --no-supervise (with or even w/o -p) but has given other
options which need supervising? Should we silently ignore --no-supervise? Or spit out a
warning or even an error?
Not thinking about the code which has to honour --no-supervise when finished parsing all
options and then has to decide what to do...

So how shall we proceed with this? While I am not happy with the --no-supervise idea (because of
the confusion it might produce) I could make a patch which

Adds --no-supervise (or whatever we want to call it)
decides what to do with it when option processing has finished

In this case: How shall we call it? Do we want a short option for it?

patmaddox added a subscriber: patmaddox.Sep 19 2024, 5:10 PM

Please upload diffs with full context (-U999999) or use git-arc from devel/freebsd-git-devtools going forward- it helps to be able to expand files a little bit.

The manpage also needs to note that the pidfile isn't locked like it is with -p -- the lock gets dropped after we exec the child. This also has implications for preventing concurrent instances: daemon(8) relies on the pidfle(3) API and the pidfile(3) API relies on advisory locks -- if the file existed but was not locked, we'll just truncate it rather than trying to investigate if the pid was valid for another instance of the same daemon (since that's hard or impossible to do reliably). This is all a good reason for the current pidfile option forcing supervisor mode, since the value drops down to whatever rc provides as a wrapper around pidfiles if we don't hold it locked.

usr.sbin/daemon/daemon.8
170 ↗	(On Diff #142129)	New sentence should start on new line
173 ↗	(On Diff #142129)	Ditto

Herald added a subscriber: ziaee. · View Herald TranscriptAug 8 2025, 7:16 PM

kevans added a reviewer: kevans.Aug 8 2025, 7:16 PM

Thanks for the input. I've changed the manpage -- hopefully addressing all issues correctly.
But just to be sure I understood things properly: I should upload a diff with 1260 lines
containing just 30 lines of changes?

In D46313#1190917, @mail_fbsd2.e4m.org wrote:

Thanks for the input. I've changed the manpage -- hopefully addressing all issues correctly.
But just to be sure I understood things properly: I should upload a diff with 1260 lines
containing just 30 lines of changes?

Correct- Phab will hide all of the unchanged lines, but it gives us the opportunity to unhide those bits if we need to for review purposes.

Updated version, hopefully adressing kevans input.

Can you move the unrelated doc changes to a separate review, @ziaee and me can easily review and merge them?

This somewhat reminds of of systemd's oneshot type. I agree with other committers against introducing another PID file, we should have an option doing the oneshot, execute-onl, etc. and the manpage shall say which options are ignored in such a case.

In D46313#1264717, @michaelo wrote:

Can you move the unrelated doc changes to a separate review,

Negative, negative! One logical change includes doc, we want to do everything we can to try and include people to 1. write doc 2. include it with code changes.

@ziaee and me can easily review and merge them?

Thanks for tagging me! Happy to help with manuals anytime :)

usr.sbin/daemon/daemon.8
38 ↗	(On Diff #160933)	Any reason not to insert these moving towards roughly alphabetical ordering? It really helps people find things.

In D46313#1264734, @ziaee wrote:

In D46313#1264717, @michaelo wrote:

Can you move the unrelated doc changes to a separate review,

Negative, negative! One logical change includes doc, we want to do everything we can to try and include people to 1. write doc 2. include it with code changes.

@ziaee and me can easily review and merge them?

Thanks for tagging me! Happy to help with manuals anytime :)

I was referring solely to this comment:

It also fixes an internal comment regarding which options enable Supervision mode.

This, I believe, can be done separately, no?

In D46313#1264737, @michaelo wrote:

It also fixes an internal comment regarding which options enable Supervision mode.

This, I believe, can be done separately, no?

Oh, sorry, ETOOSLEEPY. Yes, and that's much better because then even if the features don't get backported, we can backport the documentation fix!

mail_fbsd2.e4m.org mentioned this in D55325: Add "--output-file -o" to list of options enabling supervision mode in daemon.c.Feb 17 2026, 8:05 AM

In D46313#1264720, @michaelo wrote:

This somewhat reminds of of systemd's oneshot type. I agree with other committers against introducing another PID file, we should have an option doing the oneshot, execute-onl, etc. and the manpage shall say which options are ignored in such a case.

If have made a patch which adds the "--execute-only -x" option. If it is given together with -p, no supervision will happen.
If other options needing the supervision feature are given, -x will be not be honoured. I think this is better than ignoring all those options.

Shall I put the diff here or in a new revision?

New version which adds the "--execute-only -x" option. If it is given together with -p, no supervision will happen.
If other options needing the supervision feature are given, -x will be not be honoured. If it is desired to spit out
a warning in this case, I will add it.

I'll come up with an update to daemon.8 when there is consensus to go this way...

Change behaviour if -x was given w/o -p. Now emit an error end exit (as it happens if other bad options were supplied).

michaelo mentioned this in rGe9a69948a883: daemon(8): Add "--output-file -o" to list of options enabling supervision mode.Apr 12 2026, 4:30 PM

michaelo mentioned this in rG5aa751c6556f: daemon(8): Add "--output-file -o" to list of options enabling supervision mode.Apr 15 2026, 8:46 AM

michaelo mentioned this in rG2620eb7a3d42: daemon(8): Add "--output-file -o" to list of options enabling supervision mode.

michaelo mentioned this in rGc80f52565bc8: daemon(8): Add "--output-file -o" to list of options enabling supervision mode.Apr 15 2026, 8:49 AM

I am fine with this change, but I am not an official src comitter. @bapt I think this is also covered as oneshot with your proposed rcd(8).

This revision is now accepted and ready to land.May 24 2026, 5:48 PM

michaelo added a reviewer: bapt.May 24 2026, 5:48 PM

Folks, anyone willing to accept formally? I can't merge it by myself since I don't have src commit bit.

This version seems to have dropped the manpage revisions entirely

In D46313#1328429, @kevans wrote:

This version seems to have dropped the manpage revisions entirely

Indeed, I was sure that it was present. Let me investigate.

Since the man page changes got lost somehow (probably my fault) this is a new
attempt to hopefully do everything correctly...

This revision now requires review to proceed.Tue, Jul 21, 4:11 PM

kevans added inline comments.Tue, Jul 21, 4:55 PM

usr.sbin/daemon/daemon.8

213–231 ↗

(On Diff #182388)

Reading this fresh, I think i'd suggest:

Do not supervise the child process when using
.Fl p .
This option is useful to run the given program as another user when
.Xr su 1
can't be used but a file holding the pid is needed.
.Pp
If the
.Fl -child-pidfile
option is specified, then the
.Ar child_pidfile
will not be locked and the contents may be silently truncated by
a concurrent daemon(8) invocation.

maybe?

usr.sbin/daemon/daemon.c

129

Not your problem, but I think we jumped the shark with sorting somewhere. Some follow-up to re-sort this, shortopts, and maybe related bits below would maybe be a good idea.

mail_fbsd2.e4m.org added inline comments.Tue, Jul 21, 5:27 PM

usr.sbin/daemon/daemon.8

213–231 ↗

(On Diff #182388)

How about:

Do not supervise the child process when using
.Fl p .
This option is useful to run the given program as another user when
.Xr su 1
can't be used but a file holding the pid is needed.
.Pp
The
.Ar child_pidfile
will not be locked and the contents may be silently truncated by
a concurrent daemon(8) invocation.

as it -x can't be used w/o -p anyway...

usr.sbin/daemon/daemon.c

129

By "Some follow-up" you mean another revision or fix it here?

kevans added inline comments.Tue, Jul 21, 6:05 PM

usr.sbin/daemon/daemon.8
213–231 ↗	(On Diff #182388)	Yes, sorry, this makes more sense.
usr.sbin/daemon/daemon.c
129	Another revision, and totally up to you if you want to do it at all.

mail_fbsd2.e4m.org added inline comments.Wed, Jul 22, 9:03 AM

usr.sbin/daemon/daemon.8
213–231 ↗	(On Diff #182388)	OK. I have removed the ".Pp" -- the following paragraph looked somehow lost with it.
usr.sbin/daemon/daemon.c
129	Why not? Might be a good opportunity to learn to use phabricator. But how? style(9) talks about the getopt call: Options should be sorted in the getopt(3) call and the switch statement, unless parts of the switch cascade. But how about cases? All upper case first (corresponding to the ASCII value)? Like this: "CFabcdefgh" Or like this: "abcCdefFgh" Or: "abCcdeFgh" (this latter one corresponds best to the style(9) example). And how about options with operands vs. w/o operands? For the man page style(9) is somehow more specific but does this apply to the getopt() call as well? Shall we do "afglob:c:h:" or "ab:c:fgh:lo"?

mail_fbsd2.e4m.org updated this revision to Diff 182446.Wed, Jul 22, 9:37 AM

daemon(8): Add option to write pidfile w/o supervising it
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 172366

usr.sbin/daemon/daemon.c

daemon(8): Add option to write pidfile w/o supervising itNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 172366

usr.sbin/daemon/daemon.c

daemon(8): Add option to write pidfile w/o supervising it
Needs ReviewPublic
Actions

Revision Contents
Changeset List