linux-api.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] uapi: remove false assertion that statx_timestamp.tv_nsec can be negative
@ 2017-04-22 19:27 Dmitry V. Levin
  2017-04-26 10:55 ` David Howells
  0 siblings, 1 reply; 3+ messages in thread
From: Dmitry V. Levin @ 2017-04-22 19:27 UTC (permalink / raw)
  To: David Howells, Al Viro
  Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA

The comment asserting that the value of struct statx_timestamp.tv_nsec
must be negative when statx_timestamp.tv_sec is negative, is wrong, as
could be seen from the following example:

	#define _FILE_OFFSET_BITS 64
	#include <assert.h>
	#include <fcntl.h>
	#include <stdio.h>
	#include <sys/stat.h>
	#include <unistd.h>
	#include <asm/unistd.h>
	#include <linux/stat.h>

	int main(void)
	{
		static const struct timespec ts[2] = {
			{ .tv_nsec = UTIME_OMIT },
			{ .tv_sec = -2, .tv_nsec = 42 }
		};
		assert(utimensat(AT_FDCWD, ".", ts, 0) == 0);

		struct stat st;
		assert(stat(".", &st) == 0);
		printf("st_mtim.tv_sec = %lld, st_mtim.tv_nsec = %lu\n",
		       (long long) st.st_mtim.tv_sec,
		       (unsigned long) st.st_mtim.tv_nsec);

		struct statx stx;
		assert(syscall(__NR_statx, AT_FDCWD, ".", 0, 0, &stx) == 0);
		printf("stx_mtime.tv_sec = %lld, stx_mtime.tv_nsec = %lu\n",
		       (long long) stx.stx_mtime.tv_sec,
		       (unsigned long) stx.stx_mtime.tv_nsec);

		return 0;
	}

It expectedly prints:
st_mtim.tv_sec = -2, st_mtim.tv_nsec = 42
stx_mtime.tv_sec = -2, stx_mtime.tv_nsec = 42

The more generic comment asserting that the value of struct
statx_timestamp.tv_nsec might be negative is confusing to say the least.

It contradicts both to the struct stat.st_[acm]time_nsec tradition
and struct timespec.tv_nsec requirements in utimensat syscall.
If statx syscall ever returns a stx_[acm]time containing a negative
tv_nsec that cannot be passed unmodified to utimensat syscall,
it will cause an immense confusion.

The best fix for this source of confusion is to change the type
of struct statx_timestamp.tv_nsec from __s32 to __u32 .

Fixes: a528d35e8bfc ("statx: Add a system call to make enhanced file info available")
Signed-off-by: Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
---
 include/uapi/linux/stat.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index d538897..8b0cba6 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -45,17 +45,13 @@
 /*
  * Timestamp structure for the timestamps in struct statx.
  *
  * tv_sec holds the number of seconds before (negative) or after (positive)
  * 00:00:00 1st January 1970 UTC.
  *
- * tv_nsec holds a number of nanoseconds before (0..-999,999,999 if tv_sec is
- * negative) or after (0..999,999,999 if tv_sec is positive) the tv_sec time.
- *
- * Note that if both tv_sec and tv_nsec are non-zero, then the two values must
- * either be both positive or both negative.
+ * tv_nsec holds a number of nanoseconds (0..999,999,999) after the tv_sec time.
  *
  * __reserved is held in case we need a yet finer resolution.
  */
 struct statx_timestamp {
 	__s64	tv_sec;
 	__s32	tv_nsec;

-- 
ldv

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] uapi: remove false assertion that statx_timestamp.tv_nsec can be negative
  2017-04-22 19:27 [PATCH] uapi: remove false assertion that statx_timestamp.tv_nsec can be negative Dmitry V. Levin
@ 2017-04-26 10:55 ` David Howells
       [not found]   ` <13744.1493204125-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>
  0 siblings, 1 reply; 3+ messages in thread
From: David Howells @ 2017-04-26 10:55 UTC (permalink / raw)
  To: Dmitry V. Levin; +Cc: dhowells, Al Viro, linux-api, linux-kernel, mtk.manpages

Dmitry V. Levin <ldv@altlinux.org> wrote:

> remove false assertion

It's not a false assertion.  I defined the statx interface that way.

> The best fix for this source of confusion is to change the type
> of struct statx_timestamp.tv_nsec from __s32 to __u32 .

Okay, if we're going to do the comment change, we should effect the type
change also.  Do you want to update your patch?

David

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH v2] uapi: change the type of struct statx_timestamp.tv_nsec to unsigned
       [not found]   ` <13744.1493204125-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>
