* use of "stat -" @ 2020-05-12 10:58 Jan Beulich 2020-05-12 14:19 ` Wei Liu 0 siblings, 1 reply; 27+ messages in thread From: Jan Beulich @ 2020-05-12 10:58 UTC (permalink / raw) To: Jason Andryuk, Ian Jackson, Wei Liu; +Cc: xen-devel, Paul Durrant Hello, now that I've been able to do a little bit of work from the office again, I've run into a regression from b72682c602b8 "scripts: Use stat to check lock claim". On one of my older machines I've noticed guests wouldn't launch anymore, which I've tracked down to the system having an old stat on it. Yes, the commit says the needed behavior has been available since 2009, but please let's not forget that we continue to support building with tool chains from about 2007. Putting in place and using newer tool chain versions without touching the base distro is pretty straightforward. Replacing the coreutils package isn't, and there's not even an override available by which one could point at an alternative one. Hence I think bumping the minimum required versions of basic tools should be done even more carefully than bumping required tool chain versions (which we've not dared to do in years). After having things successfully working again with a full revert, I'm now going to experiment with adapting behavior to stat's capabilities. Would something like that be acceptable (if it works out)? Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 10:58 use of "stat -" Jan Beulich @ 2020-05-12 14:19 ` Wei Liu 2020-05-12 14:33 ` Jan Beulich 0 siblings, 1 reply; 27+ messages in thread From: Wei Liu @ 2020-05-12 14:19 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Jason Andryuk On Tue, May 12, 2020 at 12:58:46PM +0200, Jan Beulich wrote: > Hello, > > now that I've been able to do a little bit of work from the office > again, I've run into a regression from b72682c602b8 "scripts: Use > stat to check lock claim". On one of my older machines I've noticed > guests wouldn't launch anymore, which I've tracked down to the > system having an old stat on it. Yes, the commit says the needed > behavior has been available since 2009, but please let's not forget > that we continue to support building with tool chains from about > 2007. > > Putting in place and using newer tool chain versions without > touching the base distro is pretty straightforward. Replacing the > coreutils package isn't, and there's not even an override available > by which one could point at an alternative one. Hence I think > bumping the minimum required versions of basic tools should be > done even more carefully than bumping required tool chain versions > (which we've not dared to do in years). After having things > successfully working again with a full revert, I'm now going to > experiment with adapting behavior to stat's capabilities. Would > something like that be acceptable (if it works out)? Are you asking for reverting that patch? Wei. > > Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 14:19 ` Wei Liu @ 2020-05-12 14:33 ` Jan Beulich 2020-05-12 14:47 ` Andrew Cooper 0 siblings, 1 reply; 27+ messages in thread From: Jan Beulich @ 2020-05-12 14:33 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Paul Durrant, Ian Jackson, Jason Andryuk On 12.05.2020 16:19, Wei Liu wrote: > On Tue, May 12, 2020 at 12:58:46PM +0200, Jan Beulich wrote: >> now that I've been able to do a little bit of work from the office >> again, I've run into a regression from b72682c602b8 "scripts: Use >> stat to check lock claim". On one of my older machines I've noticed >> guests wouldn't launch anymore, which I've tracked down to the >> system having an old stat on it. Yes, the commit says the needed >> behavior has been available since 2009, but please let's not forget >> that we continue to support building with tool chains from about >> 2007. >> >> Putting in place and using newer tool chain versions without >> touching the base distro is pretty straightforward. Replacing the >> coreutils package isn't, and there's not even an override available >> by which one could point at an alternative one. Hence I think >> bumping the minimum required versions of basic tools should be >> done even more carefully than bumping required tool chain versions >> (which we've not dared to do in years). After having things >> successfully working again with a full revert, I'm now going to >> experiment with adapting behavior to stat's capabilities. Would >> something like that be acceptable (if it works out)? > > Are you asking for reverting that patch? Well, I assume the patch has its merits, even if I don't really understand what they are from its description. I'm instead asking whether something like the below (meanwhile tested) would be acceptable. Jan --- unstable.orig/tools/hotplug/Linux/locking.sh +++ unstable/tools/hotplug/Linux/locking.sh @@ -42,6 +42,12 @@ claim_lock() # it being possible to safely remove the lockfile when done. # See below for a correctness proof. local stat + local use_stat + local rightfile + if stat - 2>/dev/null >/dev/null + then + use_stat=y + fi while true; do eval "exec $_lockfd<>$_lockfile" flock -x $_lockfd || return $? @@ -56,7 +62,17 @@ claim_lock() # 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 [ x$use_stat != xy ] + then + # Fall back to the original Perl approach. + 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 + elif stat=$( stat -L -c '%D.%i' - $_lockfile 0<&$_lockfd 2>/dev/null ) then local file_stat local fd_stat ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 14:33 ` Jan Beulich @ 2020-05-12 14:47 ` Andrew Cooper 2020-05-12 14:59 ` Jan Beulich 0 siblings, 1 reply; 27+ messages in thread From: Andrew Cooper @ 2020-05-12 14:47 UTC (permalink / raw) To: Jan Beulich, Wei Liu; +Cc: xen-devel, Jason Andryuk, Ian Jackson, Paul Durrant On 12/05/2020 15:33, Jan Beulich wrote: > On 12.05.2020 16:19, Wei Liu wrote: >> On Tue, May 12, 2020 at 12:58:46PM +0200, Jan Beulich wrote: >>> now that I've been able to do a little bit of work from the office >>> again, I've run into a regression from b72682c602b8 "scripts: Use >>> stat to check lock claim". On one of my older machines I've noticed >>> guests wouldn't launch anymore, which I've tracked down to the >>> system having an old stat on it. Yes, the commit says the needed >>> behavior has been available since 2009, but please let's not forget >>> that we continue to support building with tool chains from about >>> 2007. >>> >>> Putting in place and using newer tool chain versions without >>> touching the base distro is pretty straightforward. Replacing the >>> coreutils package isn't, and there's not even an override available >>> by which one could point at an alternative one. Hence I think >>> bumping the minimum required versions of basic tools should be >>> done even more carefully than bumping required tool chain versions >>> (which we've not dared to do in years). After having things >>> successfully working again with a full revert, I'm now going to >>> experiment with adapting behavior to stat's capabilities. Would >>> something like that be acceptable (if it works out)? >> Are you asking for reverting that patch? > Well, I assume the patch has its merits, even if I don't really > understand what they are from its description. What is in any away unclear about the final paragraph in the commit message? > I'm instead asking > whether something like the below (meanwhile tested) would be > acceptable. Not really, seeing as removing perl was the whole point. ~Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 14:47 ` Andrew Cooper @ 2020-05-12 14:59 ` Jan Beulich 2020-05-12 15:52 ` Jason Andryuk 0 siblings, 1 reply; 27+ messages in thread From: Jan Beulich @ 2020-05-12 14:59 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel, Jason Andryuk, Ian Jackson, Wei Liu, Paul Durrant On 12.05.2020 16:47, Andrew Cooper wrote: > On 12/05/2020 15:33, Jan Beulich wrote: >> On 12.05.2020 16:19, Wei Liu wrote: >>> On Tue, May 12, 2020 at 12:58:46PM +0200, Jan Beulich wrote: >>>> now that I've been able to do a little bit of work from the office >>>> again, I've run into a regression from b72682c602b8 "scripts: Use >>>> stat to check lock claim". On one of my older machines I've noticed >>>> guests wouldn't launch anymore, which I've tracked down to the >>>> system having an old stat on it. Yes, the commit says the needed >>>> behavior has been available since 2009, but please let's not forget >>>> that we continue to support building with tool chains from about >>>> 2007. >>>> >>>> Putting in place and using newer tool chain versions without >>>> touching the base distro is pretty straightforward. Replacing the >>>> coreutils package isn't, and there's not even an override available >>>> by which one could point at an alternative one. Hence I think >>>> bumping the minimum required versions of basic tools should be >>>> done even more carefully than bumping required tool chain versions >>>> (which we've not dared to do in years). After having things >>>> successfully working again with a full revert, I'm now going to >>>> experiment with adapting behavior to stat's capabilities. Would >>>> something like that be acceptable (if it works out)? >>> Are you asking for reverting that patch? >> Well, I assume the patch has its merits, even if I don't really >> understand what they are from its description. > > What is in any away unclear about the final paragraph in the commit message? This being the last sentence instead of the first (or even the subject) makes it look like this is a nice side effect, not like this was the goal of the change. >> I'm instead asking >> whether something like the below (meanwhile tested) would be >> acceptable. > > Not really, seeing as removing perl was the whole point. The suggested change doesn't re-introduce a runtime dependency on Perl, _except_ on very old systems. Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 14:59 ` Jan Beulich @ 2020-05-12 15:52 ` Jason Andryuk 2020-05-12 19:50 ` Elliott Mitchell 0 siblings, 1 reply; 27+ messages in thread From: Jason Andryuk @ 2020-05-12 15:52 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Paul Durrant, Ian Jackson, Wei Liu, xen-devel On Tue, May 12, 2020 at 10:59 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 12.05.2020 16:47, Andrew Cooper wrote: > > On 12/05/2020 15:33, Jan Beulich wrote: > >> On 12.05.2020 16:19, Wei Liu wrote: > >>> On Tue, May 12, 2020 at 12:58:46PM +0200, Jan Beulich wrote: > >>>> now that I've been able to do a little bit of work from the office > >>>> again, I've run into a regression from b72682c602b8 "scripts: Use > >>>> stat to check lock claim". On one of my older machines I've noticed > >>>> guests wouldn't launch anymore, which I've tracked down to the > >>>> system having an old stat on it. Yes, the commit says the needed > >>>> behavior has been available since 2009, but please let's not forget > >>>> that we continue to support building with tool chains from about > >>>> 2007. Sorry for regressing your old system setup. Out of curiosity, what OS version are you using? > >>>> Putting in place and using newer tool chain versions without > >>>> touching the base distro is pretty straightforward. Replacing the > >>>> coreutils package isn't, and there's not even an override available > >>>> by which one could point at an alternative one. Hence I think > >>>> bumping the minimum required versions of basic tools should be > >>>> done even more carefully than bumping required tool chain versions > >>>> (which we've not dared to do in years). After having things > >>>> successfully working again with a full revert, I'm now going to > >>>> experiment with adapting behavior to stat's capabilities. Would > >>>> something like that be acceptable (if it works out)? > >>> Are you asking for reverting that patch? > >> Well, I assume the patch has its merits, even if I don't really > >> understand what they are from its description. > > > > What is in any away unclear about the final paragraph in the commit message? > > This being the last sentence instead of the first (or even the > subject) makes it look like this is a nice side effect, not > like this was the goal of the change. I see how the motivation wasn't conveyed properly in the commit message. It was captured in the cover letter, but that doesn't make it into the repo. :( > >> I'm instead asking > >> whether something like the below (meanwhile tested) would be > >> acceptable. > > > > Not really, seeing as removing perl was the whole point. > > The suggested change doesn't re-introduce a runtime dependency on > Perl, _except_ on very old systems. Yes, the runtime detection looks okay. However, Ian may not like testing `stat -` since he previously disliked the extra overhead of calling sed. v1 of the patchset created a dedicated C utility, but Ian preferred using stat(1). Qubes uses a different approach to remove perl to bypass stat-ing the FD. "Use plain flock on open FD. This makes the locking mechanism not tolerate removing the lock file once created." https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch So they have lockfiles persist even when no process holds the lock. I was just looking to remove perl since it's a large dependency for this single use. I'm not tied to a particular approach. Regards, Jason ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 15:52 ` Jason Andryuk @ 2020-05-12 19:50 ` Elliott Mitchell 2020-05-12 19:54 ` Andrew Cooper 0 siblings, 1 reply; 27+ messages in thread From: Elliott Mitchell @ 2020-05-12 19:50 UTC (permalink / raw) To: Jason Andryuk Cc: Wei Liu, Paul Durrant, Andrew Cooper, Ian Jackson, Jan Beulich, xen-devel On Tue, May 12, 2020 at 11:52:25AM -0400, Jason Andryuk wrote: > I was just looking to remove perl since it's a large dependency for > this single use. I'm not tied to a particular approach. Have you tried to remove Perl from a system? This is possible, but on many systems there will be hundreds or thousands of other programs which already cause Perl to be installed. Perl doesn't have the mindshare it once did, but it is far from dead. A new desktop Linux installation might have less than a hundred programs depending on Perl. A new developer Linux installation will likely have hundreds of programs depending on Perl. A decades old system like Jan is testing, there will like thousands. Removing dependancies is good. Perhaps this is looking a few years into the future where Perl is even less common. -- (\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/) \BS ( | ehem+sigmsg@m5p.com PGP 87145445 | ) / \_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/ 8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 19:50 ` Elliott Mitchell @ 2020-05-12 19:54 ` Andrew Cooper 2020-05-12 22:54 ` Elliott Mitchell 0 siblings, 1 reply; 27+ messages in thread From: Andrew Cooper @ 2020-05-12 19:54 UTC (permalink / raw) To: Elliott Mitchell, Jason Andryuk Cc: xen-devel, Ian Jackson, Wei Liu, Jan Beulich, Paul Durrant On 12/05/2020 20:50, Elliott Mitchell wrote: > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. > > On Tue, May 12, 2020 at 11:52:25AM -0400, Jason Andryuk wrote: >> I was just looking to remove perl since it's a large dependency for >> this single use. I'm not tied to a particular approach. > Have you tried to remove Perl from a system? This is possible, but on > many systems there will be hundreds or thousands of other programs which > already cause Perl to be installed. > > Perl doesn't have the mindshare it once did, but it is far from dead. A > new desktop Linux installation might have less than a hundred programs > depending on Perl. A new developer Linux installation will likely have > hundreds of programs depending on Perl. A decades old system like Jan > is testing, there will like thousands. > > Removing dependancies is good. Perhaps this is looking a few years into > the future where Perl is even less common. Not everyone has a fully fat Linux running as dom0. There are real systems using Xen which have already successfully purged perl. ~Andrew ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 19:54 ` Andrew Cooper @ 2020-05-12 22:54 ` Elliott Mitchell 2020-05-14 11:02 ` Ian Jackson 0 siblings, 1 reply; 27+ messages in thread From: Elliott Mitchell @ 2020-05-12 22:54 UTC (permalink / raw) To: Andrew Cooper Cc: Wei Liu, Paul Durrant, Jason Andryuk, Ian Jackson, Jan Beulich, xen-devel On Tue, May 12, 2020 at 08:54:26PM +0100, Andrew Cooper wrote: > On 12/05/2020 20:50, Elliott Mitchell wrote: > > On Tue, May 12, 2020 at 11:52:25AM -0400, Jason Andryuk wrote: > >> I was just looking to remove perl since it's a large dependency for > >> this single use. I'm not tied to a particular approach. > > Removing dependancies is good. Perhaps this is looking a few years into > > the future where Perl is even less common. > > Not everyone has a fully fat Linux running as dom0.?? There are real > systems using Xen which have already successfully purged perl. Gah! Misread an earlier message. Upon rereading the message seems my thinking was wrong. Yes, it is pretty reasonable to setup a system without Perl. Sorry for interrupting with the braindead comment. -- (\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/) \BS ( | ehem+sigmsg@m5p.com PGP 87145445 | ) / \_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/ 8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-12 22:54 ` Elliott Mitchell @ 2020-05-14 11:02 ` Ian Jackson 2020-05-14 12:39 ` Jan Beulich 2020-05-18 10:34 ` Jan Beulich 0 siblings, 2 replies; 27+ messages in thread From: Ian Jackson @ 2020-05-14 11:02 UTC (permalink / raw) To: Elliott Mitchell Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jason Andryuk, Jan Beulich, xen-devel I've read this thread. Jan, I'm sorry that this causes you inconvenience. I'm hoping it won't come down to a choice between supporting people who want to ship a dom0 without perl, and people who want a dom0 using more-than-a-decade-old coreutils. Jan, can you tell me what the output is of this on your ancient system: $ rm -f t $ >t $ exec 3<t $ stat -L -c '%F %i' /dev/stdin <&3 regular empty file 393549 $ rm t $ stat -L -c '%F %i' /dev/stdin <&3 regular empty file 393549 $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 $ Also, the contents of the file "u" afterwards, please. Thanks, Ian. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-14 11:02 ` Ian Jackson @ 2020-05-14 12:39 ` Jan Beulich 2020-05-18 10:34 ` Jan Beulich 1 sibling, 0 replies; 27+ messages in thread From: Jan Beulich @ 2020-05-14 12:39 UTC (permalink / raw) To: Ian Jackson Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jason Andryuk, Elliott Mitchell, xen-devel On 14.05.2020 13:02, Ian Jackson wrote: > I've read this thread. Jan, I'm sorry that this causes you > inconvenience. I'm hoping it won't come down to a choice between > supporting people who want to ship a dom0 without perl, and people who > want a dom0 using more-than-a-decade-old coreutils. Well, there are options, like producing the actual script from a locking.sh.in template, with a configure control over whether the Perl fallback is needed / wanted. > Jan, can you tell me what the output is of this on your ancient > system: > > $ rm -f t > $ >t > $ exec 3<t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 393549 > $ rm t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 393549 > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > $ > > Also, the contents of the file "u" afterwards, please. Will do early next week, as the system is in the office (and off). Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: use of "stat -" 2020-05-14 11:02 ` Ian Jackson 2020-05-14 12:39 ` Jan Beulich @ 2020-05-18 10:34 ` Jan Beulich 2020-06-24 15:55 ` Ping: " Jan Beulich 2020-06-24 16:19 ` [XEN RFC for-4.14] " Ian Jackson 1 sibling, 2 replies; 27+ messages in thread From: Jan Beulich @ 2020-05-18 10:34 UTC (permalink / raw) To: Ian Jackson, Elliott Mitchell Cc: Andrew Cooper, Paul Durrant, xen-devel, Wei Liu, Jason Andryuk [-- Attachment #1: Type: text/plain, Size: 963 bytes --] On 14.05.2020 13:02, Ian Jackson wrote: > I've read this thread. Jan, I'm sorry that this causes you > inconvenience. I'm hoping it won't come down to a choice between > supporting people who want to ship a dom0 without perl, and people who > want a dom0 using more-than-a-decade-old coreutils. > > Jan, can you tell me what the output is of this on your ancient > system: > > $ rm -f t > $ >t > $ exec 3<t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 393549 > $ rm t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 393549 > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > $ $ rm -f t $ >t $ exec 3<t $ stat -L -c '%F %i' /dev/stdin <&3 regular empty file 3380369 $ rm t $ stat -L -c '%F %i' /dev/stdin <&3 regular empty file 3380369 $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 regular empty file 3380369 > Also, the contents of the file "u" afterwards, please. Attached. Thanks for looking into this, Jan [-- Attachment #2: u --] [-- Type: text/plain, Size: 13502 bytes --] execve("/usr/bin/stat", ["stat", "-L", "-c", "%F %i", "/dev/stdin"], [/* 89 vars */]) = 0 brk(0) = 0x8bd000 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fa332c89000 access("/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory) open("/usr/local/lib64/tls/x86_64/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib64/tls/x86_64", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/usr/local/lib64/tls/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib64/tls", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/usr/local/lib64/x86_64/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib64/x86_64", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/usr/local/lib64/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib64", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 open("/usr/local/lib/tls/x86_64/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib/tls/x86_64", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/usr/local/lib/tls/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib/tls", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/usr/local/lib/x86_64/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib/x86_64", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/usr/local/lib/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/usr/local/lib", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 open("/opt/intel/compiler-8.1-026/lib/tls/x86_64/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/opt/intel/compiler-8.1-026/lib/tls/x86_64", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/opt/intel/compiler-8.1-026/lib/tls/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/opt/intel/compiler-8.1-026/lib/tls", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/opt/intel/compiler-8.1-026/lib/x86_64/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/opt/intel/compiler-8.1-026/lib/x86_64", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/opt/intel/compiler-8.1-026/lib/libselinux.so.1", O_RDONLY) = -1 ENOENT (No such file or directory) stat("/opt/intel/compiler-8.1-026/lib", 0x7fffe2b6ad70) = -1 ENOENT (No such file or directory) open("/etc/ld.so.cache", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=93598, ...}) = 0 mmap(NULL, 93598, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c72000 close(4) = 0 open("/lib64/libselinux.so.1", O_RDONLY) = 4 read(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\0_\0\0\0\0\0\0"..., 832) = 832 fstat(4, {st_mode=S_IFREG|0755, st_size=118080, ...}) = 0 mmap(NULL, 2217768, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 4, 0) = 0x7fa33284e000 fadvise64(4, 0, 2217768, POSIX_FADV_WILLNEED) = 0 mprotect(0x7fa33286a000, 2093056, PROT_NONE) = 0 mmap(0x7fa332a69000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x1b000) = 0x7fa332a69000 mmap(0x7fa332a6b000, 1832, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7fa332a6b000 close(4) = 0 open("/usr/local/lib64/libc.so.6", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/local/lib/libc.so.6", O_RDONLY) = -1 ENOENT (No such file or directory) open("/lib64/libc.so.6", O_RDONLY) = 4 read(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0@\355\1\0\0\0\0\0"..., 832) = 832 fstat(4, {st_mode=S_IFREG|0755, st_size=1690993, ...}) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fa332c71000 mmap(NULL, 3557448, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 4, 0) = 0x7fa3324e9000 fadvise64(4, 0, 3557448, POSIX_FADV_WILLNEED) = 0 mprotect(0x7fa332645000, 2093056, PROT_NONE) = 0 mmap(0x7fa332844000, 20480, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x15b000) = 0x7fa332844000 mmap(0x7fa332849000, 18504, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7fa332849000 close(4) = 0 open("/usr/local/lib64/libdl.so.2", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/local/lib/libdl.so.2", O_RDONLY) = -1 ENOENT (No such file or directory) open("/lib64/libdl.so.2", O_RDONLY) = 4 read(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\360\r\0\0\0\0\0\0"..., 832) = 832 fstat(4, {st_mode=S_IFREG|0755, st_size=19149, ...}) = 0 mmap(NULL, 2109696, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 4, 0) = 0x7fa3322e5000 fadvise64(4, 0, 2109696, POSIX_FADV_WILLNEED) = 0 mprotect(0x7fa3322e7000, 2097152, PROT_NONE) = 0 mmap(0x7fa3324e7000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x2000) = 0x7fa3324e7000 close(4) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fa332c70000 mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fa332c6e000 arch_prctl(ARCH_SET_FS, 0x7fa332c6e7a0) = 0 mprotect(0x7fa3324e7000, 4096, PROT_READ) = 0 mprotect(0x7fa332844000, 16384, PROT_READ) = 0 mprotect(0x7fa332a69000, 4096, PROT_READ) = 0 mprotect(0x609000, 4096, PROT_READ) = 0 mprotect(0x7fa332c8a000, 4096, PROT_READ) = 0 munmap(0x7fa332c72000, 93598) = 0 statfs("/selinux", {f_type="EXT2_SUPER_MAGIC", f_bsize=4096, f_blocks=5126165, f_bfree=4641521, f_bavail=4379461, f_files=1310720, f_ffree=1241977, f_fsid={-304735720, 1406917494}, f_namelen=255, f_frsize=4096}) = 0 brk(0) = 0x8bd000 brk(0x8de000) = 0x8de000 open("/proc/filesystems", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0444, st_size=0, ...}) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fa332c88000 read(4, "nodev\tsysfs\nnodev\trootfs\nnodev\tr"..., 1024) = 265 read(4, "", 1024) = 0 close(4) = 0 munmap(0x7fa332c88000, 4096) = 0 open("/usr/lib/locale/locale-archive", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale/locale.alias", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=2512, ...}) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fa332c88000 read(4, "# Locale name alias data base.\n#"..., 4096) = 2512 read(4, "", 4096) = 0 close(4) = 0 munmap(0x7fa332c88000, 4096) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_IDENTIFICATION", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_IDENTIFICATION", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=373, ...}) = 0 mmap(NULL, 373, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c88000 close(4) = 0 open("/usr/lib64/gconv/gconv-modules.cache", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=26050, ...}) = 0 mmap(NULL, 26050, PROT_READ, MAP_SHARED, 4, 0) = 0x7fa332c81000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_MEASUREMENT", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_MEASUREMENT", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=23, ...}) = 0 mmap(NULL, 23, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c80000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_TELEPHONE", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_TELEPHONE", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=59, ...}) = 0 mmap(NULL, 59, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c7f000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_ADDRESS", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_ADDRESS", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=155, ...}) = 0 mmap(NULL, 155, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c7e000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_NAME", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_NAME", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=77, ...}) = 0 mmap(NULL, 77, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c7d000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_PAPER", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_PAPER", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=34, ...}) = 0 mmap(NULL, 34, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c7c000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_MESSAGES", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_MESSAGES", O_RDONLY) = 4 fstat(4, {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 close(4) = 0 open("/usr/lib/locale/en_US.utf8/LC_MESSAGES/SYS_LC_MESSAGES", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=57, ...}) = 0 mmap(NULL, 57, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c7b000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_MONETARY", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_MONETARY", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=286, ...}) = 0 mmap(NULL, 286, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c7a000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_COLLATE", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_COLLATE", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=1163682, ...}) = 0 mmap(NULL, 1163682, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332b51000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_TIME", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_TIME", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=2454, ...}) = 0 mmap(NULL, 2454, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c79000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_NUMERIC", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_NUMERIC", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=54, ...}) = 0 mmap(NULL, 54, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332c78000 close(4) = 0 open("/usr/lib/locale/en_US.UTF-8/LC_CTYPE", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/lib/locale/en_US.utf8/LC_CTYPE", O_RDONLY) = 4 fstat(4, {st_mode=S_IFREG|0644, st_size=256324, ...}) = 0 mmap(NULL, 256324, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa332b12000 close(4) = 0 stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 open("/usr/share/locale-langpack/en_US.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale/en_US.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-bundle/en_US.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-langpack/en_US.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale/en_US.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-bundle/en_US.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-langpack/en_US/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale/en_US/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-bundle/en_US/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-langpack/en.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale/en.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-bundle/en.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-langpack/en.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale/en.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-bundle/en.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-langpack/en/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale/en/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) open("/usr/share/locale-bundle/en/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory) fstat(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 4), ...}) = 0 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fa332c77000 write(1, "regular empty file 3380369\n", 27) = 27 close(1) = 0 munmap(0x7fa332c77000, 4096) = 0 close(2) = 0 exit_group(0) = ? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Ping: use of "stat -" 2020-05-18 10:34 ` Jan Beulich @ 2020-06-24 15:55 ` Jan Beulich 2020-06-24 16:19 ` [XEN RFC for-4.14] " Ian Jackson 1 sibling, 0 replies; 27+ messages in thread From: Jan Beulich @ 2020-06-24 15:55 UTC (permalink / raw) To: Ian Jackson Cc: Wei Liu, Jason Andryuk, Andrew Cooper, Paul Durrant, Elliott Mitchell, xen-devel On 18.05.2020 12:34, Jan Beulich wrote: > On 14.05.2020 13:02, Ian Jackson wrote: >> I've read this thread. Jan, I'm sorry that this causes you >> inconvenience. I'm hoping it won't come down to a choice between >> supporting people who want to ship a dom0 without perl, and people who >> want a dom0 using more-than-a-decade-old coreutils. >> >> Jan, can you tell me what the output is of this on your ancient >> system: >> >> $ rm -f t >> $ >t >> $ exec 3<t >> $ stat -L -c '%F %i' /dev/stdin <&3 >> regular empty file 393549 >> $ rm t >> $ stat -L -c '%F %i' /dev/stdin <&3 >> regular empty file 393549 >> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 >> $ > > $ rm -f t > $ >t > $ exec 3<t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > $ rm t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > >> Also, the contents of the file "u" afterwards, please. > > Attached. > > Thanks for looking into this, Jan Is there (still) a plan to get this addressed for 4.14? Apologies if I missed something having been posted / committed... Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* [XEN RFC for-4.14] Re: use of "stat -" 2020-05-18 10:34 ` Jan Beulich 2020-06-24 15:55 ` Ping: " Jan Beulich @ 2020-06-24 16:19 ` Ian Jackson 2020-06-25 2:37 ` Jason Andryuk 2020-06-25 7:03 ` Paul Durrant 1 sibling, 2 replies; 27+ messages in thread From: Ian Jackson @ 2020-06-24 16:19 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jason Andryuk, Elliott Mitchell, xen-devel Jan Beulich writes ("Re: use of "stat -""): > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. > On 14.05.2020 13:02, Ian Jackson wrote: > > I've read this thread. Jan, I'm sorry that this causes you > > inconvenience. I'm hoping it won't come down to a choice between > > supporting people who want to ship a dom0 without perl, and people who > > want a dom0 using more-than-a-decade-old coreutils. > > > > Jan, can you tell me what the output is of this on your ancient > > system: > > > > $ rm -f t > > $ >t > > $ exec 3<t > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 393549 > > $ rm t > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 393549 > > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > > $ > > $ rm -f t > $ >t > $ exec 3<t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > $ rm t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > > > Also, the contents of the file "u" afterwards, please. > > Attached. Thanks. I think this means that `stat -' can be replaced by `stat /dev/stdin'. This script is only run on Linux where /dev/stdin has existed basically forever. The strace output shows stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 and the transcript shows that your stat(1) behaves as I hope. Jan, will you send a patch ? It is best if someone else but me writes it and tests it because then I am a "clean" reviewer. Paul, supposing I review such a patch and say it is low risk, and we commit it by Friday, can it have a release-ack ? Thanks, Ian. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-24 16:19 ` [XEN RFC for-4.14] " Ian Jackson @ 2020-06-25 2:37 ` Jason Andryuk 2020-06-25 7:05 ` Jan Beulich ` (2 more replies) 2020-06-25 7:03 ` Paul Durrant 1 sibling, 3 replies; 27+ messages in thread From: Jason Andryuk @ 2020-06-25 2:37 UTC (permalink / raw) To: Ian Jackson Cc: Jan Beulich, Wei Liu, Paul Durrant, Andrew Cooper, Elliott Mitchell, xen-devel On Wed, Jun 24, 2020 at 12:19 PM Ian Jackson <ian.jackson@citrix.com> wrote: > > Jan Beulich writes ("Re: use of "stat -""): > > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. > > On 14.05.2020 13:02, Ian Jackson wrote: > > > I've read this thread. Jan, I'm sorry that this causes you > > > inconvenience. I'm hoping it won't come down to a choice between > > > supporting people who want to ship a dom0 without perl, and people who > > > want a dom0 using more-than-a-decade-old coreutils. > > > > > > Jan, can you tell me what the output is of this on your ancient > > > system: > > > > > > $ rm -f t > > > $ >t > > > $ exec 3<t > > > $ stat -L -c '%F %i' /dev/stdin <&3 > > > regular empty file 393549 > > > $ rm t > > > $ stat -L -c '%F %i' /dev/stdin <&3 > > > regular empty file 393549 > > > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > > > $ > > > > $ rm -f t > > $ >t > > $ exec 3<t > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 3380369 > > $ rm t > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 3380369 > > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 3380369 > > > > > Also, the contents of the file "u" afterwards, please. > > > > Attached. > > Thanks. > > I think this means that `stat -' can be replaced by `stat /dev/stdin'. > > This script is only run on Linux where /dev/stdin has existed > basically forever. The strace output shows > stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 > and the transcript shows that your stat(1) behaves as I hope. > > Jan, will you send a patch ? It is best if someone else but me writes > it and tests it because then I am a "clean" reviewer. > > Paul, supposing I review such a patch and say it is low risk, and we > commit it by Friday, can it have a release-ack ? I was going to just write a patch to replace - with /dev/stdin and ask Jan to test it. When I opened the script, this comment was staring at me: # 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. On my system: $ ls -l /dev/stdin lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0 $ ls -l /proc/self/fd/0 0<lockfile lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile stat /dev/stdin will work around the lack of `stat -` support, but it doesn't address the race in the comment. Is the comment valid? How would we prove there is no race for /dev/stdin? And as a refresher, `stat -` does an fstat(0), so there is no symlink lookup. Or is there no race since `stat /proc/self/fd/0` isn't a symlink lookup but just accessing the already open fd via the proc special file? i.e. equivalent to fstat. I've mentioned it before, but maybe we should just use the Qubes patch. It leaves the lockfile even when no-one is holding the lock, but it simplifies the code and sidesteps the issues we are discussing here. https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch Regards, Jason ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 2:37 ` Jason Andryuk @ 2020-06-25 7:05 ` Jan Beulich 2020-06-25 12:04 ` Jason Andryuk 2020-06-25 13:31 ` Ian Jackson 2020-06-25 7:27 ` Rich Persaud 2020-06-25 13:27 ` Ian Jackson 2 siblings, 2 replies; 27+ messages in thread From: Jan Beulich @ 2020-06-25 7:05 UTC (permalink / raw) To: Jason Andryuk, Ian Jackson Cc: Andrew Cooper, xen-devel, Wei Liu, Elliott Mitchell, Paul Durrant On 25.06.2020 04:37, Jason Andryuk wrote: > On Wed, Jun 24, 2020 at 12:19 PM Ian Jackson <ian.jackson@citrix.com> wrote: >> >> Jan Beulich writes ("Re: use of "stat -""): >>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. >>> On 14.05.2020 13:02, Ian Jackson wrote: >>>> I've read this thread. Jan, I'm sorry that this causes you >>>> inconvenience. I'm hoping it won't come down to a choice between >>>> supporting people who want to ship a dom0 without perl, and people who >>>> want a dom0 using more-than-a-decade-old coreutils. >>>> >>>> Jan, can you tell me what the output is of this on your ancient >>>> system: >>>> >>>> $ rm -f t >>>> $ >t >>>> $ exec 3<t >>>> $ stat -L -c '%F %i' /dev/stdin <&3 >>>> regular empty file 393549 >>>> $ rm t >>>> $ stat -L -c '%F %i' /dev/stdin <&3 >>>> regular empty file 393549 >>>> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 >>>> $ >>> >>> $ rm -f t >>> $ >t >>> $ exec 3<t >>> $ stat -L -c '%F %i' /dev/stdin <&3 >>> regular empty file 3380369 >>> $ rm t >>> $ stat -L -c '%F %i' /dev/stdin <&3 >>> regular empty file 3380369 >>> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 >>> regular empty file 3380369 >>> >>>> Also, the contents of the file "u" afterwards, please. >>> >>> Attached. >> >> Thanks. >> >> I think this means that `stat -' can be replaced by `stat /dev/stdin'. >> >> This script is only run on Linux where /dev/stdin has existed >> basically forever. The strace output shows >> stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 >> and the transcript shows that your stat(1) behaves as I hope. >> >> Jan, will you send a patch ? It is best if someone else but me writes >> it and tests it because then I am a "clean" reviewer. I was about to, when I saw this reply from Jason. >> Paul, supposing I review such a patch and say it is low risk, and we >> commit it by Friday, can it have a release-ack ? > > I was going to just write a patch to replace - with /dev/stdin and ask > Jan to test it. When I opened the script, this comment was staring at > me: > # 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. > > On my system: > $ ls -l /dev/stdin > lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0 > $ ls -l /proc/self/fd/0 0<lockfile > lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile > > stat /dev/stdin will work around the lack of `stat -` support, but it > doesn't address the race in the comment. Is the comment valid? How > would we prove there is no race for /dev/stdin? And as a refresher, > `stat -` does an fstat(0), so there is no symlink lookup. Or is there > no race since `stat /proc/self/fd/0` isn't a symlink lookup but just > accessing the already open fd via the proc special file? i.e. > equivalent to fstat. Looking at vfs_statx() in the kernel, I can't see any provisions to get at the data without traversing the specified path. Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 7:05 ` Jan Beulich @ 2020-06-25 12:04 ` Jason Andryuk 2020-06-25 13:31 ` Ian Jackson 1 sibling, 0 replies; 27+ messages in thread From: Jason Andryuk @ 2020-06-25 12:04 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, Andrew Cooper, Elliott Mitchell, xen-devel, Ian Jackson On Thu, Jun 25, 2020 at 3:05 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 25.06.2020 04:37, Jason Andryuk wrote: > > On Wed, Jun 24, 2020 at 12:19 PM Ian Jackson <ian.jackson@citrix.com> wrote: > >> > >> Jan Beulich writes ("Re: use of "stat -""): > >>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. > >>> On 14.05.2020 13:02, Ian Jackson wrote: > >>>> I've read this thread. Jan, I'm sorry that this causes you > >>>> inconvenience. I'm hoping it won't come down to a choice between > >>>> supporting people who want to ship a dom0 without perl, and people who > >>>> want a dom0 using more-than-a-decade-old coreutils. > >>>> > >>>> Jan, can you tell me what the output is of this on your ancient > >>>> system: > >>>> > >>>> $ rm -f t > >>>> $ >t > >>>> $ exec 3<t > >>>> $ stat -L -c '%F %i' /dev/stdin <&3 > >>>> regular empty file 393549 > >>>> $ rm t > >>>> $ stat -L -c '%F %i' /dev/stdin <&3 > >>>> regular empty file 393549 > >>>> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > >>>> $ > >>> > >>> $ rm -f t > >>> $ >t > >>> $ exec 3<t > >>> $ stat -L -c '%F %i' /dev/stdin <&3 > >>> regular empty file 3380369 > >>> $ rm t > >>> $ stat -L -c '%F %i' /dev/stdin <&3 > >>> regular empty file 3380369 > >>> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > >>> regular empty file 3380369 > >>> > >>>> Also, the contents of the file "u" afterwards, please. > >>> > >>> Attached. > >> > >> Thanks. > >> > >> I think this means that `stat -' can be replaced by `stat /dev/stdin'. > >> > >> This script is only run on Linux where /dev/stdin has existed > >> basically forever. The strace output shows > >> stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 > >> and the transcript shows that your stat(1) behaves as I hope. > >> > >> Jan, will you send a patch ? It is best if someone else but me writes > >> it and tests it because then I am a "clean" reviewer. > > I was about to, when I saw this reply from Jason. > > >> Paul, supposing I review such a patch and say it is low risk, and we > >> commit it by Friday, can it have a release-ack ? > > > > I was going to just write a patch to replace - with /dev/stdin and ask > > Jan to test it. When I opened the script, this comment was staring at > > me: > > # 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. > > > > On my system: > > $ ls -l /dev/stdin > > lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0 > > $ ls -l /proc/self/fd/0 0<lockfile > > lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile > > > > stat /dev/stdin will work around the lack of `stat -` support, but it > > doesn't address the race in the comment. Is the comment valid? How > > would we prove there is no race for /dev/stdin? And as a refresher, > > `stat -` does an fstat(0), so there is no symlink lookup. Or is there > > no race since `stat /proc/self/fd/0` isn't a symlink lookup but just > > accessing the already open fd via the proc special file? i.e. > > equivalent to fstat. > > Looking at vfs_statx() in the kernel, I can't see any provisions to > get at the data without traversing the specified path. Ian, you wrote the comment originally. Would you please clarify the scenario where there is a race? Only the lock holder is allowed to rm the lockfile, so how is there a race between rm and stat? Regards, Jason ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 7:05 ` Jan Beulich 2020-06-25 12:04 ` Jason Andryuk @ 2020-06-25 13:31 ` Ian Jackson 2020-06-25 13:48 ` Jan Beulich 1 sibling, 1 reply; 27+ messages in thread From: Ian Jackson @ 2020-06-25 13:31 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jason Andryuk, Elliott Mitchell, xen-devel Jan Beulich writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): > Looking at vfs_statx() in the kernel, I can't see any provisions to > get at the data without traversing the specified path. The question is what "traversing the path" means. How do you explain this ? $ >t $ exec 3>t $ stat -L -c '%F %i' /dev/stdin <&3 regular empty file 421124 $ ll /dev/stdin <&3 lrwxrwxrwx 1 root root 15 Jun 7 02:01 /dev/stdin -> /proc/self/fd/0 $ ll /proc/self/fd/0 <&3 l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> /home/ian/t $ mv t u $ ll /proc/self/fd/0 <&3 l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> /home/ian/u $ rm u $ ll /proc/self/fd/0 <&3 l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> '/home/ian/u (deleted)' $ stat -L -c '%F %i' /dev/stdin <&3 regular empty file 421124 $ ll -Li /dev/stdin <&3 421124 -rw-rw-r-- 0 ian ian 0 Jun 25 14:28 /dev/stdin $ Clearly it isn't actually following this synthetic symlink to "/home/ian/u (deleted)". Ian. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 13:31 ` Ian Jackson @ 2020-06-25 13:48 ` Jan Beulich 2020-06-25 14:16 ` Jason Andryuk 0 siblings, 1 reply; 27+ messages in thread From: Jan Beulich @ 2020-06-25 13:48 UTC (permalink / raw) To: Ian Jackson Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jason Andryuk, Elliott Mitchell, xen-devel On 25.06.2020 15:31, Ian Jackson wrote: > Jan Beulich writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): >> Looking at vfs_statx() in the kernel, I can't see any provisions to >> get at the data without traversing the specified path. > > The question is what "traversing the path" means. > > How do you explain this ? > > $ >t > $ exec 3>t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 421124 > $ ll /dev/stdin <&3 > lrwxrwxrwx 1 root root 15 Jun 7 02:01 /dev/stdin -> /proc/self/fd/0 > $ ll /proc/self/fd/0 <&3 > l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> /home/ian/t > $ mv t u > $ ll /proc/self/fd/0 <&3 > l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> /home/ian/u > $ rm u > $ ll /proc/self/fd/0 <&3 > l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> '/home/ian/u (deleted)' > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 421124 > $ ll -Li /dev/stdin <&3 > 421124 -rw-rw-r-- 0 ian ian 0 Jun 25 14:28 /dev/stdin > $ > > Clearly it isn't actually following this synthetic symlink to > "/home/ian/u (deleted)". Okay, so there's clearly some trickery then which don't know where to find. Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 13:48 ` Jan Beulich @ 2020-06-25 14:16 ` Jason Andryuk 0 siblings, 0 replies; 27+ messages in thread From: Jason Andryuk @ 2020-06-25 14:16 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, Andrew Cooper, Elliott Mitchell, xen-devel, Ian Jackson On Thu, Jun 25, 2020 at 9:48 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 25.06.2020 15:31, Ian Jackson wrote: > > Jan Beulich writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): > >> Looking at vfs_statx() in the kernel, I can't see any provisions to > >> get at the data without traversing the specified path. > > > > The question is what "traversing the path" means. > > > > How do you explain this ? > > > > $ >t > > $ exec 3>t > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 421124 > > $ ll /dev/stdin <&3 > > lrwxrwxrwx 1 root root 15 Jun 7 02:01 /dev/stdin -> /proc/self/fd/0 > > $ ll /proc/self/fd/0 <&3 > > l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> /home/ian/t > > $ mv t u > > $ ll /proc/self/fd/0 <&3 > > l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> /home/ian/u > > $ rm u > > $ ll /proc/self/fd/0 <&3 > > l-wx------ 1 ian ian 64 Jun 25 14:29 /proc/self/fd/0 -> '/home/ian/u (deleted)' > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 421124 > > $ ll -Li /dev/stdin <&3 > > 421124 -rw-rw-r-- 0 ian ian 0 Jun 25 14:28 /dev/stdin > > $ > > > > Clearly it isn't actually following this synthetic symlink to > > "/home/ian/u (deleted)". > > Okay, so there's clearly some trickery then which don't know where > to find. I can't say I've taken the time to read and understand all this, but the code in here https://elixir.bootlin.com/linux/latest/source/fs/proc/fd.c#L147 seems to lookup FDs to existing structs. -Jason ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 2:37 ` Jason Andryuk 2020-06-25 7:05 ` Jan Beulich @ 2020-06-25 7:27 ` Rich Persaud 2020-06-25 13:27 ` Ian Jackson 2 siblings, 0 replies; 27+ messages in thread From: Rich Persaud @ 2020-06-25 7:27 UTC (permalink / raw) To: Jason Andryuk, Ian Jackson, Marek Marczykowski-Górecki, Jan Beulich Cc: Andrew Cooper, xen-devel, Elliott Mitchell, Wei Liu, Paul Durrant On Jun 24, 2020, at 22:39, Jason Andryuk <jandryuk@gmail.com> wrote: > > On Wed, Jun 24, 2020 at 12:19 PM Ian Jackson <ian.jackson@citrix.com> wrote: >> >> Jan Beulich writes ("Re: use of "stat -""): >>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. >>>> On 14.05.2020 13:02, Ian Jackson wrote: >>>>> I've read this thread. Jan, I'm sorry that this causes you >>>>> inconvenience. I'm hoping it won't come down to a choice between >>>>> supporting people who want to ship a dom0 without perl, and people who >>>>> want a dom0 using more-than-a-decade-old coreutils. >>>>> >>>>> Jan, can you tell me what the output is of this on your ancient >>>>> system: >>>>> >>>>> $ rm -f t >>>>> $ >t >>>>> $ exec 3<t >>>>> $ stat -L -c '%F %i' /dev/stdin <&3 >>>>> regular empty file 393549 >>>>> $ rm t >>>>> $ stat -L -c '%F %i' /dev/stdin <&3 >>>>> regular empty file 393549 >>>>> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 >>>>> $ >>> >>> $ rm -f t >>> $ >t >>> $ exec 3<t >>> $ stat -L -c '%F %i' /dev/stdin <&3 >>> regular empty file 3380369 >>> $ rm t >>> $ stat -L -c '%F %i' /dev/stdin <&3 >>> regular empty file 3380369 >>> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 >>> regular empty file 3380369 >>> >>>> Also, the contents of the file "u" afterwards, please. >>> >>> Attached. >> >> Thanks. >> >> I think this means that `stat -' can be replaced by `stat /dev/stdin'. >> >> This script is only run on Linux where /dev/stdin has existed >> basically forever. The strace output shows >> stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 >> and the transcript shows that your stat(1) behaves as I hope. >> >> Jan, will you send a patch ? It is best if someone else but me writes >> it and tests it because then I am a "clean" reviewer. >> >> Paul, supposing I review such a patch and say it is low risk, and we >> commit it by Friday, can it have a release-ack ? > > I was going to just write a patch to replace - with /dev/stdin and ask > Jan to test it. When I opened the script, this comment was staring at > me: > # 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. > > On my system: > $ ls -l /dev/stdin > lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0 > $ ls -l /proc/self/fd/0 0<lockfile > lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile > > stat /dev/stdin will work around the lack of `stat -` support, but it > doesn't address the race in the comment. Is the comment valid? How > would we prove there is no race for /dev/stdin? And as a refresher, > `stat -` does an fstat(0), so there is no symlink lookup. Or is there > no race since `stat /proc/self/fd/0` isn't a symlink lookup but just > accessing the already open fd via the proc special file? i.e. > equivalent to fstat. > > I've mentioned it before, but maybe we should just use the Qubes > patch. It leaves the lockfile even when no-one is holding the lock, > but it simplifies the code and sidesteps the issues we are discussing > here. > https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch Is there any practical downside to the Qubes patch, which is already deployed on production systems? The complexity of other approaches has been reflected in code and prior discussion. If needed, the comments in the Qubes patch could be expanded, when merging that well-tested code into 4.14. Rich ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 2:37 ` Jason Andryuk 2020-06-25 7:05 ` Jan Beulich 2020-06-25 7:27 ` Rich Persaud @ 2020-06-25 13:27 ` Ian Jackson 2020-06-25 14:08 ` Jan Beulich 2 siblings, 1 reply; 27+ messages in thread From: Ian Jackson @ 2020-06-25 13:27 UTC (permalink / raw) To: Jason Andryuk Cc: Jan Beulich, Wei Liu, Paul Durrant, Andrew Cooper, Elliott Mitchell, xen-devel Jason Andryuk writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): > I was going to just write a patch to replace - with /dev/stdin and ask > Jan to test it. When I opened the script, this comment was staring at > me: > # 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. > > On my system: > $ ls -l /dev/stdin > lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0 > $ ls -l /proc/self/fd/0 0<lockfile > lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile > > stat /dev/stdin will work around the lack of `stat -` support, but it > doesn't address the race in the comment. Is the comment valid? Thanks, but: The tests in my transcript show that the comment (which I wrote) is false. It shows that stat /dev/stdin works on deleted files, and stats the right file even if the name has been rused. > How would we prove there is no race for /dev/stdin? It is easy to create the "bad" situation by hand, without racing. The transcript shows that the output from readlink(2) is a fiction and that stat works to stat the actual open-file. > I've mentioned it before, but maybe we should just use the Qubes > patch. It leaves the lockfile even when no-one is holding the lock, > but it simplifies the code and sidesteps the issues we are discussing > here. > https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch I don't like that because this locking code might be reused (or maybe already is used) in contexts with a varying lockfile filename, leaving many lockfiles. And because having lockfiles lying about might confuse sysadmins who are used to programs which use (the broken) LOCK_EX-style locking paradigm. So tl;dr: yes, we need that patch to replace - with /dev/stdin. Thanks, Ian. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 13:27 ` Ian Jackson @ 2020-06-25 14:08 ` Jan Beulich 2020-06-25 14:18 ` Jason Andryuk 2020-06-25 14:19 ` Ian Jackson 0 siblings, 2 replies; 27+ messages in thread From: Jan Beulich @ 2020-06-25 14:08 UTC (permalink / raw) To: Ian Jackson Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jason Andryuk, Elliott Mitchell, xen-devel On 25.06.2020 15:27, Ian Jackson wrote: > Jason Andryuk writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): >> I was going to just write a patch to replace - with /dev/stdin and ask >> Jan to test it. When I opened the script, this comment was staring at >> me: >> # 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. >> >> On my system: >> $ ls -l /dev/stdin >> lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0 >> $ ls -l /proc/self/fd/0 0<lockfile >> lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile >> >> stat /dev/stdin will work around the lack of `stat -` support, but it >> doesn't address the race in the comment. Is the comment valid? > > Thanks, but: > > The tests in my transcript show that the comment (which I wrote) is > false. It shows that stat /dev/stdin works on deleted files, and > stats the right file even if the name has been rused. > >> How would we prove there is no race for /dev/stdin? > > It is easy to create the "bad" situation by hand, without racing. > > The transcript shows that the output from readlink(2) is a fiction and > that stat works to stat the actual open-file. > >> I've mentioned it before, but maybe we should just use the Qubes >> patch. It leaves the lockfile even when no-one is holding the lock, >> but it simplifies the code and sidesteps the issues we are discussing >> here. >> https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch > > I don't like that because this locking code might be reused (or maybe > already is used) in contexts with a varying lockfile filename, leaving > many lockfiles. And because having lockfiles lying about might > confuse sysadmins who are used to programs which use (the broken) > LOCK_EX-style locking paradigm. > > So tl;dr: yes, we need that patch to replace - with /dev/stdin. I'm about to test this then, but to be honest I have no idea what to do with the comment. I don't think I could properly justify its deletion in the description (beyond saying it's not really true), nor would I be certain whether to e.g. leave the test -ef part there. Also is there any reason to go through two symlinks then, rather than using /proc/self/fd/$_lockfd directly? Jan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 14:08 ` Jan Beulich @ 2020-06-25 14:18 ` Jason Andryuk 2020-06-25 14:37 ` Ian Jackson 2020-06-25 14:19 ` Ian Jackson 1 sibling, 1 reply; 27+ messages in thread From: Jason Andryuk @ 2020-06-25 14:18 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, Andrew Cooper, Elliott Mitchell, xen-devel, Ian Jackson On Thu, Jun 25, 2020 at 10:08 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 25.06.2020 15:27, Ian Jackson wrote: > > Jason Andryuk writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): > >> I was going to just write a patch to replace - with /dev/stdin and ask > >> Jan to test it. When I opened the script, this comment was staring at > >> me: > >> # 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. > >> > >> On my system: > >> $ ls -l /dev/stdin > >> lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0 > >> $ ls -l /proc/self/fd/0 0<lockfile > >> lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile > >> > >> stat /dev/stdin will work around the lack of `stat -` support, but it > >> doesn't address the race in the comment. Is the comment valid? > > > > Thanks, but: > > > > The tests in my transcript show that the comment (which I wrote) is > > false. It shows that stat /dev/stdin works on deleted files, and > > stats the right file even if the name has been rused. > > > >> How would we prove there is no race for /dev/stdin? > > > > It is easy to create the "bad" situation by hand, without racing. > > > > The transcript shows that the output from readlink(2) is a fiction and > > that stat works to stat the actual open-file. > > > >> I've mentioned it before, but maybe we should just use the Qubes > >> patch. It leaves the lockfile even when no-one is holding the lock, > >> but it simplifies the code and sidesteps the issues we are discussing > >> here. > >> https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch > > > > I don't like that because this locking code might be reused (or maybe > > already is used) in contexts with a varying lockfile filename, leaving > > many lockfiles. And because having lockfiles lying about might > > confuse sysadmins who are used to programs which use (the broken) > > LOCK_EX-style locking paradigm. > > > > So tl;dr: yes, we need that patch to replace - with /dev/stdin. > > I'm about to test this then, but to be honest I have no idea what > to do with the comment. I don't think I could properly justify its > deletion in the description (beyond saying it's not really true), > nor would I be certain whether to e.g. leave the test -ef part > there. Yes, this is what made me pause yesterday. Also, I'm not sure how test -ef would be used to check the file & FD. > Also is there any reason to go through two symlinks then, rather > than using /proc/self/fd/$_lockfd directly? I think /proc/self/fd/$_lockfd should just be used to avoid playing unnecessary FD games. Regards, Jason ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 14:18 ` Jason Andryuk @ 2020-06-25 14:37 ` Ian Jackson 0 siblings, 0 replies; 27+ messages in thread From: Ian Jackson @ 2020-06-25 14:37 UTC (permalink / raw) To: Jason Andryuk Cc: Jan Beulich, Wei Liu, Paul Durrant, Andrew Cooper, Elliott Mitchell, xen-devel Jason Andryuk writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): > On Thu, Jun 25, 2020 at 10:08 AM Jan Beulich <jbeulich@suse.com> wrote: > > I'm about to test this then, but to be honest I have no idea what > > to do with the comment. I don't think I could properly justify its > > deletion in the description (beyond saying it's not really true), > > nor would I be certain whether to e.g. leave the test -ef part > > there. > > Yes, this is what made me pause yesterday. Also, I'm not sure how > test -ef would be used to check the file & FD. $ >t $ exec 3>t $ rm u rm: cannot remove 'u': No such file or directory $ ln t u $ test t -ef u && echo y y $ test /dev/stdin <&3 -ef u && echo y y $ mv t v $ test /dev/stdin <&3 -ef u && echo y y $ rm v $ test /dev/stdin <&3 -ef u && echo y y $ This appears to work. In principle we could switch to this, but (i) we would want to check that all (recent?) versions of bash do the sensible thing (ii) we are in a release freeze. > > Also is there any reason to go through two symlinks then, rather > > than using /proc/self/fd/$_lockfd directly? > > I think /proc/self/fd/$_lockfd should just be used to avoid playing > unnecessary FD games. Xen is frozen at the moment. Can we please make the minimal change, rather than the change which cleans the code up the most ? That does imply some technical debt, but has the lowest risk for the release. If you like, combine it with a second patch that changes things to use test -ef, for post-4.14 ? Thanks, Ian. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-25 14:08 ` Jan Beulich 2020-06-25 14:18 ` Jason Andryuk @ 2020-06-25 14:19 ` Ian Jackson 1 sibling, 0 replies; 27+ messages in thread From: Ian Jackson @ 2020-06-25 14:19 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jason Andryuk, Elliott Mitchell, xen-devel Jan Beulich writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""): > I'm about to test this then, but to be honest I have no idea what > to do with the comment. I don't think I could properly justify its > deletion in the description (beyond saying it's not really true), > nor would I be certain whether to e.g. leave the test -ef part > there. You should delete the comment. You could replace it with its opposite. Something like: # Although /dev/stdin (ie /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. > Also is there any reason to go through two symlinks then, rather > than using /proc/self/fd/$_lockfd directly? /dev/stdin is clearer, to my mind. (The tiny performence difference is not relevant here. It's also possible that at some point stat(1) might handle it specially.) Thanks, Ian. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [XEN RFC for-4.14] Re: use of "stat -" 2020-06-24 16:19 ` [XEN RFC for-4.14] " Ian Jackson 2020-06-25 2:37 ` Jason Andryuk @ 2020-06-25 7:03 ` Paul Durrant 1 sibling, 0 replies; 27+ messages in thread From: Paul Durrant @ 2020-06-25 7:03 UTC (permalink / raw) To: 'Ian Jackson', 'Jan Beulich' Cc: 'Andrew Cooper', xen-devel, 'Wei Liu', 'Elliott Mitchell', 'Jason Andryuk' > -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 24 June 2020 17:20 > To: Jan Beulich <jbeulich@suse.com> > Cc: Elliott Mitchell <ehem+xen@m5p.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Jason Andryuk > <jandryuk@gmail.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; xen- > devel@lists.xenproject.org > Subject: [XEN RFC for-4.14] Re: use of "stat -" > > Jan Beulich writes ("Re: use of "stat -""): > > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified > the sender and know the content is safe. > > On 14.05.2020 13:02, Ian Jackson wrote: > > > I've read this thread. Jan, I'm sorry that this causes you > > > inconvenience. I'm hoping it won't come down to a choice between > > > supporting people who want to ship a dom0 without perl, and people who > > > want a dom0 using more-than-a-decade-old coreutils. > > > > > > Jan, can you tell me what the output is of this on your ancient > > > system: > > > > > > $ rm -f t > > > $ >t > > > $ exec 3<t > > > $ stat -L -c '%F %i' /dev/stdin <&3 > > > regular empty file 393549 > > > $ rm t > > > $ stat -L -c '%F %i' /dev/stdin <&3 > > > regular empty file 393549 > > > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > > > $ > > > > $ rm -f t > > $ >t > > $ exec 3<t > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 3380369 > > $ rm t > > $ stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 3380369 > > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > > regular empty file 3380369 > > > > > Also, the contents of the file "u" afterwards, please. > > > > Attached. > > Thanks. > > I think this means that `stat -' can be replaced by `stat /dev/stdin'. > > This script is only run on Linux where /dev/stdin has existed > basically forever. The strace output shows > stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 > and the transcript shows that your stat(1) behaves as I hope. > > Jan, will you send a patch ? It is best if someone else but me writes > it and tests it because then I am a "clean" reviewer. > > Paul, supposing I review such a patch and say it is low risk, and we > commit it by Friday, can it have a release-ack ? > In principle yes but, given Jason's response, it doesn't sound like we have agreement on what the final patch will look like yet. Paul > Thanks, > Ian. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-06-25 14:37 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-12 10:58 use of "stat -" Jan Beulich 2020-05-12 14:19 ` Wei Liu 2020-05-12 14:33 ` Jan Beulich 2020-05-12 14:47 ` Andrew Cooper 2020-05-12 14:59 ` Jan Beulich 2020-05-12 15:52 ` Jason Andryuk 2020-05-12 19:50 ` Elliott Mitchell 2020-05-12 19:54 ` Andrew Cooper 2020-05-12 22:54 ` Elliott Mitchell 2020-05-14 11:02 ` Ian Jackson 2020-05-14 12:39 ` Jan Beulich 2020-05-18 10:34 ` Jan Beulich 2020-06-24 15:55 ` Ping: " Jan Beulich 2020-06-24 16:19 ` [XEN RFC for-4.14] " Ian Jackson 2020-06-25 2:37 ` Jason Andryuk 2020-06-25 7:05 ` Jan Beulich 2020-06-25 12:04 ` Jason Andryuk 2020-06-25 13:31 ` Ian Jackson 2020-06-25 13:48 ` Jan Beulich 2020-06-25 14:16 ` Jason Andryuk 2020-06-25 7:27 ` Rich Persaud 2020-06-25 13:27 ` Ian Jackson 2020-06-25 14:08 ` Jan Beulich 2020-06-25 14:18 ` Jason Andryuk 2020-06-25 14:37 ` Ian Jackson 2020-06-25 14:19 ` Ian Jackson 2020-06-25 7:03 ` Paul Durrant
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).