X-Spam-Check-By: sourceware.org
Date: Mon, 21 Nov 2005 16:30:16 +0100
From: Corinna Vinschen <corinna-cygwin AT cygwin DOT com>
To: cygwin AT cygwin DOT com
Subject: Re: Suggest cygrunsrv extension: --pidfile option (patch included)
Message-ID: <20051121153016.GI2999@calimero.vinschen.de>
Reply-To: cygwin AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
References: <4380AB2E DOT 7010302 AT t-online DOT de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4380AB2E.7010302@t-online.de>
User-Agent: Mutt/1.4.2i
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Precedence: bulk
List-Unsubscribe: <mailto:cygwin-unsubscribe-archive-cygwin=delorie DOT com AT cygwin DOT com>
List-Subscribe: <mailto:cygwin-subscribe AT cygwin DOT com>
List-Archive: <http://sourceware.org/ml/cygwin/>
List-Post: <mailto:cygwin AT cygwin DOT com>
List-Help: <mailto:cygwin-help AT cygwin DOT com>, <http://sourceware.org/ml/#faqs>
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
Delivered-To: mailing list cygwin AT cygwin DOT com

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.

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.

 
  @@ -1461,8 +1499,19 @@
   
     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.  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.  Bottom line, shouldn't the existence
of the pid file result in complaining with LOG_ERR and exiting?

  +	    {
  +	     /* Daemon has fork()ed successfully, wait for pidfile */
  +	     int read_pid;
  +	     for (i = 1; i <= 30; i++) {
  +	       if ((read_pid = read_pidfile (pidfile_path, forktime)) != -1)
  +		 break;
  +	       syslog (LOG_INFO, "service `%s': waiting for file %s (#%d)",
  +		 svcname, pidfile_path, i);
  +	       sleep(1);
  +	     }

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.

  +	     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.

  -	    case ECHILD:
  +	    case ECHILD: case ESRCH:

I'd rather have this on two lines.

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

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

  +static int
  +pid_to_winpid(pid_t pid)
  +{
  +  char path[100];
  +  sprintf(path, "/proc/%d/winpid", pid);
  +  FILE * f = fopen(path, "r");
  +  if (!f)
  +    return -1;
  +  int winpid = -1;
  +  fscanf(f, "%d", &winpid);
  +  fclose(f);
  +  return winpid;
  +}
  +
  +#else // NO_PROC
  +
  +// Another approach without using /proc
  +#include <psapi.h> // EnumProcesses(), link with -lpsapi
  +#include <sys/cygwin.h> 
  +
  +static int
  +pid_to_winpid(pid_t pid)
  +{
  +  DWORD winpids[1000], nb;
  +  if (!EnumProcesses(winpids, sizeof(winpids), &nb))
  +    return 0;
  +  int n = nb / sizeof(DWORD);
  +
  +  int olderrno = errno;
  +  int winpid = -1;
  +  for (int i = 0; i < n; i++) {
  +    if (cygwin_winpid_to_pid(winpids[i]) == pid) {
  +      winpid = winpids[i];
  +      errno = olderrno;
  +      break;
  +    }
  +  }
  +  return winpid;
  +}

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


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


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat, Inc.

--
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/