Re: [Xen-devel] [PATCH] scripts: Use stat to check lock claim

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Mar 10, 2020 at 11:43 AM Ian Jackson <ian.jackson@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel