diff mbox series

scripts: don't rely on "stat -" support

Message ID 691aebb4-87af-60df-b6ad-07cb6fef4167@suse.com (mailing list archive)
State New, archived
Headers show
Series scripts: don't rely on "stat -" support | expand

Commit Message

Jan Beulich June 25, 2020, 3:16 p.m. UTC 
While commit b72682c602b8 ("scripts: Use stat to check lock claim")
validly indicates that stat has gained support for the special "-"
command line option in 2009, we should still try to avoid breaking being
able to run on even older distros. As it has been determined, contary to
the comment in the script using /dev/stdin (/proc/self/fd/$_lockfd) is
fine here, as Linux specially treats these /proc inodes.

Suggested-by: Ian Jackson <ian.jackson@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Ian Jackson June 25, 2020, 3:45 p.m. UTC | #1 
Jan Beulich writes ("[PATCH] scripts: don't rely on "stat -" support"):
> While commit b72682c602b8 ("scripts: Use stat to check lock claim")
> validly indicates that stat has gained support for the special "-"
> command line option in 2009, we should still try to avoid breaking being
> able to run on even older distros. As it has been determined, contary to
> the comment in the script using /dev/stdin (/proc/self/fd/$_lockfd) is
> fine here, as Linux specially treats these /proc inodes.
> 
> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Thanks.

The only code change here is this:

> --- a/tools/hotplug/Linux/locking.sh
> +++ b/tools/hotplug/Linux/locking.sh
> @@ -45,18 +45,14 @@ claim_lock()
> -        if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
> +        if stat=$( stat -L -c '%D.%i' /dev/stdin $_lockfile 0<&$_lockfd 2>/dev/null )

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

Has anyone executed this ?

Paul, can we have a release-ack ?

Thanks,
Ian.
Jan Beulich June 25, 2020, 3:47 p.m. UTC | #2 
On 25.06.2020 17:45, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH] scripts: don't rely on "stat -" support"):
>> While commit b72682c602b8 ("scripts: Use stat to check lock claim")
>> validly indicates that stat has gained support for the special "-"
>> command line option in 2009, we should still try to avoid breaking being
>> able to run on even older distros. As it has been determined, contary to
>> the comment in the script using /dev/stdin (/proc/self/fd/$_lockfd) is
>> fine here, as Linux specially treats these /proc inodes.
>>
>> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.
> 
> The only code change here is this:
> 
>> --- a/tools/hotplug/Linux/locking.sh
>> +++ b/tools/hotplug/Linux/locking.sh
>> @@ -45,18 +45,14 @@ claim_lock()
>> -        if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
>> +        if stat=$( stat -L -c '%D.%i' /dev/stdin $_lockfile 0<&$_lockfd 2>/dev/null )
> 
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Has anyone executed this ?

I have, of course, to confirm this fixes my issue. But I'm not sure
that's what you've meant to ask - you may have wanted assurance
that someone else has also tried it.

Jan
Jason Andryuk June 25, 2020, 4:23 p.m. UTC | #3 
On Thu, Jun 25, 2020 at 11:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.06.2020 17:45, Ian Jackson wrote:
> > Jan Beulich writes ("[PATCH] scripts: don't rely on "stat -" support"):
> >> While commit b72682c602b8 ("scripts: Use stat to check lock claim")
> >> validly indicates that stat has gained support for the special "-"
> >> command line option in 2009, we should still try to avoid breaking being
> >> able to run on even older distros. As it has been determined, contary to
> >> the comment in the script using /dev/stdin (/proc/self/fd/$_lockfd) is
> >> fine here, as Linux specially treats these /proc inodes.
> >>
> >> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > Thanks.
> >
> > The only code change here is this:
> >
> >> --- a/tools/hotplug/Linux/locking.sh
> >> +++ b/tools/hotplug/Linux/locking.sh
> >> @@ -45,18 +45,14 @@ claim_lock()
> >> -        if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
> >> +        if stat=$( stat -L -c '%D.%i' /dev/stdin $_lockfile 0<&$_lockfd 2>/dev/null )
> >
> > Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >
> > Has anyone executed this ?
>
> I have, of course, to confirm this fixes my issue. But I'm not sure
> that's what you've meant to ask - you may have wanted assurance
> that someone else has also tried it.

Tested-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Ian Jackson June 25, 2020, 4:35 p.m. UTC | #4 
Jason Andryuk writes ("Re: [PATCH] scripts: don't rely on "stat -" support"):
> On Thu, Jun 25, 2020 at 11:47 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 25.06.2020 17:45, Ian Jackson wrote:
> > > Jan Beulich writes ("[PATCH] scripts: don't rely on "stat -" support"):
> > >> While commit b72682c602b8 ("scripts: Use stat to check lock claim")
> > >> validly indicates that stat has gained support for the special "-"
> > >> command line option in 2009, we should still try to avoid breaking being
> > >> able to run on even older distros. As it has been determined, contary to
> > >> the comment in the script using /dev/stdin (/proc/self/fd/$_lockfd) is
> > >> fine here, as Linux specially treats these /proc inodes.
> > >>
> > >> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >
> > > Thanks.
> > >
> > > The only code change here is this:
> > >
> > >> --- a/tools/hotplug/Linux/locking.sh
> > >> +++ b/tools/hotplug/Linux/locking.sh
> > >> @@ -45,18 +45,14 @@ claim_lock()
> > >> -        if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
> > >> +        if stat=$( stat -L -c '%D.%i' /dev/stdin $_lockfile 0<&$_lockfd 2>/dev/null )
> > >
> > > Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > >
> > > Has anyone executed this ?
> >
> > I have, of course, to confirm this fixes my issue. But I'm not sure
> > that's what you've meant to ask - you may have wanted assurance
> > that someone else has also tried it.
> 
> Tested-by: Jason Andryuk <jandryuk@gmail.com>
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

:-).

Thanks,
Ian.
Paul Durrant June 26, 2020, 7:35 a.m. UTC | #5 
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 25 June 2020 17:36
> To: Jason Andryuk <jandryuk@gmail.com>
> Cc: Jan Beulich <jbeulich@suse.com>; Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org; Wei
> Liu <wl@xen.org>
> Subject: Re: [PATCH] scripts: don't rely on "stat -" support
> 
> Jason Andryuk writes ("Re: [PATCH] scripts: don't rely on "stat -" support"):
> > On Thu, Jun 25, 2020 at 11:47 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >
> > > On 25.06.2020 17:45, Ian Jackson wrote:
> > > > Jan Beulich writes ("[PATCH] scripts: don't rely on "stat -" support"):
> > > >> While commit b72682c602b8 ("scripts: Use stat to check lock claim")
> > > >> validly indicates that stat has gained support for the special "-"
> > > >> command line option in 2009, we should still try to avoid breaking being
> > > >> able to run on even older distros. As it has been determined, contary to
> > > >> the comment in the script using /dev/stdin (/proc/self/fd/$_lockfd) is
> > > >> fine here, as Linux specially treats these /proc inodes.
> > > >>
> > > >> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
> > > >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > >
> > > > Thanks.
> > > >
> > > > The only code change here is this:
> > > >
> > > >> --- a/tools/hotplug/Linux/locking.sh
> > > >> +++ b/tools/hotplug/Linux/locking.sh
> > > >> @@ -45,18 +45,14 @@ claim_lock()
> > > >> -        if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
> > > >> +        if stat=$( stat -L -c '%D.%i' /dev/stdin $_lockfile 0<&$_lockfd 2>/dev/null )
> > > >
> > > > Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > >
> > > > Has anyone executed this ?
> > >
> > > I have, of course, to confirm this fixes my issue. But I'm not sure
> > > that's what you've meant to ask - you may have wanted assurance
> > > that someone else has also tried it.
> >
> > Tested-by: Jason Andryuk <jandryuk@gmail.com>
> > Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
> 
> :-).
> 

In which case...

Release-acked-by: Paul Durrant <paul@xen.org>

> Thanks,
> Ian.
diff mbox series

Patch

--- a/tools/hotplug/Linux/locking.sh
+++ b/tools/hotplug/Linux/locking.sh
@@ -45,18 +45,14 @@  claim_lock()
     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.  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:
+        # Although /dev/stdin (i.e. /proc/self/fd/0) looks like a symlink,
+        # stat(2) bypasses the synthetic symlink and directly accesses the
+        # underlying open-file.  So this works correctly even if the file
+        # has been renamed or unlinked.  stat will output two lines like:
         # WW.XXX
         # YY.ZZZ
         # which need to be separated and compared.
-        if stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null )
+        if stat=$( stat -L -c '%D.%i' /dev/stdin $_lockfile 0<&$_lockfd 2>/dev/null )
         then
             local file_stat
             local fd_stat