Hi Javier, Please find attached a revised patch that I propose for this NMU. It's a bit more invasive that what I would usually feel comfortable including in an NMU, so I won't upload without your approval.
On Tue, 2006-02-14 at 12:42 +0100, Javier Fernández-Sanguino Peña wrote: > Why doesn't --make-pidfile work? start-stop-daemon creates the pidfile, but portreserve forks after this as it becomes a daemon. The pidfile then points to the non-existent parent process. > * /var/run should not be harcoded, it should be #DEFINEd (that way the > patch is usable for other distros) Agreed, fixed. > * it should return an integer to determine if it is succesful or not (the > main program could, or could not, care about the pid file > creation and check the return value) Agreed, fixed. > * there's a buffer overflow if 'fname' is longer than 512 chars. buf should > *not* be of a static size There is no buffer overflow, the worst would be the pidfile name is truncated, we are using snprintf not sprintf. However I have added further error checking to handle the (unlikely) case of the pidfile name exceeding 512 characters. Note that that case is (and was in the previous patch) not possible in this instance of the code as do_pid is only ever called with NULL as its first argument. > * it should refuse to run (or create a pidfile) if the pidfile already > exists. <snip> Agreed - improved pidfile handling is now in the patch. > I suggest you reuse a pidfile creation function from some other tested > program, there should be plenty of examples around. Unfortunately most are heavily integrated with the specified application and its error logging routines etc. This is why we originally developed the put_pid function that I included in the original patch, it acts as a library function within my group of friends. I've now updated and improved it based on your (much appreciated) comments on its deficiencies. > I don't like the patch to portreserve.c and I object to this NMU. You *can* > NMU, however, to include the changes to the init.d script. Are you happier with this revised patch? Thanks for your time. Regards -- Matt Brown [EMAIL PROTECTED] Mob +64 21 611 544 www.mattb.net.nz
diff -ur portreserve-0.0.0/debian/changelog portreserve-0.0.0-matt/debian/changelog
--- portreserve-0.0.0/debian/changelog 2006-02-12 11:07:31.000000000 +0000
+++ portreserve-0.0.0-matt/debian/changelog 2006-02-15 12:04:55.000000000 +0000
@@ -1,3 +1,16 @@
+portreserve (0.0.0-2.1) unstable; urgency=low
+
+ * Non-maintainer upload
+ * Fixed minor init script bugs (Closes: #352103)
+ - Use -z instead of -n to test list of service files
+ - Use $NAME instead of the undefined $prog in the pidfile name
+ * Reworked portreserve pidfile handling
+ - Check for existance of pidfile on startup, fail if already running
+ - Create pidfile on startup
+ - Remove pidfile when program exits cleanly
+
+ -- Matt Brown <[EMAIL PROTECTED]> Thu, 16 Feb 2006 01:02:03 +1300
+
portreserve (0.0.0-2) unstable; urgency=low
* Added xmlto to Build-Depends (Closes: #337848)
diff -ur portreserve-0.0.0/debian/portreserve.init portreserve-0.0.0-matt/debian/portreserve.init
--- portreserve-0.0.0/debian/portreserve.init 2006-02-12 11:07:31.000000000 +0000
+++ portreserve-0.0.0-matt/debian/portreserve.init 2006-02-15 11:43:34.000000000 +0000
@@ -11,7 +11,7 @@
test -f $DAEMON || exit 0
NAME=`basename $DAEMON`
-PIDFILE=/var/run/$prog.pid
+PIDFILE=/var/run/$NAME.pid
running()
{
@@ -22,8 +22,8 @@
# No pid, probably no daemon present
[ -z "$pid" ] && return 1
[ ! -d /proc/$pid ] && return 1
- cmd=`cat /proc/$pid/cmdline | tr "\000" "\n"|head -n 1 |cut -d : -f 1`
-
+ cmdline=`cat /proc/$pid/cmdline | tr "\000" "\n"|head -n 1 |cut -d : -f 1`
+ cmd=`basename $cmdline`
[ "$cmd" != "$NAME" ] && return 1
return 0
}
@@ -36,7 +36,7 @@
if [ ! -d /etc/portreserve ] ; then
return 1
fi
- if [ -n "`find /etc/portreserve \! -name "*~" -a \! -name "*.*" -type f`" ] ; then
+ if [ -z "`find /etc/portreserve \! -name "*~" -a \! -name "*.*" -type f`" ] ; then
return 1
fi
return 0
Only in portreserve-0.0.0-matt/: .deps
Only in portreserve-0.0.0: portreserve.spec
diff -ur portreserve-0.0.0/src/portreserve.c portreserve-0.0.0-matt/src/portreserve.c
--- portreserve-0.0.0/src/portreserve.c 2003-09-03 14:12:52.000000000 +0000
+++ portreserve-0.0.0-matt/src/portreserve.c 2006-02-15 12:06:27.000000000 +0000
@@ -64,7 +64,13 @@
# include <unistd.h>
#endif
+#include <signal.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
#define UNIX_SOCKET "/var/run/portreserve/socket"
+#define PIDFILE_DIR "/var/run"
struct map {
struct map *next;
@@ -265,8 +271,76 @@
}
int
+do_pid(char *fname, int check, int unlink_pid)
+{
+ char *defname = "portreserve";
+ char buf[512];
+ FILE *fp=NULL;
+ int fd, rv;
+ long int pid;
+
+ /* Determine pidfile name */
+ if( fname == NULL ) {
+ fname = defname;
+ rv = snprintf( &buf[0], 512, "%s/%s.pid", PIDFILE_DIR, fname );
+ } else {
+ rv = snprintf( &buf[0], 512, "%s", fname );
+ }
+ if (rv >= 512)
+ return -1;
+ if (unlink_pid) {
+ /* Remove the pidfile */
+ unlink(buf);
+ return 0;
+ }
+ if (check) {
+ /* Check if the pidfile already exists */
+ if (access (buf, F_OK) == -1) {
+ return 0;
+ }
+ /* Check it is not stale */
+ fp = fopen(&buf[0], "r");
+ if (!fp)
+ return -1;
+ if (fscanf(fp, "%ld", &pid)!=1)
+ return -1;
+ if (kill(pid,0)==0 || errno != ESRCH) {
+ /* Process in pidfile exists */
+ return -1;
+ }
+ /* Stale pidfile, ok to overrwite */
+ return 0;
+ }
+ /* Create a new pidfile */
+ fd=creat(&buf[0],0660);
+ if (fd<0) {
+ return -1;
+ }
+ if (snprintf(&buf[0],512,"%i\n",getpid())>=512) {
+ printf("failed to create pid string\n");
+ close(fd);
+ return -1;
+ }
+ if (write(fd,buf,strlen(&buf[0])) != (signed)strlen(&buf[0])) {
+ printf("failed to write pidstring\n");
+ close(fd);
+ return -1;
+ }
+ close(fd);
+ return 0;
+}
+
+void
+handle_sigterm (int sig)
+{
+ do_pid(NULL, 0, 1);
+ exit(0);
+}
+
+int
main (int argc, char **argv)
{
+ int rv;
const char *p = strrchr (argv[0], '/');
if (!p++)
p = argv[0];
@@ -278,7 +352,9 @@
r += portrelease (argv[i]);
return r;
}
-
+
+ signal (SIGTERM, handle_sigterm);
+
if (argc > 1) {
if (!strcmp (argv[1], "-d"))
debug = 1;
@@ -288,6 +364,9 @@
"what this program does.");
}
+ if (do_pid(NULL, 1, 0)<0)
+ error(EXIT_FAILURE, errno, "Pidfile already exists!");
+
if (!debug) {
switch (fork ()) {
case 0:
@@ -302,7 +381,12 @@
close (STDOUT_FILENO);
close (STDERR_FILENO);
setsid ();
+ if (do_pid(NULL, 0, 0)<0)
+ error (EXIT_FAILURE, errno,
+ "Failed to write pidfile!");
}
- return portreserve ();
+ rv = portreserve();
+ do_pid(NULL, 0, 1);
+ return rv;
}
signature.asc
Description: This is a digitally signed message part
