Hi, Please see my comments about your proposed patch below.
Also, I have uploaded to git tests for utime(2), can you please check them with xfs ? http://sourceforge.net/p/ntfs-3g/pjd-fstest/ci/master/tree/ Regards Jean-Pierre Zorro Lang wrote: > re-write the test cases about(chown): > "If owner or group is specified as ( uid_t)-1 or ( gid_t)-1, respectively, > the corresponding ID of the file shall not be changed. If both owner and > group are -1, the times need not be updated." > --- > > In linux system, the chown/00.t run failed in xfs file system. The reason is > this test case think the ctime will not be changed after chown -1,-1 in > Linux:xfs, but the linux kernel always changes the ctime in VFS in this > condition , no matter what fs is running. > > The posix about chown said: > "If owner or group is specified as ( uid_t)-1 or ( gid_t)-1, respectively, > the corresponding ID of the file shall not be changed. If both owner and > group are -1, the times need not be updated." > > But the linux manual about chown(man 2 chown) said: > "If the owner or group is specified as -1, then that ID is not changed." > > So I think there are two things should be amended: > 1) the important thing is test the ID not be changed. > 2) then test the ctime "need not" to be update. > > I don't know why use "need not", not use "must not" or "should not", I think > maybe the ctime be updated or not, all is OK. ([email protected] talk about > this with me first.) Following previous discussions, it appears the specification is unclear, but there are only two possibilities, and for a given implementation there is a single one. > > > tests/chown/00.t | 53 ++++++++++++++++++----------------------------------- > 1 file changed, 18 insertions(+), 35 deletions(-) > > diff --git a/tests/chown/00.t b/tests/chown/00.t > index 577170d..1063dd8 100755 > --- a/tests/chown/00.t > +++ b/tests/chown/00.t > @@ -7,9 +7,9 @@ dir=`dirname $0` > . ${dir}/../misc.sh > > if supported lchmod; then > - echo "1..186" > + echo "1..190" > else > - echo "1..171" > + echo "1..175" > fi > > n0=`namegen` > @@ -278,65 +278,48 @@ test_check $ctime1 -lt $ctime2 > expect 0 unlink ${n0} > # 154 > expect 0 create ${n0} 0644 > +fuid=`${fstest} stat ${n0} uid` > +fgid=`${fstest} stat ${n0} gid` > ctime1=`${fstest} stat ${n0} ctime` > sleep 1 > expect 0 -- chown ${n0} -1 -1 > +expect $fuid,$fgid stat ${n0} uid,gid > ctime2=`${fstest} stat ${n0} ctime` > -case "${os}:${fs}" in > -Linux:ext3) > - test_check $ctime1 -lt $ctime2 > - ;; > -*) > - test_check $ctime1 -eq $ctime2 > - ;; > -esac > +test_check $ctime1 -le $ctime2 I do not agree with this change : the Posix specification is unclear, and some implementers have chosen to update ctime whereas other implementers have kept it unchanged. But the test is also aimed at checking for no regression after some change, and if the behavior for ctime changes for some implementation, it has to be detected. This is unfortunate, but the test has to accept both strategies and identify unwanted changes. So if linux:ext3 returns the old value, it has to be detected, and if sunos:zfs returns a changed value, it also has to be detected. In other words, I prefer you add an linux:xfs case where appropriate (probably like linux:ext3). At about line 170 in the same test, there is another situation where Linux behave differently from Solaris in dropping the setuid/setgid flags, but you did not mention any change there. Do you confirm xfs on Linux behaves like Solaris and drops the flags ? > expect 0 unlink ${n0} > # 158 > expect 0 mkdir ${n0} 0644 > +fuid=`${fstest} stat ${n0} uid` > +fgid=`${fstest} stat ${n0} gid` > ctime1=`${fstest} stat ${n0} ctime` > sleep 1 > expect 0 -- chown ${n0} -1 -1 > +expect $fuid,$fgid stat ${n0} uid,gid This test for uid and gid being unchanged is probably redundant, as a similar test has been done above (unless there is a significant difference, of course). > ctime2=`${fstest} stat ${n0} ctime` > -case "${os}:${fs}" in > -Linux:ext3) > - test_check $ctime1 -lt $ctime2 > - ;; > -*) > - test_check $ctime1 -eq $ctime2 > - ;; > -esac > +test_check $ctime1 -le $ctime2 > expect 0 rmdir ${n0} > # 162 > expect 0 mkfifo ${n0} 0644 > +fuid=`${fstest} stat ${n0} uid` > +fgid=`${fstest} stat ${n0} gid` > ctime1=`${fstest} stat ${n0} ctime` > sleep 1 > expect 0 -- chown ${n0} -1 -1 > +expect $fuid,$fgid stat ${n0} uid,gid > ctime2=`${fstest} stat ${n0} ctime` > -case "${os}:${fs}" in > -Linux:ext3) > - test_check $ctime1 -lt $ctime2 > - ;; > -*) > - test_check $ctime1 -eq $ctime2 > - ;; > -esac > +test_check $ctime1 -le $ctime2 > expect 0 unlink ${n0} > # 166 > expect 0 symlink ${n1} ${n0} > +fuid=`${fstest} lstat ${n0} uid` > +fgid=`${fstest} lstat ${n0} gid` > ctime1=`${fstest} lstat ${n0} ctime` > sleep 1 > expect 0 -- lchown ${n0} -1 -1 > +expect $fuid,$fgid lstat ${n0} uid,gid > ctime2=`${fstest} lstat ${n0} ctime` > -case "${os}:${fs}" in > -Linux:ext3) > - test_check $ctime1 -lt $ctime2 > - ;; > -*) > - test_check $ctime1 -eq $ctime2 > - ;; > -esac > +test_check $ctime1 -le $ctime2 > expect 0 unlink ${n0} > - > # unsuccessful chown(2) does not update ctime. > # 170 > expect 0 create ${n0} 0644 > ------------------------------------------------------------------------------ Shape the Mobile Experience: Free Subscription Software experts and developers: Be at the forefront of tech innovation. Intel(R) Software Adrenaline delivers strategic insight and game-changing conversations that shape the rapidly evolving mobile landscape. Sign up now. http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk _______________________________________________ ntfs-3g-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
