Hi,

the attached patch adds the functionality to start-stop-daemon that makes it capable
of tracking and detecting the PID of programs it starts and which fork themselves. The
manpage entries for the new arguments summarize the feature quite well:

Hi,

First of all, thanks for sending this.

Peter Gyongyosi wrote:
[...]
[...]
TRACEME and only tries once).  For reference, this is imitating the
EXPECT_DAEMON and EXPECT_FORK cases in init/job_process.c.

I guess start-stop-daemon is the right place to implement this kind of
hack, but I'd much rather see daemons providing either a --no-detach
or a --pidfile option.  A serious problem with the ptrace approach is
making it impossible to debug the daemon with "gdb -p" (and that
downside ought to be mentioned in the documentation if this is
adopted).

For your particular application: is it possible to run each service in
a separate cgroup (like systemd does), maybe?  (See
http://0pointer.de/blog/projects/systemd-for-admins-4.html for what
that looks like from the enduser pov.)  What services use "expect =
fork/daemon"?  Maybe they can be fixed (so strace and gdb -p can use
if for no other reason)?

Hmm...

The nested ifs here are hard to read.  It should be safe to drop the
else after handling execvp failure.

Hope that helps,
Jonathan

Hi,
for what upstart does, and obviously, the code there served as an example.

And the "yikes" is also totally deserved, sleeping and retrying the
SETOPTIONS or detaching is not the most beautiful code I've ever
written. For me, however, while being extremely far from being an expert
of the ptrace interface, it seems like this thing is inherently race-y
-- even upstart code contains error handling for such situations, like
this in job_process.c:

        /* We may have already had the wait notification for the new child
         * waiting at SIGSTOP, in which case a ptrace() call will succeed
         * for it.
         */
        if (ptrace (PTRACE_SETOPTIONS, job->pid[process], NULL, 0) < 0) {
                nih_debug ("Failed to set options for new %s %s process
(%d), "
                           "probably not yet forked: %s",
                           job_name (job), process_name (process),
                           job->pid[process], strerror (errno));
                return;
        }

If you can refer me to some good documentation or examples on how to do
this in a better way, I'd be happy to read them and make this code more
robust.

But I do not understand your other remark: I am indeeed using TRACEME,
all other ptrace calls are just to limit the tracing to only forks and
to get the PID we need.
daemons started this way. And it's not just me: upstart does exactly the
same thing for a large part of the services it starts on a standard
Lucid install and I'm pretty sure someone would've recognized if they
couldn't "gdb -p", for example, sshd :) Also, see the comment section in
the blog post about this feature in upstart at
http://netsplit.com/2007/12/07/how-to-and-why-supervise-forking-processes/
, where the author addresses exactly the same issue:

But I also agree that it's better if the daemons themselves can create
their own pidfiles or stay in foreground: if it's added, maybe a WARNING
should be included for this option just as there is one for
--background, telling the user to only use this as a last resort in
special cases.

Hmm, it seems like a viable option, I'll play around with it when I'll
have the time. However, in our particular application the goal was to
make things work with as little modification to the boot process or the
init scripts as possible, as we're using this in a customized distro
using lots of packages and the burden of forking/rewriting them wouldn't
have worth it. But since then, upstart seems to start to get the
functionality we needed (that is, working properly in chroots), so maybe
that'll be the way to go.

What services use expect = fork/daemon? Quite a lot of them, for
example, on my workstation:

gyp@chuck:/etc/init$ egrep "expect (fork|daemon)" *
acpid.conf:expect fork
anacron.conf:expect fork
atd.conf:expect fork
avahi-daemon.conf:expect daemon
cron.conf:expect fork
dbus.conf:expect fork
mountall.conf:expect daemon
network-manager.conf:expect fork
nmbd.conf:expect fork
plymouth.conf:expect fork
rsyslog.conf:expect fork
squid.conf:expect fork
ssh.conf:expect fork
udev.conf:expect fork
upstart-udev-bridge.conf:expect daemon
ureadahead.conf:expect fork
ureadahead-other.conf:expect fork
usplash.conf:expect fork


Most of these services are able to write their own pidfiles, it's just
that upstart does not want to use this feature, probably because it
wants to work independently of the capabilities of the actual service.

I agree, but see my remarks about ptrace being a strange animal above :)

