Replace the perl locking check with stat(1).  Stat is able to fstat
stdin (file descriptor 0) when passed '-' as an argument.  This is now
used to check $_lockfd.  stat(1) support for '-' was introduced to
coreutils in 2009.

After A releases its lock, script B will return from flock and execute
stat.  Since the lockfile has been removed from A, stat prints an error
to stderr and exits non-zero.  '|| :' is needed to squash the non-zero
exit status - otherwise the script terminates since `set -e` is enabled.
stderr needs to be redirected to /dev/null otherwise
/var/log/xen/xen-hotplug.log will get filled with "No such file or
directory" messages.

This change removes the only runtime dependency of the xen toolstack on
perl.

While here, replace some tabs with spaces to match the rest of the file.

Suggested-by: Ian Jackson <ian.jackson@citrix.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/hotplug/Linux/locking.sh | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/tools/hotplug/Linux/locking.sh b/tools/hotplug/Linux/locking.sh
index c6a7e96ff9..03361f405f 100644
--- a/tools/hotplug/Linux/locking.sh
+++ b/tools/hotplug/Linux/locking.sh
@@ -41,7 +41,9 @@ claim_lock()
     # from chiark-utils, except using flock.  It has the benefit of
     # it being possible to safely remove the lockfile when done.
     # See below for a correctness proof.
-    local rightfile
+    local stat
+    local fd_stat
+    local file_stat
     while true; do
         eval "exec $_lockfd<>$_lockfile"
         flock -x $_lockfd || return $?
@@ -50,16 +52,20 @@ claim_lock()
         # 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
-	# Some versions of bash appear to be buggy if the same
-	# $_lockfile is opened repeatedly. Close the current fd here.
+        # file system to find the inum.  stat(1) translates '-' into an
+        # fstat(2) of FD 0.  So we just need to arrange the FDs properly
+        # to get the fstat(2) we need.  stat will output two lines like:
+        # WW.XXX
+        # YY.ZZZ
+        # which need to be separated and compared.
+        stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null || : )
+        if [ -n "$stat" ]; then
+            fd_stat=$( echo "$stat" | sed -n '1p' )
+            file_stat=$( echo "$stat" | sed -n '2p' )
+            if [ "$fd_stat" = "$file_stat" ] ; then break; fi
+        fi
+        # Some versions of bash appear to be buggy if the same
+        # $_lockfile is opened repeatedly. Close the current fd here.
         eval "exec $_lockfd<&-"
     done
 }
-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Tue, Mar 10, 2020 at 09:29:04AM -0400, Jason Andryuk wrote:
> Replace the perl locking check with stat(1).  Stat is able to fstat
> stdin (file descriptor 0) when passed '-' as an argument.  This is now
> used to check $_lockfd.  stat(1) support for '-' was introduced to
> coreutils in 2009.
> 
> After A releases its lock, script B will return from flock and execute
> stat.  Since the lockfile has been removed from A, stat prints an error
> to stderr and exits non-zero.  '|| :' is needed to squash the non-zero
> exit status - otherwise the script terminates since `set -e` is enabled.
> stderr needs to be redirected to /dev/null otherwise
> /var/log/xen/xen-hotplug.log will get filled with "No such file or
> directory" messages.
> 
> This change removes the only runtime dependency of the xen toolstack on
> perl.
> 
> While here, replace some tabs with spaces to match the rest of the file.
> 
> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Acked-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Jason Andryuk writes ("[PATCH] scripts: Use stat to check lock claim"):
> Replace the perl locking check with stat(1).  Stat is able to fstat
> stdin (file descriptor 0) when passed '-' as an argument.  This is now
> used to check $_lockfd.  stat(1) support for '-' was introduced to
> coreutils in 2009.
> 
> After A releases its lock, script B will return from flock and execute
> stat.  Since the lockfile has been removed from A, stat prints an error
> to stderr and exits non-zero.  '|| :' is needed to squash the non-zero
> exit status - otherwise the script terminates since `set -e` is enabled.
> stderr needs to be redirected to /dev/null otherwise
> /var/log/xen/xen-hotplug.log will get filled with "No such file or
> directory" messages.

Thanks.  This is looking good.

I have two nits:

> While here, replace some tabs with spaces to match the rest of the file.

Please can you do this in a separate patch, ideally beforehand.  (I
don't think this is a blocker in this case, given how small this patch
is.)

> +        stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null || : )
> +        if [ -n "$stat" ]; then
> +            fd_stat=$( echo "$stat" | sed -n '1p' )
> +            file_stat=$( echo "$stat" | sed -n '2p' )
> +            if [ "$fd_stat" = "$file_stat" ] ; then break; fi

I think you don't need sed here:

$ ls -li t u
844307 -rw-rw-r-- 1 iwj iwj 117844 Oct 31 12:50 t
826417 -rw-r--r-- 1 iwj iwj   1765 Jan 31  2019 u
$ bash -c 'x=$( stat -L -c "%D.%i" t u 2>/dev/null || : ); echo ${x%
*} = ${x#*
}'
fe04.844307 = fe04.826417
$

The syntax (with newlines within the ${ }) is a bit odd but not
invoking sed here will be faster.

