xen 4.4.0 unnecessary dependency on Perl

xen 4.4.0 unnecessary dependency on Perl

[Anthony Wright]

The shell script tools/hotplug/Linux/locking.sh contains a single
section of 8 lines of perl that causes xen 4.4.0 to be dependent on
perl, something that isn't mentioned anywhere in the documentation. The
section of code is trying to deal with a race condition and is
accompanied by the comment "Perl is very fast so use that".

To me it would seem wise to minimise the number of xen's dependencies
and so use Python instead or if speed is really important here, a custom
C program. We have a custom Linux distro that includes xen and to us the
size of the distro is very important. This single 8 line section of code
introduces a dependency on Perl and so adds another 10M to 20M.

Anthony.

Re: xen 4.4.0 unnecessary dependency on Perl

[Ian Campbell]

On Mon, 2014-07-28 at 21:25 +0100, Anthony Wright wrote:

> The shell script tools/hotplug/Linux/locking.sh contains a single
> section of 8 lines of perl that causes xen 4.4.0 to be dependent on
> perl, something that isn't mentioned anywhere in the documentation.
> The section of code is trying to deal with a race condition and is
> accompanied by the comment "Perl is very fast so use that".
> 
> To me it would seem wise to minimise the number of xen's dependencies
> and so use Python instead or if speed is really important here, a custom
> C program. We have a custom Linux distro that includes xen and to us the
> size of the distro is very important. This single 8 line section of code
> introduces a dependency on Perl and so adds another 10M to 20M.

Please send a patch.

