Bug#511300: Acknowledgement (Support locking so that concurrent instances of the same backup action not possible)

2009-01-19 Thread intrigeri 
Hi Tuomas, hi BTS!

Tuomas Jormola wrote (13 Jan 2009 12:56:44 GMT) :
 Seems fine to me at the first glance, but... have you checked how
 other programs do so? I bet there is a robust, long-time used piece
 of code somewhere that does exactly this and takes care of the
 usual weird corner case.
 In shell, I guess you're pretty much limited to use what ever
 commands you have at your disposal. ps is the utility to get
 information about running processes, so I don't see there's lots of
 other options...

I'm neither specifically suggesting you to use something other than
ps, nor implicitely saying your code is wrong. I'm pretty sure it does
the Right Thing in most cases. I'm sorry if my wording sounded harsh
or vexating to you.

The thing is, brand new code trying to solve a classic problem often
works right in *most* cases, whereas older, largely used pieces of
code often have been fixed over time to take care of the tricky case
one almost never thinks of when writing new implementations.

Bye,
-- 
  intrigeri intrig...@boum.org
  | gnupg key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
  | We're dreaming of something else.
  | Something more clandestine, something happier.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#511300: Acknowledgement (Support locking so that concurrent instances of the same backup action not possible)

2009-01-13 Thread Tuomas Jormola 
On Mon, Jan 12, 2009 at 10:10:46PM +0100, intrigeri wrote:
 Tuomas Jormola wrote (12 Jan 2009 15:01:03 GMT) :
  On Mon, Jan 12, 2009 at 01:17:59PM +0100, intrigeri wrote:
  As pointed out in your comments, the checkpidalive portability needs
  to be fixed; do you intend to do so at some point?
  Quick check on Linux/AIX/Solaris/Mac OS X systems would suggest that
  ps -A | awk '{print $1}' would give you the list of all the pids running
  on the system. On HP-UX, it's ps -e. So maybe something like this could
  be done (untested, and we should check at least how *BSD ps behaves)
 
  function checkpidalive() {
  local pid=$1
  [ -z $pid ]  return 2
  [ -d /proc/$pid ]  return 0
  local psargs
  local uname=`uname`
  case $uname in
  HP-UX) psargs=-e ;;
  *) psargs=-A ;;
  esac
  ps $psargs | awk '{print $1}' | grep -q ^${pid}$
  return $?
  }
 
 Seems fine to me at the first glance, but... have you checked how
 other programs do so? I bet there is a robust, long-time used piece of
 code somewhere that does exactly this and takes care of the usual
 weird corner case.
In shell, I guess you're pretty much limited to use what ever commands
you have at your disposal. ps is the utility to get information about
running processes, so I don't see there's lots of other options...

-- 
Tuomas Jormola t...@solitudo.net


signature.asc
Description: Digital signature

Bug#511300: Acknowledgement (Support locking so that concurrent instances of the same backup action not possible)

2009-01-12 Thread intrigeri 
Hello,

Tuomas Jormola wrote (09 Jan 2009 11:33:15 GMT) :
 Naive sample implementation of this feature, not tested at all.
 Maybe better use as inspiration rather than concrete
 implementation...

This seems like a good start to implement an important missing
feature. Great :)

As pointed out in your comments, the checkpidalive portability needs
to be fixed; do you intend to do so at some point?

Also, why is the hostname needed in the lock file name?

Bye,
-- 
  intrigeri intrig...@boum.org
  | gnupg key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
  | We're dreaming of something else.
  | Something more clandestine, something happier.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#511300: Acknowledgement (Support locking so that concurrent instances of the same backup action not possible)

2009-01-12 Thread Tuomas Jormola 
On Mon, Jan 12, 2009 at 01:17:59PM +0100, intrigeri wrote:
 Hello,
 
 Tuomas Jormola wrote (09 Jan 2009 11:33:15 GMT) :
  Naive sample implementation of this feature, not tested at all.
  Maybe better use as inspiration rather than concrete
  implementation...
 
 This seems like a good start to implement an important missing
 feature. Great :)
 
 As pointed out in your comments, the checkpidalive portability needs
 to be fixed; do you intend to do so at some point?
Quick check on Linux/AIX/Solaris/Mac OS X systems would suggest that
ps -A | awk '{print $1}' would give you the list of all the pids running
on the system. On HP-UX, it's ps -e. So maybe something like this could
be done (untested, and we should check at least how *BSD ps behaves)

function checkpidalive() {
local pid=$1
[ -z $pid ]  return 2
[ -d /proc/$pid ]  return 0
local psargs
local uname=`uname`
case $uname in
HP-UX) psargs=-e ;;
*) psargs=-A ;;
esac
ps $psargs | awk '{print $1}' | grep -q ^${pid}$
return $?
}

 Also, why is the hostname needed in the lock file name?
Well I thought that the lock directory might be shared among different
hosts that might all run rdiff-backup, and there might be similarily
named configs on these hosts. But if you hear the voice of reason in
your head, you'll notice that there's too many ifs and unnecessary
complexity involved. Just get rid of the hostname in the file name, it's
not needed at all :)

-- 
Tuomas Jormola t...@solitudo.net


signature.asc
Description: Digital signature

Bug#511300: Acknowledgement (Support locking so that concurrent instances of the same backup action not possible)

