X-Spam-Check-By: sourceware.org
Date: Tue, 22 Nov 2005 10:54:42 +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: <20051122095442.GL2999@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> <20051121153016 DOT GI2999 AT calimero DOT vinschen DOT de> <438221C6 DOT 1080103 AT t-online DOT de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <438221C6.1080103@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 21 20:36, Christian Franke wrote:
> Corinna Vinschen wrote:
> > -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)

It did that before.  The pid is still the pgid or the pid of the server
process.  The name change is somewhat arbitrary.

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

Ouch.  The comment lead me on an entirely wrong track.  I understood
that you were saving *time*, not a *timestamp*.  I didn't see how that
would be possible with this extra test and log entry.

Could you please change the comment to "Save timestamp if ...", maybe
with a reference to the later usage?

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

Sounds interesting, but I think scanning /proc is dangerous, because of
Windows reusing of PIDs a lot.  You would have to test if the process
really belongs to this pidfile, too.

If the service has been started by SCM, we wouldn't be here, since SCM
wouldn't let us start the service twice.  So, when we arrive at this
point, the pid file either indicates a running service process which is
not under control of SCM anymore, or it's just a stale PID file.

If the process still exists, we're a bit in trouble.  In this case the
above log message hopefully helps.  Otherwise, maybe cygrunsrv should
remove the pid file when the service is stopped.  That should help in
the cases were daemons refuse to run when the pid file exists, shouldn't
it?

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

Ok, maybe a more elaborate comment would be enough.

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

Yes, please.  I have no problems with using C++ style for short comments
after an expression, but longer comments or comments on their own line
should rather use the K&R style, like this:

  /* foo */

  /* foo bla ...
     ... blub */


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/