diff mbox series

[v2,2/5] uapi: deprecate STATX_ALL

Message ID 20181019122049.27121-2-mszeredi@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/5] orangefs: fix request_mask misuse | expand

Commit Message

Miklos Szeredi Oct. 19, 2018, 12:20 p.m. UTC
Constants of the *_ALL type can be actively harmful due to the fact that
developers will usually fail to consider the possible effects of future
changes to the definition.

Deprecate STATX_ALL in the uapi, while no damage has been done yet.

We could keep something like this around in the kernel, but there's
actually no point, since all filesystems should be explicitly checking
flags that they support and not rely on the VFS masking unknown ones out: a
flag could be known to the VFS, yet not known to the filesystem (see
orangefs bug fixed in the previous patch).

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
---
 fs/stat.c                       |  1 -
 include/uapi/linux/stat.h       | 11 ++++++++++-
 samples/statx/test-statx.c      |  2 +-
 tools/include/uapi/linux/stat.h | 11 ++++++++++-
 4 files changed, 21 insertions(+), 4 deletions(-)

Comments

David Howells Oct. 19, 2018, 3:35 p.m. UTC | #1
Miklos Szeredi <mszeredi@redhat.com> wrote:

> +/*
> + * This is deprecated, and shall remain the same value in the future.  To avoid
> + * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
> + * instead.
> + */
> +#define STATX_ALL		0x00000fffU

The comment is misleading.  STATX_ALL is *not* equivalent to
STATX_BASIC_STATS | STATX_BTIME, even though it might be numerically the
same.  You would need to update the comment when you add STATX_ATTRIBUTES
to mention that also.

Apart from that, I'm okay with this.

Further, please provide a manpage update also.

David
Miklos Szeredi Oct. 19, 2018, 3:40 p.m. UTC | #2
On Fri, Oct 19, 2018 at 5:35 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> +/*
>> + * This is deprecated, and shall remain the same value in the future.  To avoid
>> + * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
>> + * instead.
>> + */
>> +#define STATX_ALL            0x00000fffU
>
> The comment is misleading.  STATX_ALL is *not* equivalent to
> STATX_BASIC_STATS | STATX_BTIME, even though it might be numerically the
> same.  You would need to update the comment when you add STATX_ATTRIBUTES
> to mention that also.

The definition of STATX_ALL is, and will remain, equivalent to
STATX_BASIC_STATS | STATX_BTIME.

What is misleading about this?

If you feel confused by this comment, then maybe I should just drop that part.

Thanks,
Miklos
David Howells Oct. 19, 2018, 3:47 p.m. UTC | #3
Miklos Szeredi <miklos@szeredi.hu> wrote:

> What is misleading about this?

The manpage says:

           STATX_ALL           [All currently available fields]

> If you feel confused by this comment, then maybe I should just drop that part.

I think that would be better.  Don't try to give it a definition.

David
Miklos Szeredi Oct. 19, 2018, 5:30 p.m. UTC | #4
On Fri, Oct 19, 2018 at 5:47 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
>> What is misleading about this?
>
> The manpage says:
>
>            STATX_ALL           [All currently available fields]

Ah, the manpage needs to be fixed, obviously.

>
>> If you feel confused by this comment, then maybe I should just drop that part.
>
> I think that would be better.  Don't try to give it a definition.

Okay.

Thanks,
Miklos
diff mbox series

Patch

diff --git a/fs/stat.c b/fs/stat.c
index f8e6fb2c3657..8d297a279991 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -73,7 +73,6 @@  int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 
 	memset(stat, 0, sizeof(*stat));
 	stat->result_mask |= STATX_BASIC_STATS;
-	request_mask &= STATX_ALL;
 	query_flags &= KSTAT_QUERY_FLAGS;
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(path, stat, request_mask,
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 7b35e98d3c58..ed456ac0f90d 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -148,9 +148,18 @@  struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
-#define STATX_ALL		0x00000fffU	/* All currently supported flags */
+
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
+#ifndef __KERNEL__
+/*
+ * This is deprecated, and shall remain the same value in the future.  To avoid
+ * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
+ * instead.
+ */
+#define STATX_ALL		0x00000fffU
+#endif
+
 /*
  * Attributes to be found in stx_attributes and masked in stx_attributes_mask.
  *
diff --git a/samples/statx/test-statx.c b/samples/statx/test-statx.c
index d4d77b09412c..e354048dea3c 100644
--- a/samples/statx/test-statx.c
+++ b/samples/statx/test-statx.c
@@ -211,7 +211,7 @@  int main(int argc, char **argv)
 	struct statx stx;
 	int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
 
-	unsigned int mask = STATX_ALL;
+	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
 
 	for (argv++; *argv; argv++) {
 		if (strcmp(*argv, "-F") == 0) {
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
index 7b35e98d3c58..ed456ac0f90d 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h
@@ -148,9 +148,18 @@  struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
-#define STATX_ALL		0x00000fffU	/* All currently supported flags */
+
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
+#ifndef __KERNEL__
+/*
+ * This is deprecated, and shall remain the same value in the future.  To avoid
+ * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
+ * instead.
+ */
+#define STATX_ALL		0x00000fffU
+#endif
+
 /*
  * Attributes to be found in stx_attributes and masked in stx_attributes_mask.
  *