Now that xend is gone Python is reasonably optional as well I think
(it's needed for pygrub but not a lot else IIRC). In the absence of any
other suitable facilities in a base system that might point to a custom
C utility being the best approach, but you should probably wait for
other feedback (from Ian Jackson, CCd, in particular) before picking an
approach.

Ian.

Re: xen 4.4.0 unnecessary dependency on Perl

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] xen 4.4.0 unnecessary dependency on Perl"):
> On Mon, 2014-07-28 at 21:25 +0100, Anthony Wright wrote:
> > To me it would seem wise to minimise the number of xen's dependencies
> > and so use Python instead or if speed is really important here, a custom
> > C program. We have a custom Linux distro that includes xen and to us the
> > size of the distro is very important. This single 8 line section of code
> > introduces a dependency on Perl and so adds another 10M to 20M.
> 
> Please send a patch.
> 
> Now that xend is gone Python is reasonably optional as well I think
> (it's needed for pygrub but not a lot else IIRC). In the absence of any
> other suitable facilities in a base system that might point to a custom
> C utility being the best approach, but you should probably wait for
> other feedback (from Ian Jackson, CCd, in particular) before picking an
> approach.

The perl fragment there is comparing inode numbers.  I think that can
be done with stat(1).  For the fd stdin, it can be done with `stat -L
/dev/stdin'.

Ian.

Re: xen 4.4.0 unnecessary dependency on Perl

[Anthony Wright]

----- Original Message -----
> Ian Campbell writes ("Re: [Xen-devel] xen 4.4.0 unnecessary dependency
> on Perl"):
> > On Mon, 2014-07-28 at 21:25 +0100, Anthony Wright wrote:
> > > To me it would seem wise to minimise the number of xen's
> > > dependencies
> > > and so use Python instead or if speed is really important here, a
> > > custom
> > > C program. We have a custom Linux distro that includes xen and to
> > > us the
> > > size of the distro is very important. This single 8 line section
> > > of code
> > > introduces a dependency on Perl and so adds another 10M to 20M.
> >
> > Please send a patch.
> >
> > Now that xend is gone Python is reasonably optional as well I think
> > (it's needed for pygrub but not a lot else IIRC). In the absence of
> > any
> > other suitable facilities in a base system that might point to a
> > custom
> > C utility being the best approach, but you should probably wait for
> > other feedback (from Ian Jackson, CCd, in particular) before picking
> > an
> > approach.
> 
> The perl fragment there is comparing inode numbers. I think that can
> be done with stat(1). For the fd stdin, it can be done with `stat -L
> /dev/stdin'.
> 
> Ian.
Below is a patch that converts the perl script into shell script. I'm new to submitting patches, so I apologise if this is the wrong way to do it.

I believe the patch provides the same functionality as the perl, but it does exactly what the comment says isn't good enough. Having said that from reading around the rest of the locking file and examining the code, I'm inclined to ignore the comment. Perl isn't adding anything from a performance perspective as it's the stat() kernel call where all the work is done, and I can't see any race condition with rm.

If we open the file and get a valid $_lockfd (confirmed as flock succeeding), then while the $_lockfd is open it will always have a valid inode, and it's inode number will always be the same, thus the first stat will always succeed. With the second stat I can't envisage any race condition with rm. If $_lockfile has been removed, the stat() will fail. If it's been removed and re-created, the stat() will succeed, but return a different inode number. If it hasn't been touched then stat() will succeed and return the same inode number as $_lockfd. All these cases are handled, and there is no potential for a race.

Anthony

diff -ur xen-4.4.0.orig/tools/hotplug/Linux/locking.sh xen-4.4.0/tools/hotplug/Linux/locking.sh
--- xen-4.4.0.orig/tools/hotplug/Linux/locking.sh       2014-03-10 10:43:57.000000000 +0000
+++ xen-4.4.0/tools/hotplug/Linux/locking.sh    2014-07-30 13:30:46.864655020 +0100
@@ -46,19 +46,11 @@
     while true; do
         eval "exec $_lockfd<>$_lockfile"
         flock -x $_lockfd || return $?
-        # 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
+
+        _fd_inum=`stat -L -c '%i' /proc/self/fd/$_lockfd`
+        _file_inum=`sh -c "stat -c '%i' $_lockfile || true"`
+        if [ x$_fd_inum = x$_file_inum ]; then break; fi
+
        # Some versions of bash appear to be buggy if the same
        # $_lockfile is opened repeatedly. Close the current fd here.
         eval "exec $_lockfd<&-"

Re: xen 4.4.0 unnecessary dependency on Perl

[Ian Jackson]

Anthony Wright writes ("Re: [Xen-devel] xen 4.4.0 unnecessary dependency on Perl"):
> Below is a patch that converts the perl script into shell script. I'm new to submitting patches, so I apologise if this is the wrong way to do it.

This patch is pretty good.  We also need you to sign it off, and it
would be good if you could provide a commit message.  See `Signing off
a patch' in
  http://wiki.xen.org/wiki/Submitting_Xen_Patches

> I believe the patch provides the same functionality as the perl, but it does exactly what the comment says isn't good enough.

I agree with you that the comment is wrong.

It is wrong for two entirely separate reasons:


I.

No-one unlinks the lockfile unless they have it locked.  (This is an
essential part of this locking protocol.)  (And, no-one will re-link
into the filesystem an unlinked lockfile.)  That means that if we have
successfully locked a file then either:

 1. The lockfile name referred to the file we have flocked at the time
    we flocked it.  This means that at that time, we were the holder
    of the abstract lock.  We haven't released the abstract lock (by
    removing the flock or unlinking the file).  Therefore no-one else
    is allowed to have deleted the file after that time and no-one is
    racing with our stat to delete the file.

or:

 2. The lockfile name already referred to a different file, or no file
    at all, when we flocked the file we have open.  That means that at
    the time we got the flock, our file was already unlinked.  We have
    therefore definitely lost the race; again, our stat of our open
    file is not racing with the task which unlinked it - the unlink
    happened earlier.

II.

It is not the case that stat -L /proc/self/fd/N works by reading the
synthetic link target (with or as if by readlink(2)) and then doing a
filesystem path lookup on the reported link target path.  stat(1)
given -L calls stat(2) rather than lstat(2).  stat(2) on
/proc/self/fd/N accesses the actual open file object (found via the
process's file table) without regard to the name(s) it may or may not
now have:

$ exec 3<>t
$ stat -c %i t
813539
$ ls -i t
813539 t
$ stat -L -c %i /proc/self/fd/3
813539
$ rm t
$ stat -L -c %i /proc/self/fd/3
813539
$ stat -c %i t
stat: cannot stat `t': No such file or directory
$ stat -c %i "`readlink /proc/self/fd/3`"
stat: cannot stat `/u/iwj/t (deleted)': No such file or directory
$


So,

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

Re: xen 4.4.0 unnecessary dependency on Perl

[Olaf Hering]

On Wed, Jul 30, Anthony Wright wrote:

> Below is a patch that converts the perl script into shell script. I'm
> new to submitting patches, so I apologise if this is the wrong way to
> do it.

http://lists.xen.org/archives/html/xen-devel/2014-07/msg03949.html

Please see the reply from Ian and resubmit your locking change.
Thanks.

Olaf