Re: [xen-unstable test] 13461: regressions - FAIL

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Jul 05, 2012 at 11:40:04AM +0100, Ian Jackson wrote:
> Daniel P. Berrange writes ("Re: [Xen-devel] [xen-unstable test] 13461: regressions - FAIL"):
> > Yes, as you say flock() operates on the inode, so if something deletes
> > and recreates the file, future flocks will operate differently. Ideally
> > you should just never rm the files at all.
> > 
> > If you need to 'rm' them, then to avoid this, you must do two things
> > 
> >  - Only 'rm /foo' while holding the lock on /foo
> >  - Record the inode before acquiring the lock. After acquiring the
> >    lock check whether the inode on disk is the same. If not,
> >    release the lock & repeat.
> 
> It seems more logical to me to check the inum of the open fd against
> the file.  Something like this perhaps (untested):

Yes, using the open fd is actually what I should have said.

> 
> diff -r ad08cd8e7097 tools/hotplug/Linux/locking.sh
> --- a/tools/hotplug/Linux/locking.sh	Thu Jul 05 11:00:28 2012 +0100
> +++ b/tools/hotplug/Linux/locking.sh	Thu Jul 05 11:39:59 2012 +0100
> @@ -30,6 +30,7 @@ _setlockfd()
>      done
>      _lockdict[$i]="$1"
>      let _lockfd=200+i
> +    let _lockfile="$LOCK_BASEDIR/$1"
>  }
>  
>  
> @@ -37,13 +38,32 @@ claim_lock()
>  {
>      mkdir -p "$LOCK_BASEDIR"
>      _setlockfd $1
> -    eval "exec $_lockfd>>$LOCK_BASEDIR/$1"
> -    flock -x $_lockfd
> +    # The locking strategy is identical to that from with-lock-ex(1)
> +    # from chiark-utils, except using flock.  It has the benefit of
> +    # it being possible to safely remove the lockfile when done.
> +    local rightfile
> +    while true; do
> +        eval "exec $_lockfd>>$lockfile"
> +        flock -x $_lockfd
> +        # We can't just stat /dev/stdin or /proc/self/fd/$_lockfd or
> +        # use bash's test -ef because those all go through what is
> +        # actually a synthetic symlink in /proc and we aren't
> +        # guaranteed that our stat(2) won't lose the race with an
> +        # rm(1) between reading the synthetic link and traversing the
> +        # file system to find the inum.  Perl is very fast so use that.
> +        rightfile=$( perl -e '
> +            open STDIN, "<&'$_lockfd'" or die $!;
> +            my $fd_inum = (stat STDIN)[1]; die $! unless defined $fd_inum;
> +            my $file_inum = (stat $ARGV[0])[1];
> +            print "y\n" if $fd_inum eq $file_inum;
> +                             ' "$_lockfile" )
> +        if [ x$rightfile = xy ]; then break; fi
> +    done
>  }
>  
>  
>  release_lock()
>  {
>      _setlockfd $1
> -    flock -u $_lockfd
> +    rm "$_lockfile"

I think you still want the 'flock' line here, but have it after the
'rm' line. Otherwise you leave the $_lockfd filehandle open. Yes,
I know the calling script will probably just exit, but it doesn't
hurt to be careful here.

Regards,
Danie.
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|