This is the mail archive of the cygwin mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Suggest cygrunsrv extension: --pidfile option (patch included)


Corinna Vinschen wrote:

On Nov 20 17:58, Christian Franke wrote:


Hi,

when porting new daemons to Cygwin, it is necessary to add a Cygwin specific option to prevent fork()ing.
Otherwise, running as service via cygrunsrv would not be possible.


For daemons which are able to write /var/run/daemon.pid files, this pid can be used to track the daemon.

Suggest adding a --pidfile option to cygrunsrv for this purpose:

cygrunsrv -I syslogd --pidfile /var/run/syslog.pid -p /usr/sbin/syslogd

(Yes, "-a -D" is missing)


For a working prototype, try { this->patch->here; }


http://franke.dvrdns.org/cygwin/cygrunsrv-pidfile-patch.txt

Note that the patch contains a new module with a waitanypid() function.
This was necessary (tell me if I missed something) because waitpid() cannot wait for child's childs.


Thanks for any comment



I like the idea and I would like to incorporate this change into cygrunsrv, but I have some nits.

First, and MOST important ;-), you missed to add a ChangeLog entry.
Please create one.



OK.



For the other nits I have to refer to your patch, which I partly
inline here:


@@ -1265,7 +1281,7 @@
#undef err_out
#undef err_out_set_error
-int server_pid;
+int terminate_pid;


I don't see a reason to change the name of this variable. server_pid is
still correct, so I'd rather keep it.



No, the variable now contains the parameter for the kill() in terminate child:
No pidfile: -pid (to kill pgrp as before)
With pidfile: pid (to kill single process, see below)


Because the variable can no longer be used as before in service_main(), I changed the name.


report_service_status ();
+ /* Save time if old pid file exists */
+ time_t forktime = 0;
+ if (pidfile_path && !access(pidfile_path, 0))
+ {
+ syslog (LOG_INFO, "service `%s': old pid file %s exists",
+ svcname, pidfile_path);
+ forktime = time(0);
+ sleep(1);
+ }
+


How do you save time here?!? This looks confusing.


Time is saved if old pidfile exists, read_pid_file() will later accept only files newer than this timestamp.
This should work to detect stale pid files.



If an old pid file
exists, then either the (or some) service process is still running, or
the service died without removing the pid file. Some processes also refuse
to start if a pid file still exists.



I agree that error handling in incomplete here.
(But I think we have similar issues in the initd stuff of several *ix favors ;-)


This first approach assumes that an old process is no longer running.
This should be the case if the daemon is only started under the control of SCM+cygrunsrv
(and service is installed only once of course)



Bottom line, shouldn't the existence
of the pid file result in complaining with LOG_ERR and exiting?



It depends, 33.3% of my initial test cases (syslogd, xinetd, smartd) would not work in this case ;-)


Cygwin syslogd *always* leaves a /var/run/syslog.pid behind.

As an alternative, /proc could be scanned for existing processes at least if pidfile exists.


[...]

I'm missing a call to report_service_status here. The dwWaitHint member is
set to 5 seconds, but this code waits up to 30 seconds without calling
report_service_status. Looks like potential trouble.



Agree, I simply forgot this. Same issue in the fork() wait loop before.



+ if (read_pid != -1) + {
+ /* Got the real pid, daemon now running */
+ terminate_pid = read_pid; /* terminate this pid (pgrp unknown) */


Instead of using the process pid, why not retrieving the pgid of the service process from /proc/$pid/pgid, same as you get the winpid?
This would also allow to keep the terminate_child function untouched.



No, this was intentional, yes, the comment is misleading here.
Daemons should be designed to receive signals only on the "exposed" pid, and act accordingly for their child processes.


With no pidfile, sending signal to pgrp is OK.


 -	    case ECHILD:
 +	    case ECHILD: case ESRCH:

I'd rather have this on two lines.



OK;-)


+//
+// Convert Cygwin pid into Windows pid
+// Returns windows pid, or -1 on error
+//

Could you please convert all comments to plain C /* */ comments?



Hmm... this is a C++ module with plain C++ comments ;-)
Today, I consider "// ..." also as plain C comments, these are part of C99 (AFAIK).


But I change this to good old K&R style if you want...


[...]

I think I'd prefer the variation reading /proc. It's more Cygwin :-)



OK.



That's all. Did I mention that I really like this patch?



Thanks!!-)



Christian



-- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]