Alternatively, if you don't mind using --printf instead of -c,

$ bash -c 'x=$( stat -L --format "%D.%i " t u 2>/dev/null || : ); echo ${x%% *} = ${x#* }'
fe04.844307 = fe04.826417
$

I don't know when --format was introduced.

I'm sorry to bounce the patch over such a small thing, but this is
path is already quite slow and is critical for domain creation and I
would prefer not to add (two) additional subprocess invocations here.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Tue, Mar 10, 2020 at 11:43 AM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Jason Andryuk writes ("[PATCH] scripts: Use stat to check lock claim"):
> > Replace the perl locking check with stat(1).  Stat is able to fstat
> > stdin (file descriptor 0) when passed '-' as an argument.  This is now
> > used to check $_lockfd.  stat(1) support for '-' was introduced to
> > coreutils in 2009.
> >
> > After A releases its lock, script B will return from flock and execute
> > stat.  Since the lockfile has been removed from A, stat prints an error
> > to stderr and exits non-zero.  '|| :' is needed to squash the non-zero
> > exit status - otherwise the script terminates since `set -e` is enabled.
> > stderr needs to be redirected to /dev/null otherwise
> > /var/log/xen/xen-hotplug.log will get filled with "No such file or
> > directory" messages.
>
> Thanks.  This is looking good.
>
> I have two nits:
>
> > While here, replace some tabs with spaces to match the rest of the file.
>
> Please can you do this in a separate patch, ideally beforehand.  (I
> don't think this is a blocker in this case, given how small this patch
> is.)

Sure, I can do that.

> > +        stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null || : )
> > +        if [ -n "$stat" ]; then
> > +            fd_stat=$( echo "$stat" | sed -n '1p' )
> > +            file_stat=$( echo "$stat" | sed -n '2p' )
> > +            if [ "$fd_stat" = "$file_stat" ] ; then break; fi
>
> I think you don't need sed here:
>
> $ ls -li t u
> 844307 -rw-rw-r-- 1 iwj iwj 117844 Oct 31 12:50 t
> 826417 -rw-r--r-- 1 iwj iwj   1765 Jan 31  2019 u
> $ bash -c 'x=$( stat -L -c "%D.%i" t u 2>/dev/null || : ); echo ${x%
> *} = ${x#*
> }'
> fe04.844307 = fe04.826417
> $
>
> The syntax (with newlines within the ${ }) is a bit odd but not
> invoking sed here will be faster.

When the lockfile is removed, we only have one line of output.  The
above constructs do nothing in that case, so the substituted values
are identical.  (That was one benefit of sed combined with ensure
there was some output).

This could be worked around by doing
  if stat=$( stat -L ....) ; then
Dropping `|| :` to ensure only successful executions are processed.
Since it's in an "if", `set -e` doesn't terminate the script.

> Alternatively, if you don't mind using --printf instead of -c,
>
> $ bash -c 'x=$( stat -L --format "%D.%i " t u 2>/dev/null || : ); echo ${x%% *} = ${x#* }'
> fe04.844307 = fe04.826417
> $
>
> I don't know when --format was introduced.

Looks like --printf was introduced in 2005.  I think I prefer this to
having the newlines.  You still have some of the string substitution
concerns, but I think think relying on a successful stat(1) call to
give two output values is reasonable.

> I'm sorry to bounce the patch over such a small thing, but this is
> path is already quite slow and is critical for domain creation and I
> would prefer not to add (two) additional subprocess invocations here.

No worries.

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Tue, Mar 10, 2020 at 4:06 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Tue, Mar 10, 2020 at 11:43 AM Ian Jackson <ian.jackson@citrix.com> wrote:
> > Alternatively, if you don't mind using --printf instead of -c,
> >
> > $ bash -c 'x=$( stat -L --format "%D.%i " t u 2>/dev/null || : ); echo ${x%% *} = ${x#* }'
> > fe04.844307 = fe04.826417
> > $
> >
> > I don't know when --format was introduced.
>
> Looks like --printf was introduced in 2005.  I think I prefer this to
> having the newlines.  You still have some of the string substitution
> concerns, but I think think relying on a successful stat(1) call to
> give two output values is reasonable.

busybox stat does not support --printf.  This is not an immediate
concern for me, but it's something I thought of and just tested.  The
newline approach avoids this complication.

> > I'm sorry to bounce the patch over such a small thing, but this is
> > path is already quite slow and is critical for domain creation and I
> > would prefer not to add (two) additional subprocess invocations here.
>
> No worries.

The above gyrations can be avoided if we just call stat twice - once
for the fd and once for the file.  They aren't required to be in a
single call.  But moving forward with a single call, we have a few
options:

We could use an array to side-step the line splitting:

if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
then
        stat=(${stat})
        fd_stat=${stat[0]}
        file_stat=${stat[1]}
        [ "$fd_stat" = "$file_stat" ]

Another option is to use the bashism $'\n' instead of the literal newlines:

if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
then
        fd_stat=${stat%$'\n'*}
        file_stat=${stat#*$'\n'}

Or just use your newline construct.  Which do you prefer?

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel