Add tests for utimensat(2) #6

asomers · 2017-01-26T00:40:38Z

Add tests for utimensat(2). They pass on FreeBSD/UFS. They fail on FreeBSD/ZFS (but I have a fix ready). I haven't tested them on Linux or Illumos.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c * In zfs_freebsd_setattr, if the caller wants to set the birthtime, set the bits that zfs_settattr expects * In zfs_setattr, if XAT_CREATETIME is set, set xoa_createtime, expected by zfs_xvattr_set. The two levels of indirection seem excessive, but it minimizes diffs vs OpenZFS. * In zfs_setattr, check for overflow of va_birthtime (from delphij) * Remove red herring in zfs_getattr sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h * Un-booby-trap some macros New tests are under review at pjd/pjdfstest#6 Reviewed by: avg MFC after: 3 weeks Sponsored by: Spectra Logic Corp Differential Revision: https://reviews.freebsd.org/D9353 git-svn-id: svn+ssh://svn.freebsd.org/base/head@313483 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c * In zfs_freebsd_setattr, if the caller wants to set the birthtime, set the bits that zfs_settattr expects * In zfs_setattr, if XAT_CREATETIME is set, set xoa_createtime, expected by zfs_xvattr_set. The two levels of indirection seem excessive, but it minimizes diffs vs OpenZFS. * In zfs_setattr, check for overflow of va_birthtime (from delphij) * Remove red herring in zfs_getattr sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h * Un-booby-trap some macros New tests are under review at pjd/pjdfstest#6 Reviewed by: avg MFC after: 3 weeks Sponsored by: Spectra Logic Corp Differential Revision: https://reviews.freebsd.org/D9353

ngie-eign

I took a first-pass at the change. I'll look at it further when I have some time to do so.

ngie-eign · 2017-02-09T23:21:06Z

pjdfstest.c

+		printf("%lld", (long long)sp->st_mtime);
+	else if (strcmp(what, "mtime_ns") == 0)
+		printf("%lld", (long long)sp->st_mtim.tv_nsec);
+	else if (strcmp(what, "birthtime") == 0)


A #ifdef will need to be added for this: FreeBSD is the only OS that supports .birthtime in struct stat.

ngie-eign · 2017-02-09T23:28:31Z

tests/utimensat/00.t

+cd ${n1}
+
+DATE1=1900000000 #Sun Mar 17 11:46:40 MDT 2030
+DATE2=1950000000 #Fri Oct 17 04:40:00 MDT 2031


Is this a problem with architectures/OSes that don't support 64-bit time_t?

Nope. I chose all of those constants to fit within 32 bits.

ngie-eign · 2017-02-09T23:30:33Z

tests/utimensat/00.t

+
+expect 0 mkdir ${n1} 0755
+cdir=`pwd`
+cd ${n1}


This should probably be expect-ed so we're sure that we're going to the proper directory before and after the test and not just littering temporary files where the test is being run from... whether or not pjdfstest does this 100% of the time elsewhere though is a good question. If not, then it should be fixed everywhere in a separate commit.

No, pjdfstest generally doesn't do that. Let's do that separately.

ngie-eign · 2017-02-09T23:31:41Z

tests/utimensat/01.t

+	echo "not ok 4 mtime was not updated"
+fi
+if [ "$delta_atime" -gt 0 ]; then
+	if [ "$delta_atime" -lt 300 ]; then


Again, why 5 mins?

ngie-eign · 2017-02-09T23:31:59Z

tests/utimensat/01.t

+delta_atime=$(( $new_atime - $old_atime ))
+
+if [ "$delta_mtime" -gt 0 ]; then
+	if [ "$delta_mtime" -lt 300 ]; then


Why 5 mins?

Why isn't this value a constant?

There's no way for pjdfstest to know exactly what time will get set when it says UTIME_NOW. So it checks for anything that's "approximately now". It can't even know what direction the error will be, because ntp or something may come and change the clock. Five minutes is a pretty arbitrary margin of error. I'll constantify it.

ngie-eign · 2017-02-09T23:35:00Z

tests/utimensat/01.t

+. ${dir}/../misc.sh
+echo "1..7"
+
+require utimensat


These calls should be moved up before the test plan summaries -- the reason why is that if the require fails, then it'll print out something like:

1..7 1..1

This will confuse the TAP parsers. I know this will cause kyua to choke at least (and I think prove will reject the test because of the test plan mismatch).

Actually, I'll just delete it, because utimensat is part of POSIX and AFAIK everything supports it (but I don't have a OSX machine to check). I'll add a separate require statement for the birthtime-specific tests.

ngie-eign · 2017-02-09T23:36:21Z

tests/utimensat/01.t

+
+require utimensat
+
+UTIME_NOW=-1


It would be better for these constants to be symbolic and referenced in the C code -- there's no telling whether or not all of the constants are defined with the same values.

good call. Looks like it's different on Linux.

ngie-eign · 2017-02-09T23:38:13Z

tests/utimensat/03.t

+#! /bin/sh
+# $FreeBSD$
+
+desc="utimensat can update birthtimes"


Please skip if not run on FreeBSD.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c * In zfs_freebsd_setattr, if the caller wants to set the birthtime, set the bits that zfs_settattr expects * In zfs_setattr, if XAT_CREATETIME is set, set xoa_createtime, expected by zfs_xvattr_set. The two levels of indirection seem excessive, but it minimizes diffs vs OpenZFS. * In zfs_setattr, check for overflow of va_birthtime (from delphij) * Remove red herring in zfs_getattr sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h * Un-booby-trap some macros New tests are under review at pjd/pjdfstest#6 Reviewed by: avg MFC after: 3 weeks Sponsored by: Spectra Logic Corp Differential Revision: https://reviews.freebsd.org/D9353 git-svn-id: svn+ssh://svn.freebsd.org/base/head@313483 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c * In zfs_freebsd_setattr, if the caller wants to set the birthtime, set the bits that zfs_settattr expects * In zfs_setattr, if XAT_CREATETIME is set, set xoa_createtime, expected by zfs_xvattr_set. The two levels of indirection seem excessive, but it minimizes diffs vs OpenZFS. * In zfs_setattr, check for overflow of va_birthtime (from delphij) * Remove red herring in zfs_getattr sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h * Un-booby-trap some macros New tests are under review at pjd/pjdfstest#6 Reviewed by: avg MFC after: 3 weeks Sponsored by: Spectra Logic Corp Differential Revision: https://reviews.freebsd.org/D9353

asomers · 2017-03-08T18:49:32Z

@pjd are you interested in this PR? If not, I can submit it directly to FreeBSD.

ngie-eign

These changes look good to me.

Fix setting birthtime in ZFS sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c * In zfs_freebsd_setattr, if the caller wants to set the birthtime, set the bits that zfs_settattr expects * In zfs_setattr, if XAT_CREATETIME is set, set xoa_createtime, expected by zfs_xvattr_set. The two levels of indirection seem excessive, but it minimizes diffs vs OpenZFS. * In zfs_setattr, check for overflow of va_birthtime (from delphij) * Remove red herring in zfs_getattr sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h * Un-booby-trap some macros New tests are under review at pjd/pjdfstest#6 Reviewed by: avg MFC after: 3 weeks Sponsored by: Spectra Logic Corp Differential Revision: https://reviews.freebsd.org/D9353

The st_birthtime tests are only supported on FreeBSD, but everything else is universal.

ngie-eign · 2017-04-03T01:53:07Z

This change unfortunately broke OS X again. I'm trying to figure out more details about struct stat between the OSes. Once I have an idea of how things are split up, I'll send out another PR.

Fix setting birthtime in ZFS sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c * In zfs_freebsd_setattr, if the caller wants to set the birthtime, set the bits that zfs_settattr expects * In zfs_setattr, if XAT_CREATETIME is set, set xoa_createtime, expected by zfs_xvattr_set. The two levels of indirection seem excessive, but it minimizes diffs vs OpenZFS. * In zfs_setattr, check for overflow of va_birthtime (from delphij) * Remove red herring in zfs_getattr sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h * Un-booby-trap some macros New tests are under review at pjd/pjdfstest#6 Reviewed by: avg MFC after: 3 weeks Sponsored by: Spectra Logic Corp Differential Revision: https://reviews.freebsd.org/D9353 git-svn-id: https://svn.freebsd.org/base/stable/11@316391 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

ngie-eign requested changes Feb 9, 2017

View reviewed changes

ngie-eign approved these changes Mar 9, 2017

View reviewed changes

Add tests for utimensat(2)

f65d99a

The st_birthtime tests are only supported on FreeBSD, but everything else is universal.

asomers force-pushed the master branch from 18eea24 to f65d99a Compare April 2, 2017 17:14

asomers merged commit 81e28ab into pjd:master Apr 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for utimensat(2) #6

Add tests for utimensat(2) #6

asomers commented Jan 26, 2017

ngie-eign left a comment

ngie-eign Feb 9, 2017

ngie-eign Feb 9, 2017

asomers Feb 10, 2017 •

edited

Loading

ngie-eign Feb 9, 2017

asomers Feb 10, 2017

ngie-eign Feb 9, 2017

ngie-eign Feb 9, 2017

asomers Feb 11, 2017

ngie-eign Feb 9, 2017

asomers Feb 10, 2017

ngie-eign Feb 9, 2017

asomers Feb 10, 2017

ngie-eign Feb 9, 2017

asomers commented Mar 8, 2017

ngie-eign left a comment

ngie-eign commented Apr 3, 2017 •

edited

Loading

Add tests for utimensat(2) #6

Add tests for utimensat(2) #6

Conversation

asomers commented Jan 26, 2017

ngie-eign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Mar 8, 2017

ngie-eign left a comment

Choose a reason for hiding this comment

ngie-eign commented Apr 3, 2017 • edited Loading

asomers Feb 10, 2017 •

edited

Loading

ngie-eign commented Apr 3, 2017 •

edited

Loading