| Message ID | 20200310132904.16992-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scripts: Use stat to check lock claim | expand |
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>
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.
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
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
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 }
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(-)