2009-01-12 Thread intrigeri 
Tuomas Jormola wrote (12 Jan 2009 15:01:03 GMT) :
 On Mon, Jan 12, 2009 at 01:17:59PM +0100, intrigeri wrote:
 As pointed out in your comments, the checkpidalive portability needs
 to be fixed; do you intend to do so at some point?
 Quick check on Linux/AIX/Solaris/Mac OS X systems would suggest that
 ps -A | awk '{print $1}' would give you the list of all the pids running
 on the system. On HP-UX, it's ps -e. So maybe something like this could
 be done (untested, and we should check at least how *BSD ps behaves)

 function checkpidalive() {
   local pid=$1
   [ -z $pid ]  return 2
   [ -d /proc/$pid ]  return 0
   local psargs
   local uname=`uname`
   case $uname in
   HP-UX) psargs=-e ;;
   *) psargs=-A ;;
   esac
   ps $psargs | awk '{print $1}' | grep -q ^${pid}$
   return $?
 }

Seems fine to me at the first glance, but... have you checked how
other programs do so? I bet there is a robust, long-time used piece of
code somewhere that does exactly this and takes care of the usual
weird corner case.

 Also, why is the hostname needed in the lock file name?
 Well I thought that the lock directory might be shared among
 different hosts that might all run rdiff-backup, and there might be
 similarily named configs on these hosts. But if you hear the voice
 of reason in your head, you'll notice that there's too many ifs and
 unnecessary complexity involved. Just get rid of the hostname in the
 file name, it's not needed at all :)

He he. I would not dare sharing /var/lock among several hosts, nor can
see the use of doing so.

Bye,
-- 
  intrigeri intrig...@boum.org
  | gnupg key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
  | Do not be trapped by the need to achieve anything.
  | This way, you achieve everything.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#511300: Acknowledgement (Support locking so that concurrent instances of the same backup action not possible)

2009-01-09 Thread Tuomas Jormola 
Naive sample implementation of this feature, not tested at all. Maybe better
use as inspiration rather than concrete implementation...

-- 
Tuomas Jormola t...@solitudo.net
diff -ur backupninja-0.9.5.orig/etc/backupninja.conf.in backupninja-0.9.5/etc/backupninja.conf.in
--- backupninja-0.9.5.orig/etc/backupninja.conf.in	2007-03-04 12:29:26.0 +0200
+++ backupninja-0.9.5/etc/backupninja.conf.in	2009-01-09 12:50:24.494527275 +0200
@@ -62,6 +62,9 @@
 # where backupninja libs are found
 libdirectory = @pkglibdir@
 
+# where backupninja stores action lock files
+lockdirectory = @localstatedir@/lock
+
 # whether to use colors in the log file
 usecolors = yes
 
diff -ur backupninja-0.9.5.orig/man/backupninja.conf.5 backupninja-0.9.5/man/backupninja.conf.5
--- backupninja-0.9.5.orig/man/backupninja.conf.5	2005-11-19 19:11:28.0 +0200
+++ backupninja-0.9.5/man/backupninja.conf.5	2009-01-09 12:51:00.844527176 +0200
@@ -66,6 +66,10 @@
 .B scriptdirectory 
 Where backupninja handler scripts are found
 
+.TP 
+.B lockdirectory 
+Where backupninja stores action lock files
+
 .TP
 .B usecolors
 If set to 'yes', use colors in the log file and debug output.
diff -ur backupninja-0.9.5.orig/src/backupninja.in backupninja-0.9.5/src/backupninja.in
--- backupninja-0.9.5.orig/src/backupninja.in	2007-10-12 20:42:46.0 +0300
+++ backupninja-0.9.5/src/backupninja.in	2009-01-09 13:29:38.552333945 +0200
@@ -227,6 +227,27 @@
 	return 1
 }
 
+# Returns the hostname in a portable way
+function findhostname() {
+	local hostname=$HOSTNAME
+	[ -z $hostname ]  hostname=`hostname 2/dev/null`
+	if [ -n $hostname ]; then
+		echo $hostname
+		return
+	fi
+	echo localhost
+}
+
+# Return 1 if given PID is running, 0 if not and 2 in case of error.
+# TODO: Only Linux and supported, should support other operating systems
+# by running ps with suitable arguments for the system and parsing the result
+function checkpidalive() {
+	local pid=$1
+	[ -z $pid ]  return 2
+	[ -d /proc/$pid ]  return 0
+	return 1
+}
+
 function usage() {
 	cat  EOF
 $0 usage:
@@ -273,6 +294,41 @@
 	local run=no
 	setfile $file
 
+	# skip over this config if another instance is already running
+	getconf lockdir @localstatedir@/lock/backupninja
+	if ! [ -d $lockdir ]; then
+		if ! mkdir -p $lockdir /dev/null 21; then
+			msg *failed* -- $file
+			errormsg=$errormsg\n== could not create lock directory $lockdir ==\n
+			error  finished action $file: ERROR
+			return
+		fi
+	fi
+	if ! [ -w $lockdir ]; then
+		msg *failed* -- $file
+		errormsg=$errormsg\n== lock directory $lockdir not writable ==\n
+		error  finished action $file: ERROR
+		return
+	fi
+	local hostname=`findhostname`
+	local basename=`basename $file`
+	local lockfile=${lockdir}/${hostname}_${basename}.lock
+	local currentargs=${bash_ar...@]}
+	if [ -e $lockfile ]; then
+		local previouspid=`cat $lockfile | cut -d' ' -f1`
+		local previousargs=`cat $lockfile | cut -d' ' -f2-`
+		checkpidalive $previouspid
+		local previouspidalive=$?
+		if [ $previouspidalive == 0 ]  [ $previousargs == $currentargs ]; then
+			info  skipping action $file because the action is currently running already
+			return
+		else
+			echo $$ $currentargs  $lockfile
+		fi
+	else
+		echo $$ $currentargs  $lockfile
+	fi
+
 	# skip over this config if when option
 	# is not set to the current time.
 	getconf when $defaultwhen


signature.asc
Description: Digital signature