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

[Christian Franke]

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/