Absolutely right, will do that.

In general, what do you think, is there a point in adding this feature
to upstream start-stop-daemon? We needed this, I wrote this, it's been
tested and seem working fine in our setup, so I thought I'd send it in.
I can see its value outside our specific situation, but I admit these
are very special cases when this might be needed. If yes, what changes
would it need to be accepted? I'm willing to fix some things up and test
the changes, though my resources are quite limited, so it might take a
while...

Thanks for your input,
Peter

Hi again,

Peter Gyongyosi wrote:

Sorry, that was just my sloppiness.  Perhaps renaming the function,
along the lines of s/start_trace/set_trace_options/, would help sloppy
readers like me?

[...]

I have no experience writing ptrace-heavy code myself, but the manual
says the following:

	The value of request determines the action to be performed:

	PTRACE_TRACEME
		Indicates that this process is to be traced by its parent.
		Any signal (except SIGKILL) delivered to this process will
		cause it to stop and its parent to be notified via
		wait(2).  Also, all subsequent calls to execve(2) by this
		process will cause a SIGTRAP to be sent to it, giving the
		parent a chance to gain control before the new program
		begins execution.

Doesn't that mean that a wait() in the parent is the right way to
start tracing?

[...]
| Note that the trace is removed as soon as we've seen the two (or one)
| forks, so it's only there for a very short amount of time.

That addresses my concern about ptrace, yes.
code that so far seems to have used fork() but could change behavior
any second ;-)).  But sure, that's the proper place for this.
[...]

Yep, was just musing.  (Plus if that approach ends up easy and
flexible enough, it could replace this code.)

Very good to hear.
[... 14 other core packages ...]

Wow.  Thanks for checking.

[...]

Guillem would probably be the one to say whether this belongs in
start-stop-daemon.  (Based on "git log", it looks like he has been
maintaining it for the last while.  Incidentally, looks like Scott
James Remnant was taking care of s-s-d for around a year before that.)

We can wait. ;-)  And any interested others are welcome to help.

Thanks again.
Jonathan

Hi!

[...]

Why? If it's because it does not have such option, then the correct
fix is to add it, it should not amount to more than 10-15 lines of C.

Why not fix upstart so that it can work on a chroot? or is that not
possible given its design (which would be odd).

That patch become the current --background and --make-pidfile options,
which to be frank are already bad as they are, as people tend to use
them when they should be adding the code into the daemon properly. So
I'd rather not add more such options, if no really strong reasons are
put forward.

thanks,
guillem

Hi,

Yes, the correct fix would be to add the support to that daemon indeed.
However, adding this support is usually not the difficult part, but
maintaining it is: either you have to push it upstream and wait until it
floats back to the distro or you have to maintain a separate branch
which you have to update and recompile every time it needs to be
upgraded (eg. because of security patches). Although that'd be the
proper solution, it is time-consuming and tedious, while adapting the
init script is quick and can be done/maintained easily until pidfile
support is added to the daemon itself.

I'd say that it wouldn't have been a trivial fix, or at least much more
complicated than adding this hack to S-S-D. Scott James Remnant has
started adding this feature to Upstart, but it's not in  mainline
upstart yet AFAIK:
https://code.launchpad.net/~canonical-scott/upstart/session-support

Well, I think we're looking at this from different points of view :)
You're looking at it -- at least partially -- from an OS
maintainer/creator point of view while I'm looking at it from the system
integrator side. From where you stand, it's totally understandable not
wanting to have yet another tool that package maintainers/developers can
abuse to create hack-ish initscripts instead of solving things properly.
But from the other side, it's often simply not feasible to do the Right
Thing -- you don't want to start fixing complex problems in upstream
packages, you just want a simple but working solution for your problem.
(This was my scenario -- I was simply not willing to spend weeks
understanding/patching/testing upstart to add proper chroot support that
is robust enough to be accepted upstream.)

I have no strong arguments in favor of adding this other than "it's a
feature that might be useful for lots of people". I can understand your
point of view, but I'm not entirely convinced that keeping a feature out
of a tool is the right way to educate people about writing proper,
well-behaved daemons.

greets,
Peter

this patch would be really useful for daemons like udev that does not
create the pidfile (bugs #791944 and #908796)

ciao!