@ 2017-04-26 11:19     ` Dmitry V. Levin
  0 siblings, 0 replies; 3+ messages in thread
From: Dmitry V. Levin @ 2017-04-26 11:19 UTC (permalink / raw)
  To: David Howells
  Cc: Al Viro, linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w

The comment asserting that the value of struct statx_timestamp.tv_nsec
must be negative when statx_timestamp.tv_sec is negative, is wrong, as
could be seen from the following example:

	#define _FILE_OFFSET_BITS 64
	#include <assert.h>
	#include <fcntl.h>
	#include <stdio.h>
	#include <sys/stat.h>
	#include <unistd.h>
	#include <asm/unistd.h>
	#include <linux/stat.h>

	int main(void)
	{
		static const struct timespec ts[2] = {
			{ .tv_nsec = UTIME_OMIT },
			{ .tv_sec = -2, .tv_nsec = 42 }
		};
		assert(utimensat(AT_FDCWD, ".", ts, 0) == 0);

		struct stat st;
		assert(stat(".", &st) == 0);
		printf("st_mtim.tv_sec = %lld, st_mtim.tv_nsec = %lu\n",
		       (long long) st.st_mtim.tv_sec,
		       (unsigned long) st.st_mtim.tv_nsec);

		struct statx stx;
		assert(syscall(__NR_statx, AT_FDCWD, ".", 0, 0, &stx) == 0);
		printf("stx_mtime.tv_sec = %lld, stx_mtime.tv_nsec = %lu\n",
		       (long long) stx.stx_mtime.tv_sec,
		       (unsigned long) stx.stx_mtime.tv_nsec);

		return 0;
	}

It expectedly prints:
st_mtim.tv_sec = -2, st_mtim.tv_nsec = 42
stx_mtime.tv_sec = -2, stx_mtime.tv_nsec = 42

The more generic comment asserting that the value of struct
statx_timestamp.tv_nsec might be negative is confusing to say the least.

It contradicts both the struct stat.st_[acm]time_nsec tradition and
struct timespec.tv_nsec requirements in utimensat syscall.
If statx syscall ever returns a stx_[acm]time containing a negative
tv_nsec that cannot be passed unmodified to utimensat syscall,
it will cause an immense confusion.

Fix this source of confusion by changing the type of struct
statx_timestamp.tv_nsec from __s32 to __u32.

Fixes: a528d35e8bfc ("statx: Add a system call to make enhanced file info available")
Signed-off-by: Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
---
v1: fix misleading comments
v2: change the type from __s32 to __u32
---
 include/uapi/linux/stat.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index d538897..17b1030 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -48,17 +48,13 @@
  * tv_sec holds the number of seconds before (negative) or after (positive)
  * 00:00:00 1st January 1970 UTC.
  *
- * tv_nsec holds a number of nanoseconds before (0..-999,999,999 if tv_sec is
- * negative) or after (0..999,999,999 if tv_sec is positive) the tv_sec time.
- *
- * Note that if both tv_sec and tv_nsec are non-zero, then the two values must
- * either be both positive or both negative.
+ * tv_nsec holds a number of nanoseconds (0..999,999,999) after the tv_sec time.
  *
  * __reserved is held in case we need a yet finer resolution.
  */
 struct statx_timestamp {
 	__s64	tv_sec;
-	__s32	tv_nsec;
+	__u32	tv_nsec;
 	__s32	__reserved;
 };
 
-- 
ldv

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-04-26 11:19 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-22 19:27 [PATCH] uapi: remove false assertion that statx_timestamp.tv_nsec can be negative Dmitry V. Levin
2017-04-26 10:55 ` David Howells
     [not found]   ` <13744.1493204125-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>
2017-04-26 11:19     ` [PATCH v2] uapi: change the type of struct statx_timestamp.tv_nsec to unsigned Dmitry V. Levin

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).