[PATCH 0/3] nfsd: implement OPEN_XOR_DELEGATION part of delstid draft

[PATCH 0/3] nfsd: implement OPEN_XOR_DELEGATION part of delstid draft

[Jeff Layton]

This adds support for part of the "delstid" draft:

    https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/

Specifically, this adds support for the OPEN_XOR_DELEGATION part of the
draft. That allows the client to avoid issuing CLOSE compounds when it
holds a delegation.

For the XDR handling, I used Chuck's new lkxdrgen tool to generate the
relevant boilerplate, and then hand tweaked it in places to work around
bugs in the decoder, naming conflicts, etc.

I've left the encoders and decoders for the delegated timestamp handling
in the XDR patch, but I'm still studying that piece and it isn't
implemented yet. That may require some timestamp handling surgery at the
VFS layer. I think it's doable and may actually be simpler to implement
on top of the multigrain work.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (3):
      nfsd: bring in support for delstid draft XDR encoding
      nfsd: add support for FATTR4_OPEN_ARGUMENTS
      nfsd: implement OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION

 fs/nfsd/Makefile          |   2 +-
 fs/nfsd/delstid_xdr.c     | 464 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/delstid_xdr.h     | 105 +++++++++++
 fs/nfsd/nfs4state.c       |  29 ++-
 fs/nfsd/nfs4xdr.c         |  57 +++++-
 fs/nfsd/nfsd.h            |   3 +-
 include/uapi/linux/nfs4.h |   7 +-
 7 files changed, 658 insertions(+), 9 deletions(-)
---
base-commit: 6d0cd2727ed2cf725b1f20dc4e2d0d138c1cf117
change-id: 20240815-delstid-93290691ad11

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton]

This adds support for the "delstid" draft:

    https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/

Most of this was autogenerated using Chuck's lkxdrgen tool with some
by-hand tweaks to work around some symbol conflicts, and to add some
missing pieces that were needed from nfs4_1.x.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/Makefile      |   2 +-
 fs/nfsd/delstid_xdr.c | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/delstid_xdr.h | 102 +++++++++++
 fs/nfsd/nfs4xdr.c     |   1 +
 4 files changed, 568 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
index b8736a82e57c..187fa45640e6 100644
--- a/fs/nfsd/Makefile
+++ b/fs/nfsd/Makefile
@@ -18,7 +18,7 @@ nfsd-$(CONFIG_NFSD_V2) += nfsproc.o nfsxdr.o
 nfsd-$(CONFIG_NFSD_V2_ACL) += nfs2acl.o
 nfsd-$(CONFIG_NFSD_V3_ACL) += nfs3acl.o
 nfsd-$(CONFIG_NFSD_V4)	+= nfs4proc.o nfs4xdr.o nfs4state.o nfs4idmap.o \
-			   nfs4acl.o nfs4callback.o nfs4recover.o
+			   nfs4acl.o nfs4callback.o nfs4recover.o delstid_xdr.o
 nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
 nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
 nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
diff --git a/fs/nfsd/delstid_xdr.c b/fs/nfsd/delstid_xdr.c
new file mode 100644
index 000000000000..63494d14f5d2
--- /dev/null
+++ b/fs/nfsd/delstid_xdr.c
@@ -0,0 +1,464 @@
+// SPDX-License-Identifier: GPL-2.0
+// Generated by lkxdrgen, with hand-edits.
+// XDR specification modification time: Wed Aug 14 13:35:03 2024
+
+#include "delstid_xdr.h"
+
+static inline bool
+xdrgen_decode_void(struct xdr_stream *xdr)
+{
+	return true;
+}
+
+static inline bool
+xdrgen_decode_bool(struct xdr_stream *xdr, bool *ptr)
+{
+	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*ptr = (*p != xdr_zero);
+	return true;
+}
+
+static inline bool
+xdrgen_decode_int(struct xdr_stream *xdr, s32 *ptr)
+{
+	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*ptr = be32_to_cpup(p);
+	return true;
+}
+
+static inline bool
+xdrgen_decode_unsigned_int(struct xdr_stream *xdr, u32 *ptr)
+{
+	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*ptr = be32_to_cpup(p);
+	return true;
+}
+
+static inline bool
+xdrgen_decode_uint32_t(struct xdr_stream *xdr, u32 *ptr)
+{
+	return xdrgen_decode_unsigned_int(xdr, ptr);
+}
+
+static inline bool
+xdrgen_decode_long(struct xdr_stream *xdr, s32 *ptr)
+{
+	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*ptr = be32_to_cpup(p);
+	return true;
+}
+
+static inline bool
+xdrgen_decode_unsigned_long(struct xdr_stream *xdr, u32 *ptr)
+{
+	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*ptr = be32_to_cpup(p);
+	return true;
+}
+
+static inline bool
+xdrgen_decode_hyper(struct xdr_stream *xdr, s64 *ptr)
+{
+	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT * 2);
+
+	if (unlikely(!p))
+		return false;
+	*ptr = get_unaligned_be64(p);
+	return true;
+}
+
+static inline bool
+xdrgen_decode_int64_t(struct xdr_stream *xdr, s64 *ptr)
+{
+	return xdrgen_decode_hyper(xdr, ptr);
+}
+
+static inline bool
+xdrgen_decode_unsigned_hyper(struct xdr_stream *xdr, u64 *ptr)
+{
+	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT * 2);
+
+	if (unlikely(!p))
+		return false;
+	*ptr = get_unaligned_be64(p);
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_opaque(struct xdr_stream *xdr, opaque *ptr, u32 maxlen)
+{
+	__be32 *p;
+	u32 len;
+
+	if (unlikely(xdr_stream_decode_u32(xdr, &len) != XDR_UNIT))
+		return false;
+	if (unlikely(maxlen && len > maxlen))
+		return false;
+	if (len != 0) {
+		p = xdr_inline_decode(xdr, len);
+		if (unlikely(!p))
+			return false;
+		ptr->data = (u8 *)p;
+	}
+	ptr->len = len;
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_string(struct xdr_stream *xdr, string *ptr, u32 maxlen)
+{
+	__be32 *p;
+	u32 len;
+
+	if (unlikely(xdr_stream_decode_u32(xdr, &len) != XDR_UNIT))
+		return false;
+	if (unlikely(maxlen && len > maxlen))
+		return false;
+	if (len != 0) {
+		p = xdr_inline_decode(xdr, len);
+		if (unlikely(!p))
+			return false;
+		ptr->data = (unsigned char *)p;
+	}
+	ptr->len = len;
+	return true;
+}
+
+static inline bool
+xdrgen_encode_void(struct xdr_stream *xdr)
+{
+	return true;
+}
+
+static inline bool
+xdrgen_encode_bool(struct xdr_stream *xdr, bool val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*p = val ? xdr_one : xdr_zero;
+	return true;
+}
+
+static inline bool
+xdrgen_encode_int(struct xdr_stream *xdr, s32 val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*p = cpu_to_be32(val);
+	return true;
+}
+
+static inline bool
+xdrgen_encode_unsigned_int(struct xdr_stream *xdr, u32 val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*p = cpu_to_be32(val);
+	return true;
+}
+
+static inline bool
+xdrgen_encode_uint32_t(struct xdr_stream *xdr, u32 val)
+{
+	return xdrgen_encode_unsigned_int(xdr, val);
+}
+
+static inline bool
+xdrgen_encode_long(struct xdr_stream *xdr, s32 val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*p = cpu_to_be32(val);
+	return true;
+}
+
+static inline bool
+xdrgen_encode_unsigned_long(struct xdr_stream *xdr, u32 val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
+
+	if (unlikely(!p))
+		return false;
+	*p = cpu_to_be32(val);
+	return true;
+}
+
+static inline bool
+xdrgen_encode_hyper(struct xdr_stream *xdr, s64 val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT * 2);
+
+	if (unlikely(!p))
+		return false;
+	put_unaligned_be64(val, p);
+	return true;
+}
+
+static inline bool
+xdrgen_encode_int64_t(struct xdr_stream *xdr, s64 val)
+{
+	return xdrgen_encode_hyper(xdr, val);
+}
+
+static inline bool
+xdrgen_encode_unsigned_hyper(struct xdr_stream *xdr, u64 val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT * 2);
+
+	if (unlikely(!p))
+		return false;
+	put_unaligned_be64(val, p);
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_encode_opaque(struct xdr_stream *xdr, opaque val)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT + xdr_align_size(val.len));
+
+	if (unlikely(!p))
+		return false;
+	xdr_encode_opaque(p, val.data, val.len);
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_encode_string(struct xdr_stream *xdr, string val, u32 maxlen)
+{
+	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT + xdr_align_size(val.len));
+
+	if (unlikely(!p))
+		return false;
+	xdr_encode_opaque(p, val.data, val.len);
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_fattr4_offline(struct xdr_stream *xdr, fattr4_offline *ptr)
+{
+	return xdrgen_decode_bool(xdr, ptr);
+};
+
+static bool __maybe_unused
+xdrgen_decode_bitmap4(struct xdr_stream *xdr, bitmap4 *ptr)
+{
+	if (xdr_stream_decode_u32(xdr, &ptr->count) < 0)
+		return false;
+	for (u32 i = 0; i < ptr->count; i++)
+		if (!xdrgen_decode_uint32_t(xdr, &ptr->element[i]))
+			return false;
+	return true;
+};
+
+static bool __maybe_unused
+xdrgen_decode_open_arguments4(struct xdr_stream *xdr, struct open_arguments4 *ptr)
+{
+	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_access))
+		return false;
+	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_deny))
+		return false;
+	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_access_want))
+		return false;
+	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_open_claim))
+		return false;
+	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_create_mode))
+		return false;
+	return true;
+};
+
+static bool __maybe_unused
+xdrgen_decode_open_args_share_access4(struct xdr_stream *xdr, enum open_args_share_access4 *ptr)
+{
+	u32 val;
+
+	if (xdr_stream_decode_u32(xdr, &val) < 0)
+		return false;
+	*ptr = val;
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_open_args_share_deny4(struct xdr_stream *xdr, enum open_args_share_deny4 *ptr)
+{
+	u32 val;
+
+	if (xdr_stream_decode_u32(xdr, &val) < 0)
+		return false;
+	*ptr = val;
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_open_args_share_access_want4(struct xdr_stream *xdr, enum open_args_share_access_want4 *ptr)
+{
+	u32 val;
+
+	if (xdr_stream_decode_u32(xdr, &val) < 0)
+		return false;
+	*ptr = val;
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_open_args_open_claim4(struct xdr_stream *xdr, enum open_args_open_claim4 *ptr)
+{
+	u32 val;
+
+	if (xdr_stream_decode_u32(xdr, &val) < 0)
+		return false;
+	*ptr = val;
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_open_args_createmode4(struct xdr_stream *xdr, enum open_args_createmode4 *ptr)
+{
+	u32 val;
+
+	if (xdr_stream_decode_u32(xdr, &val) < 0)
+		return false;
+	*ptr = val;
+	return true;
+}
+
+static bool __maybe_unused
+xdrgen_decode_fattr4_open_arguments(struct xdr_stream *xdr, fattr4_open_arguments *ptr)
+{
+	return xdrgen_decode_open_arguments4(xdr, ptr);
+};
+
+static bool __maybe_unused
+xdrgen_decode_nfstime4(struct xdr_stream *xdr, struct _nfstime4 *ptr)
+{
+	if (!xdrgen_decode_int64_t(xdr, &ptr->seconds))
+		return false;
+	if (!xdrgen_decode_uint32_t(xdr, &ptr->nseconds))
+		return false;
+	return true;
+};
+
+static bool __maybe_unused
+xdrgen_decode_fattr4_time_deleg_access(struct xdr_stream *xdr, fattr4_time_deleg_access *ptr)
+{
+	return xdrgen_decode_nfstime4(xdr, ptr);
+};
+
+static bool __maybe_unused
+xdrgen_decode_fattr4_time_deleg_modify(struct xdr_stream *xdr, fattr4_time_deleg_modify *ptr)
+{
+	return xdrgen_decode_nfstime4(xdr, ptr);
+};
+
+static bool __maybe_unused
+xdrgen_encode_fattr4_offline(struct xdr_stream *xdr, const fattr4_offline value)
+{
+	return xdrgen_encode_bool(xdr, value);
+};
+
+static bool __maybe_unused
+xdrgen_encode_bitmap4(struct xdr_stream *xdr, const bitmap4 value)
+{
+	if (xdr_stream_encode_u32(xdr, value.count) != XDR_UNIT)
+		return false;
+	for (u32 i = 0; i < value.count; i++)
+		if (!xdrgen_encode_uint32_t(xdr, value.element[i]))
+			return false;
+	return true;
+};
+
+static bool __maybe_unused
+xdrgen_encode_open_arguments4(struct xdr_stream *xdr, const struct open_arguments4 *value)
+{
+	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_access))
+		return false;
+	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_deny))
+		return false;
+	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_access_want))
+		return false;
+	if (!xdrgen_encode_bitmap4(xdr, value->oa_open_claim))
+		return false;
+	if (!xdrgen_encode_bitmap4(xdr, value->oa_create_mode))
+		return false;
+	return true;
+};
+
+static bool __maybe_unused
+xdrgen_encode_open_args_share_access4(struct xdr_stream *xdr, enum open_args_share_access4 value)
+{
+	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
+}
+
+static bool __maybe_unused
+xdrgen_encode_open_args_share_deny4(struct xdr_stream *xdr, enum open_args_share_deny4 value)
+{
+	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
+}
+
+static bool __maybe_unused
+xdrgen_encode_open_args_share_access_want4(struct xdr_stream *xdr, enum open_args_share_access_want4 value)
+{
+	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
+}
+
+static bool __maybe_unused
+xdrgen_encode_open_args_open_claim4(struct xdr_stream *xdr, enum open_args_open_claim4 value)
+{
+	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
+}
+
+static bool __maybe_unused
+xdrgen_encode_open_args_createmode4(struct xdr_stream *xdr, enum open_args_createmode4 value)
+{
+	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
+}
+
+static bool __maybe_unused
+xdrgen_encode_fattr4_open_arguments(struct xdr_stream *xdr, const fattr4_open_arguments *value)
+{
+	return xdrgen_encode_open_arguments4(xdr, value);
+};
+
+static bool __maybe_unused
+xdrgen_encode_nfstime4(struct xdr_stream *xdr, const struct _nfstime4 *value)
+{
+	if (!xdrgen_encode_int64_t(xdr, value->seconds))
+		return false;
+	if (!xdrgen_encode_uint32_t(xdr, value->nseconds))
+		return false;
+	return true;
+};
+
+static bool __maybe_unused
+xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const fattr4_time_deleg_access value)
+{
+	return xdrgen_encode_nfstime4(xdr, &value);
+};
+
+static bool __maybe_unused
+xdrgen_encode_fattr4_time_deleg_modify(struct xdr_stream *xdr, const fattr4_time_deleg_modify value)
+{
+	return xdrgen_encode_nfstime4(xdr, &value);
+};
diff --git a/fs/nfsd/delstid_xdr.h b/fs/nfsd/delstid_xdr.h
new file mode 100644
index 000000000000..3ca8d0cc8569
--- /dev/null
+++ b/fs/nfsd/delstid_xdr.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Generated by lkxdrgen, with hand edits. */
+/* XDR specification modification time: Wed Aug 14 13:35:03 2024 */
+
+#ifndef _DELSTID_H
+#define _DELSTID_H
+
+#include <linux/types.h>
+#include <linux/sunrpc/xdr.h>
+#include <linux/sunrpc/svc.h>
+
+typedef struct {
+	u32 len;
+	unsigned char *data;
+} string;
+
+typedef struct {
+	u32 len;
+	u8 *data;
+} opaque;
+
+typedef struct {
+	u32 count;
+	uint32_t *element;
+} bitmap4;
+
+typedef struct _nfstime4 {
+	int64_t seconds;
+	uint32_t nseconds;
+} nfstime4;
+
+typedef bool fattr4_offline;
+
+#define FATTR4_OFFLINE (83)
+
+typedef struct open_arguments4 {
+	bitmap4 oa_share_access;
+	bitmap4 oa_share_deny;
+	bitmap4 oa_share_access_want;
+	bitmap4 oa_open_claim;
+	bitmap4 oa_create_mode;
+} open_arguments4;
+
+enum open_args_share_access4 {
+	OPEN_ARGS_SHARE_ACCESS_READ = 1,
+	OPEN_ARGS_SHARE_ACCESS_WRITE = 2,
+	OPEN_ARGS_SHARE_ACCESS_BOTH = 3,
+};
+
+enum open_args_share_deny4 {
+	OPEN_ARGS_SHARE_DENY_NONE = 0,
+	OPEN_ARGS_SHARE_DENY_READ = 1,
+	OPEN_ARGS_SHARE_DENY_WRITE = 2,
+	OPEN_ARGS_SHARE_DENY_BOTH = 3,
+};
+
+enum open_args_share_access_want4 {
+	OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG = 3,
+	OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG = 4,
+	OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL = 5,
+	OPEN_ARGS_SHARE_ACCESS_WANT_SIGNAL_DELEG_WHEN_RESRC_AVAIL = 17,
+	OPEN_ARGS_SHARE_ACCESS_WANT_PUSH_DELEG_WHEN_UNCONTENDED = 18,
+	OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 20,
+	OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION = 21,
+};
+
+enum open_args_open_claim4 {
+	OPEN_ARGS_OPEN_CLAIM_NULL = 0,
+	OPEN_ARGS_OPEN_CLAIM_PREVIOUS = 1,
+	OPEN_ARGS_OPEN_CLAIM_DELEGATE_CUR = 2,
+	OPEN_ARGS_OPEN_CLAIM_DELEGATE_PREV = 3,
+	OPEN_ARGS_OPEN_CLAIM_FH = 4,
+	OPEN_ARGS_OPEN_CLAIM_DELEG_CUR_FH = 5,
+	OPEN_ARGS_OPEN_CLAIM_DELEG_PREV_FH = 6,
+};
+
+enum open_args_createmode4 {
+	OPEN_ARGS_CREATEMODE_UNCHECKED4 = 0,
+	OPEN_ARGS_CREATE_MODE_GUARDED = 1,
+	OPEN_ARGS_CREATEMODE_EXCLUSIVE4 = 2,
+	OPEN_ARGS_CREATE_MODE_EXCLUSIVE4_1 = 3,
+};
+
+typedef open_arguments4 fattr4_open_arguments;
+
+#define FATTR4_OPEN_ARGUMENTS (86)
+
+#define OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION (0x200000)
+
+#define OPEN4_RESULT_NO_OPEN_STATEID (0x00000010)
+
+typedef nfstime4 fattr4_time_deleg_access;
+
+typedef nfstime4 fattr4_time_deleg_modify;
+
+#define FATTR4_TIME_DELEG_ACCESS (84)
+
+#define FATTR4_TIME_DELEG_MODIFY (85)
+
+#define OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS (0x100000)
+
+#endif /* _DELSTID_H */
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 643ca3f8ebb3..b3d2000c8a08 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -55,6 +55,7 @@
 #include "netns.h"
 #include "pnfs.h"
 #include "filecache.h"
+#include "delstid_xdr.h"
 
 #include "trace.h"
 

-- 
2.46.0

[PATCH 2/3] nfsd: add support for FATTR4_OPEN_ARGUMENTS

[Jeff Layton]

Add support for FATTR4_OPEN_ARGUMENTS. This a new mechanism for the
client to discover what OPEN features the server supports.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/delstid_xdr.c |  2 +-
 fs/nfsd/delstid_xdr.h |  3 +++
 fs/nfsd/nfs4xdr.c     | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsd.h        |  3 ++-
 4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/delstid_xdr.c b/fs/nfsd/delstid_xdr.c
index 63494d14f5d2..b63f338da357 100644
--- a/fs/nfsd/delstid_xdr.c
+++ b/fs/nfsd/delstid_xdr.c
@@ -435,7 +435,7 @@ xdrgen_encode_open_args_createmode4(struct xdr_stream *xdr, enum open_args_creat
 	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
 }
 
-static bool __maybe_unused
+bool
 xdrgen_encode_fattr4_open_arguments(struct xdr_stream *xdr, const fattr4_open_arguments *value)
 {
 	return xdrgen_encode_open_arguments4(xdr, value);
diff --git a/fs/nfsd/delstid_xdr.h b/fs/nfsd/delstid_xdr.h
index 3ca8d0cc8569..2a91a353ab93 100644
--- a/fs/nfsd/delstid_xdr.h
+++ b/fs/nfsd/delstid_xdr.h
@@ -99,4 +99,7 @@ typedef nfstime4 fattr4_time_deleg_modify;
 
 #define OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS (0x100000)
 
+/* delstid.c */
+bool xdrgen_encode_fattr4_open_arguments(struct xdr_stream *xdr,
+					 const fattr4_open_arguments *value);
 #endif /* _DELSTID_H */
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b3d2000c8a08..dbaadb0ad980 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3400,6 +3400,58 @@ static __be32 nfsd4_encode_fattr4_xattr_support(struct xdr_stream *xdr,
 	return nfsd4_encode_bool(xdr, err == 0);
 }
 
+#define NFSD_OA_SHARE_ACCESS	(BIT(OPEN_ARGS_SHARE_ACCESS_READ)	| \
+				 BIT(OPEN_ARGS_SHARE_ACCESS_WRITE)	| \
+				 BIT(OPEN_ARGS_SHARE_ACCESS_BOTH))
+
+#define NFSD_OA_SHARE_DENY	(BIT(OPEN_ARGS_SHARE_DENY_NONE)		| \
+				 BIT(OPEN_ARGS_SHARE_DENY_READ)		| \
+				 BIT(OPEN_ARGS_SHARE_DENY_WRITE)	| \
+				 BIT(OPEN_ARGS_SHARE_DENY_BOTH))
+
+#define NFSD_OA_SHARE_ACCESS_WANT	(BIT(OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG)		| \
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG)		| \
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL))
+
+#define NFSD_OA_OPEN_CLAIM	(BIT(OPEN_ARGS_OPEN_CLAIM_NULL)		| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_PREVIOUS)	| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEGATE_CUR)	| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEGATE_PREV)| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_FH)		| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEG_CUR_FH)	| \
+				 BIT(OPEN_ARGS_OPEN_CLAIM_DELEG_PREV_FH))
+
+#define NFSD_OA_CREATE_MODE	(BIT(OPEN_ARGS_CREATEMODE_UNCHECKED4)	| \
+				 BIT(OPEN_ARGS_CREATE_MODE_GUARDED)	| \
+				 BIT(OPEN_ARGS_CREATEMODE_EXCLUSIVE4)	| \
+				 BIT(OPEN_ARGS_CREATE_MODE_EXCLUSIVE4_1))
+
+static __be32 nfsd4_encode_fattr4_open_arguments(struct xdr_stream *xdr,
+						 const struct nfsd4_fattr_args *args)
+{
+	uint32_t oa_share_access = NFSD_OA_SHARE_ACCESS;
+	uint32_t oa_share_deny = NFSD_OA_SHARE_DENY;
+	uint32_t oa_share_access_want = NFSD_OA_SHARE_ACCESS_WANT;
+	uint32_t oa_open_claim = NFSD_OA_OPEN_CLAIM;
+	uint32_t oa_create_mode = NFSD_OA_CREATE_MODE;
+	struct open_arguments4 oa;
+
+	oa.oa_share_access.count = 1;
+	oa.oa_share_access.element = &oa_share_access;
+	oa.oa_share_deny.count = 1;
+	oa.oa_share_deny.element = &oa_share_deny;
+	oa.oa_share_access_want.count = 1;
+	oa.oa_share_access_want.element = &oa_share_access_want;
+	oa.oa_open_claim.count = 1;
+	oa.oa_open_claim.element = &oa_open_claim;
+	oa.oa_create_mode.count = 1;
+	oa.oa_create_mode.element = &oa_create_mode;
+
+	if (!xdrgen_encode_fattr4_open_arguments(xdr, &oa))
+		return nfserr_resource;
+	return nfs_ok;
+}
+
 static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
 	[FATTR4_SUPPORTED_ATTRS]	= nfsd4_encode_fattr4_supported_attrs,
 	[FATTR4_TYPE]			= nfsd4_encode_fattr4_type,
@@ -3500,6 +3552,7 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
 
 	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
 	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
+	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
 };
 
 /*
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 4ccbf014a2c7..c98fb104ba7d 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -454,7 +454,8 @@ enum {
 	(NFSD4_1_SUPPORTED_ATTRS_WORD2 | \
 	FATTR4_WORD2_MODE_UMASK | \
 	NFSD4_2_SECURITY_ATTRS | \
-	FATTR4_WORD2_XATTR_SUPPORT)
+	FATTR4_WORD2_XATTR_SUPPORT | \
+	FATTR4_WORD2_OPEN_ARGUMENTS)
 
 extern const u32 nfsd_suppattrs[3][3];
 

-- 
2.46.0

[PATCH 3/3] nfsd: implement OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION

[Jeff Layton]

Allow clients to request getting a delegation xor an open stateid if a
delegation isn't available. This allows the client to avoid sending a
final CLOSE for the (useless) open stateid, when it is granted a
delegation.

This is done by moving the increment of the open stateid and unlocking
of the st_mutex until after we acquire a delegation. If we get a
delegation, we zero out the op_stateid field and set the NO_OPEN_STATEID
flag. If the open stateid was brand new, then unhash it too in this case
since it won't be needed.

If we can't get a delegation or the new flag wasn't requested, then just
increment and copy the open stateid as usual.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c       | 29 +++++++++++++++++++++++++----
 fs/nfsd/nfs4xdr.c         |  5 +++--
 include/uapi/linux/nfs4.h |  7 +++++--
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b67f151837c1..9d209cbd95cd 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -56,6 +56,7 @@
 #include "pnfs.h"
 #include "filecache.h"
 #include "trace.h"
+#include "delstid_xdr.h"
 
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
@@ -6029,6 +6030,17 @@ static void nfsd4_deleg_xgrade_none_ext(struct nfsd4_open *open,
 	 */
 }
 
+/* Are we only returning a delegation stateid? */
+static bool open_xor_delegation(struct nfsd4_open *open)
+{
+	if (!(open->op_deleg_want & OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION))
+		return false;
+	if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ &&
+	    open->op_delegate_type != NFS4_OPEN_DELEGATE_WRITE)
+		return false;
+	return true;
+}
+
 /**
  * nfsd4_process_open2 - finish open processing
  * @rqstp: the RPC transaction being executed
@@ -6051,6 +6063,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	struct nfs4_delegation *dp = NULL;
 	__be32 status;
 	bool new_stp = false;
+	bool deleg_only = false;
 
 	/*
 	 * Lookup file; if found, lookup stateid and check open request,
@@ -6105,9 +6118,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 			open->op_odstate = NULL;
 	}
 
-	nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
-	mutex_unlock(&stp->st_mutex);
-
 	if (nfsd4_has_session(&resp->cstate)) {
 		if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
 			open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE_EXT;
@@ -6121,7 +6131,18 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	* OPEN succeeds even if we fail.
 	*/
 	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
+	deleg_only = open_xor_delegation(open);
 nodeleg:
+	if (deleg_only) {
+		memcpy(&open->op_stateid, &zero_stateid, sizeof(open->op_stateid));
+		open->op_rflags |= OPEN4_RESULT_NO_OPEN_STATEID;
+		if (new_stp)
+			release_open_stateid(stp);
+	} else {
+		nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
+	}
+	mutex_unlock(&stp->st_mutex);
+
 	status = nfs_ok;
 	trace_nfsd_open(&stp->st_stid.sc_stateid);
 out:
@@ -6137,7 +6158,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	/*
 	* To finish the open response, we just need to set the rflags.
 	*/
-	open->op_rflags = NFS4_OPEN_RESULT_LOCKTYPE_POSIX;
+	open->op_rflags |= NFS4_OPEN_RESULT_LOCKTYPE_POSIX;
 	if (nfsd4_has_session(&resp->cstate))
 		open->op_rflags |= NFS4_OPEN_RESULT_MAY_NOTIFY_LOCK;
 	else if (!(open->op_openowner->oo_flags & NFS4_OO_CONFIRMED))
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index dbaadb0ad980..ecc9ddf4c28d 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1067,7 +1067,7 @@ static __be32 nfsd4_decode_share_access(struct nfsd4_compoundargs *argp, u32 *sh
 		return nfs_ok;
 	if (!argp->minorversion)
 		return nfserr_bad_xdr;
-	switch (w & NFS4_SHARE_WANT_MASK) {
+	switch (w & NFS4_SHARE_WANT_TYPE_MASK) {
 	case NFS4_SHARE_WANT_NO_PREFERENCE:
 	case NFS4_SHARE_WANT_READ_DELEG:
 	case NFS4_SHARE_WANT_WRITE_DELEG:
@@ -3411,7 +3411,8 @@ static __be32 nfsd4_encode_fattr4_xattr_support(struct xdr_stream *xdr,
 
 #define NFSD_OA_SHARE_ACCESS_WANT	(BIT(OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG)		| \
 					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG)		| \
-					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL))
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL)		| \
+					 BIT(OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION))
 
 #define NFSD_OA_OPEN_CLAIM	(BIT(OPEN_ARGS_OPEN_CLAIM_NULL)		| \
 				 BIT(OPEN_ARGS_OPEN_CLAIM_PREVIOUS)	| \
diff --git a/include/uapi/linux/nfs4.h b/include/uapi/linux/nfs4.h
index caf4db2fcbb9..4273e0249fcb 100644
--- a/include/uapi/linux/nfs4.h
+++ b/include/uapi/linux/nfs4.h
@@ -58,7 +58,7 @@
 #define NFS4_SHARE_DENY_BOTH	0x0003
 
 /* nfs41 */
-#define NFS4_SHARE_WANT_MASK		0xFF00
+#define NFS4_SHARE_WANT_TYPE_MASK	0xFF00
 #define NFS4_SHARE_WANT_NO_PREFERENCE	0x0000
 #define NFS4_SHARE_WANT_READ_DELEG	0x0100
 #define NFS4_SHARE_WANT_WRITE_DELEG	0x0200
@@ -66,13 +66,16 @@
 #define NFS4_SHARE_WANT_NO_DELEG	0x0400
 #define NFS4_SHARE_WANT_CANCEL		0x0500
 
-#define NFS4_SHARE_WHEN_MASK		0xF0000
+#define NFS4_SHARE_WHEN_MASK				0xF0000
 #define NFS4_SHARE_SIGNAL_DELEG_WHEN_RESRC_AVAIL	0x10000
 #define NFS4_SHARE_PUSH_DELEG_WHEN_UNCONTENDED		0x20000
 
+#define NFS4_SHARE_WANT_MOD_MASK			0xF00000
 #define NFS4_SHARE_WANT_DELEG_TIMESTAMPS		0x100000
 #define NFS4_SHARE_WANT_OPEN_XOR_DELEGATION		0x200000
 
+#define NFS4_SHARE_WANT_MASK	(NFS4_SHARE_WANT_TYPE_MASK | NFS4_SHARE_WANT_MOD_MASK)
+
 #define NFS4_CDFC4_FORE	0x1
 #define NFS4_CDFC4_BACK 0x2
 #define NFS4_CDFC4_BOTH 0x3

-- 
2.46.0

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Chuck Lever]

On Fri, Aug 16, 2024 at 08:42:07AM -0400, Jeff Layton wrote:
> This adds support for the "delstid" draft:
> 
>     https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/
> 
> Most of this was autogenerated using Chuck's lkxdrgen tool with some
> by-hand tweaks to work around some symbol conflicts, and to add some
> missing pieces that were needed from nfs4_1.x.

I haven't read delstid closely enough to comment on the approach
you've taken in 3/3, but I do have some thoughts about code
organization here. I will try to study that draft soon.

And, I'm assuming you are continuing to evolve support for the draft
and will be growing this series. So I will hold off on careful
inspection of 3/3 for the moment.

First, I'm pleased that you found xdrgen useful for rapid
prototyping. That's not something I had envisioned when I created
the tool, but it's a good match, looks like.

Here you add a separate set of source files for delstid XDR...  That
approach might not be scalable for adding subsequent new features in
general, it occurs to me.

We might end up with a bunch of these little code blurbs with no
clear understanding of how they inter-relate.  Thoughts about how to
manage these are welcome: xdrgen could generate only header files
and then we would #include them where needed, for example.

For now, I suggest folding the new generated XDR code into the
existing NFSv4 "open" XDR code in fs/nfsd/nfs4xdr.c, when you have
some time for cleaning up the patches. An alternative would be to
leave it and I can fold these together before committing.

(The long term, of course, will hopefully be generating all XDR code
automatically, but we're a ways out from that, IMO).

The generator adds __maybe_unused to some of these functions to
avoid having to reason about which encoders/decoders are not needed.
It assumes the C compiler will simply not generate machine code for
unused functions.

But that clutters the source code if you plan to mix it with hand-
written code. You might remove that decorator to identify the
functions that are actually not used by your implementation.

----

On an unrelated note, do you know of a plan to add delstid-related
unit tests to pynfs ?


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/Makefile      |   2 +-
>  fs/nfsd/delstid_xdr.c | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/nfsd/delstid_xdr.h | 102 +++++++++++
>  fs/nfsd/nfs4xdr.c     |   1 +
>  4 files changed, 568 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
> index b8736a82e57c..187fa45640e6 100644
> --- a/fs/nfsd/Makefile
> +++ b/fs/nfsd/Makefile
> @@ -18,7 +18,7 @@ nfsd-$(CONFIG_NFSD_V2) += nfsproc.o nfsxdr.o
>  nfsd-$(CONFIG_NFSD_V2_ACL) += nfs2acl.o
>  nfsd-$(CONFIG_NFSD_V3_ACL) += nfs3acl.o
>  nfsd-$(CONFIG_NFSD_V4)	+= nfs4proc.o nfs4xdr.o nfs4state.o nfs4idmap.o \
> -			   nfs4acl.o nfs4callback.o nfs4recover.o
> +			   nfs4acl.o nfs4callback.o nfs4recover.o delstid_xdr.o
>  nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
>  nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
>  nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
> diff --git a/fs/nfsd/delstid_xdr.c b/fs/nfsd/delstid_xdr.c
> new file mode 100644
> index 000000000000..63494d14f5d2
> --- /dev/null
> +++ b/fs/nfsd/delstid_xdr.c
> @@ -0,0 +1,464 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Generated by lkxdrgen, with hand-edits.
> +// XDR specification modification time: Wed Aug 14 13:35:03 2024
> +
> +#include "delstid_xdr.h"
> +
> +static inline bool
> +xdrgen_decode_void(struct xdr_stream *xdr)
> +{
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_decode_bool(struct xdr_stream *xdr, bool *ptr)
> +{
> +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*ptr = (*p != xdr_zero);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_decode_int(struct xdr_stream *xdr, s32 *ptr)
> +{
> +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*ptr = be32_to_cpup(p);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_decode_unsigned_int(struct xdr_stream *xdr, u32 *ptr)
> +{
> +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*ptr = be32_to_cpup(p);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_decode_uint32_t(struct xdr_stream *xdr, u32 *ptr)
> +{
> +	return xdrgen_decode_unsigned_int(xdr, ptr);
> +}
> +
> +static inline bool
> +xdrgen_decode_long(struct xdr_stream *xdr, s32 *ptr)
> +{
> +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*ptr = be32_to_cpup(p);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_decode_unsigned_long(struct xdr_stream *xdr, u32 *ptr)
> +{
> +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*ptr = be32_to_cpup(p);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_decode_hyper(struct xdr_stream *xdr, s64 *ptr)
> +{
> +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT * 2);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*ptr = get_unaligned_be64(p);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_decode_int64_t(struct xdr_stream *xdr, s64 *ptr)
> +{
> +	return xdrgen_decode_hyper(xdr, ptr);
> +}
> +
> +static inline bool
> +xdrgen_decode_unsigned_hyper(struct xdr_stream *xdr, u64 *ptr)
> +{
> +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT * 2);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*ptr = get_unaligned_be64(p);
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_opaque(struct xdr_stream *xdr, opaque *ptr, u32 maxlen)
> +{
> +	__be32 *p;
> +	u32 len;
> +
> +	if (unlikely(xdr_stream_decode_u32(xdr, &len) != XDR_UNIT))
> +		return false;
> +	if (unlikely(maxlen && len > maxlen))
> +		return false;
> +	if (len != 0) {
> +		p = xdr_inline_decode(xdr, len);
> +		if (unlikely(!p))
> +			return false;
> +		ptr->data = (u8 *)p;
> +	}
> +	ptr->len = len;
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_string(struct xdr_stream *xdr, string *ptr, u32 maxlen)
> +{
> +	__be32 *p;
> +	u32 len;
> +
> +	if (unlikely(xdr_stream_decode_u32(xdr, &len) != XDR_UNIT))
> +		return false;
> +	if (unlikely(maxlen && len > maxlen))
> +		return false;
> +	if (len != 0) {
> +		p = xdr_inline_decode(xdr, len);
> +		if (unlikely(!p))
> +			return false;
> +		ptr->data = (unsigned char *)p;
> +	}
> +	ptr->len = len;
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_void(struct xdr_stream *xdr)
> +{
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_bool(struct xdr_stream *xdr, bool val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*p = val ? xdr_one : xdr_zero;
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_int(struct xdr_stream *xdr, s32 val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*p = cpu_to_be32(val);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_unsigned_int(struct xdr_stream *xdr, u32 val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*p = cpu_to_be32(val);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_uint32_t(struct xdr_stream *xdr, u32 val)
> +{
> +	return xdrgen_encode_unsigned_int(xdr, val);
> +}
> +
> +static inline bool
> +xdrgen_encode_long(struct xdr_stream *xdr, s32 val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*p = cpu_to_be32(val);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_unsigned_long(struct xdr_stream *xdr, u32 val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> +
> +	if (unlikely(!p))
> +		return false;
> +	*p = cpu_to_be32(val);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_hyper(struct xdr_stream *xdr, s64 val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT * 2);
> +
> +	if (unlikely(!p))
> +		return false;
> +	put_unaligned_be64(val, p);
> +	return true;
> +}
> +
> +static inline bool
> +xdrgen_encode_int64_t(struct xdr_stream *xdr, s64 val)
> +{
> +	return xdrgen_encode_hyper(xdr, val);
> +}
> +
> +static inline bool
> +xdrgen_encode_unsigned_hyper(struct xdr_stream *xdr, u64 val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT * 2);
> +
> +	if (unlikely(!p))
> +		return false;
> +	put_unaligned_be64(val, p);
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_encode_opaque(struct xdr_stream *xdr, opaque val)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT + xdr_align_size(val.len));
> +
> +	if (unlikely(!p))
> +		return false;
> +	xdr_encode_opaque(p, val.data, val.len);
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_encode_string(struct xdr_stream *xdr, string val, u32 maxlen)
> +{
> +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT + xdr_align_size(val.len));
> +
> +	if (unlikely(!p))
> +		return false;
> +	xdr_encode_opaque(p, val.data, val.len);
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_fattr4_offline(struct xdr_stream *xdr, fattr4_offline *ptr)
> +{
> +	return xdrgen_decode_bool(xdr, ptr);
> +};
> +
> +static bool __maybe_unused
> +xdrgen_decode_bitmap4(struct xdr_stream *xdr, bitmap4 *ptr)
> +{
> +	if (xdr_stream_decode_u32(xdr, &ptr->count) < 0)
> +		return false;
> +	for (u32 i = 0; i < ptr->count; i++)
> +		if (!xdrgen_decode_uint32_t(xdr, &ptr->element[i]))
> +			return false;
> +	return true;
> +};
> +
> +static bool __maybe_unused
> +xdrgen_decode_open_arguments4(struct xdr_stream *xdr, struct open_arguments4 *ptr)
> +{
> +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_access))
> +		return false;
> +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_deny))
> +		return false;
> +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_access_want))
> +		return false;
> +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_open_claim))
> +		return false;
> +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_create_mode))
> +		return false;
> +	return true;
> +};
> +
> +static bool __maybe_unused
> +xdrgen_decode_open_args_share_access4(struct xdr_stream *xdr, enum open_args_share_access4 *ptr)
> +{
> +	u32 val;
> +
> +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> +		return false;
> +	*ptr = val;
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_open_args_share_deny4(struct xdr_stream *xdr, enum open_args_share_deny4 *ptr)
> +{
> +	u32 val;
> +
> +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> +		return false;
> +	*ptr = val;
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_open_args_share_access_want4(struct xdr_stream *xdr, enum open_args_share_access_want4 *ptr)
> +{
> +	u32 val;
> +
> +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> +		return false;
> +	*ptr = val;
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_open_args_open_claim4(struct xdr_stream *xdr, enum open_args_open_claim4 *ptr)
> +{
> +	u32 val;
> +
> +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> +		return false;
> +	*ptr = val;
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_open_args_createmode4(struct xdr_stream *xdr, enum open_args_createmode4 *ptr)
> +{
> +	u32 val;
> +
> +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> +		return false;
> +	*ptr = val;
> +	return true;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_decode_fattr4_open_arguments(struct xdr_stream *xdr, fattr4_open_arguments *ptr)
> +{
> +	return xdrgen_decode_open_arguments4(xdr, ptr);
> +};
> +
> +static bool __maybe_unused
> +xdrgen_decode_nfstime4(struct xdr_stream *xdr, struct _nfstime4 *ptr)
> +{
> +	if (!xdrgen_decode_int64_t(xdr, &ptr->seconds))
> +		return false;
> +	if (!xdrgen_decode_uint32_t(xdr, &ptr->nseconds))
> +		return false;
> +	return true;
> +};
> +
> +static bool __maybe_unused
> +xdrgen_decode_fattr4_time_deleg_access(struct xdr_stream *xdr, fattr4_time_deleg_access *ptr)
> +{
> +	return xdrgen_decode_nfstime4(xdr, ptr);
> +};
> +
> +static bool __maybe_unused
> +xdrgen_decode_fattr4_time_deleg_modify(struct xdr_stream *xdr, fattr4_time_deleg_modify *ptr)
> +{
> +	return xdrgen_decode_nfstime4(xdr, ptr);
> +};
> +
> +static bool __maybe_unused
> +xdrgen_encode_fattr4_offline(struct xdr_stream *xdr, const fattr4_offline value)
> +{
> +	return xdrgen_encode_bool(xdr, value);
> +};
> +
> +static bool __maybe_unused
> +xdrgen_encode_bitmap4(struct xdr_stream *xdr, const bitmap4 value)
> +{
> +	if (xdr_stream_encode_u32(xdr, value.count) != XDR_UNIT)
> +		return false;
> +	for (u32 i = 0; i < value.count; i++)
> +		if (!xdrgen_encode_uint32_t(xdr, value.element[i]))
> +			return false;
> +	return true;
> +};
> +
> +static bool __maybe_unused
> +xdrgen_encode_open_arguments4(struct xdr_stream *xdr, const struct open_arguments4 *value)
> +{
> +	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_access))
> +		return false;
> +	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_deny))
> +		return false;
> +	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_access_want))
> +		return false;
> +	if (!xdrgen_encode_bitmap4(xdr, value->oa_open_claim))
> +		return false;
> +	if (!xdrgen_encode_bitmap4(xdr, value->oa_create_mode))
> +		return false;
> +	return true;
> +};
> +
> +static bool __maybe_unused
> +xdrgen_encode_open_args_share_access4(struct xdr_stream *xdr, enum open_args_share_access4 value)
> +{
> +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_encode_open_args_share_deny4(struct xdr_stream *xdr, enum open_args_share_deny4 value)
> +{
> +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_encode_open_args_share_access_want4(struct xdr_stream *xdr, enum open_args_share_access_want4 value)
> +{
> +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_encode_open_args_open_claim4(struct xdr_stream *xdr, enum open_args_open_claim4 value)
> +{
> +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_encode_open_args_createmode4(struct xdr_stream *xdr, enum open_args_createmode4 value)
> +{
> +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> +}
> +
> +static bool __maybe_unused
> +xdrgen_encode_fattr4_open_arguments(struct xdr_stream *xdr, const fattr4_open_arguments *value)
> +{
> +	return xdrgen_encode_open_arguments4(xdr, value);
> +};
> +
> +static bool __maybe_unused
> +xdrgen_encode_nfstime4(struct xdr_stream *xdr, const struct _nfstime4 *value)
> +{
> +	if (!xdrgen_encode_int64_t(xdr, value->seconds))
> +		return false;
> +	if (!xdrgen_encode_uint32_t(xdr, value->nseconds))
> +		return false;
> +	return true;
> +};
> +
> +static bool __maybe_unused
> +xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const fattr4_time_deleg_access value)
> +{
> +	return xdrgen_encode_nfstime4(xdr, &value);
> +};
> +
> +static bool __maybe_unused
> +xdrgen_encode_fattr4_time_deleg_modify(struct xdr_stream *xdr, const fattr4_time_deleg_modify value)
> +{
> +	return xdrgen_encode_nfstime4(xdr, &value);
> +};
> diff --git a/fs/nfsd/delstid_xdr.h b/fs/nfsd/delstid_xdr.h
> new file mode 100644
> index 000000000000..3ca8d0cc8569
> --- /dev/null
> +++ b/fs/nfsd/delstid_xdr.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Generated by lkxdrgen, with hand edits. */
> +/* XDR specification modification time: Wed Aug 14 13:35:03 2024 */
> +
> +#ifndef _DELSTID_H
> +#define _DELSTID_H
> +
> +#include <linux/types.h>
> +#include <linux/sunrpc/xdr.h>
> +#include <linux/sunrpc/svc.h>
> +
> +typedef struct {
> +	u32 len;
> +	unsigned char *data;
> +} string;
> +
> +typedef struct {
> +	u32 len;
> +	u8 *data;
> +} opaque;
> +
> +typedef struct {
> +	u32 count;
> +	uint32_t *element;
> +} bitmap4;
> +
> +typedef struct _nfstime4 {
> +	int64_t seconds;
> +	uint32_t nseconds;
> +} nfstime4;
> +
> +typedef bool fattr4_offline;
> +
> +#define FATTR4_OFFLINE (83)
> +
> +typedef struct open_arguments4 {
> +	bitmap4 oa_share_access;
> +	bitmap4 oa_share_deny;
> +	bitmap4 oa_share_access_want;
> +	bitmap4 oa_open_claim;
> +	bitmap4 oa_create_mode;
> +} open_arguments4;
> +
> +enum open_args_share_access4 {
> +	OPEN_ARGS_SHARE_ACCESS_READ = 1,
> +	OPEN_ARGS_SHARE_ACCESS_WRITE = 2,
> +	OPEN_ARGS_SHARE_ACCESS_BOTH = 3,
> +};
> +
> +enum open_args_share_deny4 {
> +	OPEN_ARGS_SHARE_DENY_NONE = 0,
> +	OPEN_ARGS_SHARE_DENY_READ = 1,
> +	OPEN_ARGS_SHARE_DENY_WRITE = 2,
> +	OPEN_ARGS_SHARE_DENY_BOTH = 3,
> +};
> +
> +enum open_args_share_access_want4 {
> +	OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG = 3,
> +	OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG = 4,
> +	OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL = 5,
> +	OPEN_ARGS_SHARE_ACCESS_WANT_SIGNAL_DELEG_WHEN_RESRC_AVAIL = 17,
> +	OPEN_ARGS_SHARE_ACCESS_WANT_PUSH_DELEG_WHEN_UNCONTENDED = 18,
> +	OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 20,
> +	OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION = 21,
> +};
> +
> +enum open_args_open_claim4 {
> +	OPEN_ARGS_OPEN_CLAIM_NULL = 0,
> +	OPEN_ARGS_OPEN_CLAIM_PREVIOUS = 1,
> +	OPEN_ARGS_OPEN_CLAIM_DELEGATE_CUR = 2,
> +	OPEN_ARGS_OPEN_CLAIM_DELEGATE_PREV = 3,
> +	OPEN_ARGS_OPEN_CLAIM_FH = 4,
> +	OPEN_ARGS_OPEN_CLAIM_DELEG_CUR_FH = 5,
> +	OPEN_ARGS_OPEN_CLAIM_DELEG_PREV_FH = 6,
> +};
> +
> +enum open_args_createmode4 {
> +	OPEN_ARGS_CREATEMODE_UNCHECKED4 = 0,
> +	OPEN_ARGS_CREATE_MODE_GUARDED = 1,
> +	OPEN_ARGS_CREATEMODE_EXCLUSIVE4 = 2,
> +	OPEN_ARGS_CREATE_MODE_EXCLUSIVE4_1 = 3,
> +};
> +
> +typedef open_arguments4 fattr4_open_arguments;
> +
> +#define FATTR4_OPEN_ARGUMENTS (86)
> +
> +#define OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION (0x200000)
> +
> +#define OPEN4_RESULT_NO_OPEN_STATEID (0x00000010)
> +
> +typedef nfstime4 fattr4_time_deleg_access;
> +
> +typedef nfstime4 fattr4_time_deleg_modify;
> +
> +#define FATTR4_TIME_DELEG_ACCESS (84)
> +
> +#define FATTR4_TIME_DELEG_MODIFY (85)
> +
> +#define OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS (0x100000)
> +
> +#endif /* _DELSTID_H */
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 643ca3f8ebb3..b3d2000c8a08 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -55,6 +55,7 @@
>  #include "netns.h"
>  #include "pnfs.h"
>  #include "filecache.h"
> +#include "delstid_xdr.h"
>  
>  #include "trace.h"
>  
> 
> -- 
> 2.46.0
> 

-- 
Chuck Lever

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton]

On Fri, 2024-08-16 at 11:17 -0400, Chuck Lever wrote:
> On Fri, Aug 16, 2024 at 08:42:07AM -0400, Jeff Layton wrote:
> > This adds support for the "delstid" draft:
> > 
> >     https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/
> > 
> > Most of this was autogenerated using Chuck's lkxdrgen tool with
> > some
> > by-hand tweaks to work around some symbol conflicts, and to add
> > some
> > missing pieces that were needed from nfs4_1.x.
> 
> I haven't read delstid closely enough to comment on the approach
> you've taken in 3/3, but I do have some thoughts about code
> organization here. I will try to study that draft soon.
> 
> And, I'm assuming you are continuing to evolve support for the draft
> and will be growing this series. So I will hold off on careful
> inspection of 3/3 for the moment.
> 

Yes. The client pieces are already in place, and I read I get is that
the draft is all but done at this point. 3/3 is probably pretty close
to what should go in. There are really 3 parts to the delstid draft:

1/ the OPEN_XOR_DELEGATION part, which allows the server to avoid
giving out an open stateid when giving out a deleg.

2/ delegated timestamp support (which is the part I'm still looking at)

3/ FATTR4_OPEN_ARGUMENTS (which enables 1 and 2)

This patchset encompasses 1 & 3. Part 2 shouldn't have much bearing on
this set. It's really a separate feature entirely that just happens to
also depend on FATTR4_OPEN_ARGUMENTS support.

> First, I'm pleased that you found xdrgen useful for rapid
> prototyping. That's not something I had envisioned when I created
> the tool, but it's a good match, looks like.
> 

Yeah. It has some bugs that still need fixing, but it was certainly
better than having to roll all of that by hand.

> Here you add a separate set of source files for delstid XDR...  That
> approach might not be scalable for adding subsequent new features in
> general, it occurs to me.
>
> We might end up with a bunch of these little code blurbs with no
> clear understanding of how they inter-relate.  Thoughts about how to
> manage these are welcome: xdrgen could generate only header files
> and then we would #include them where needed, for example.
>
> For now, I suggest folding the new generated XDR code into the
> existing NFSv4 "open" XDR code in fs/nfsd/nfs4xdr.c, when you have
> some time for cleaning up the patches. An alternative would be to
> leave it and I can fold these together before committing.
> 
> (The long term, of course, will hopefully be generating all XDR code
> automatically, but we're a ways out from that, IMO).
> 

This is where I disagree.

The nice thing about lkxdrgen is that most of what it generates is
fairly self-contained. I think we ought to try to keep the new (mostly
autogenerated) and old code (hand-rolled) separate for now.

I'm not sure what makes the most sense for a new naming scheme, but I
really don't think we should paste all of this into nfs4xdr.c, which is
just a huge jumble of hand-rolled XDR code. 

> The generator adds __maybe_unused to some of these functions to
> avoid having to reason about which encoders/decoders are not needed.
> It assumes the C compiler will simply not generate machine code for
> unused functions.
> 
> But that clutters the source code if you plan to mix it with hand-
> written code. You might remove that decorator to identify the
> functions that are actually not used by your implementation.
> 

Yeah, I was planning to remove all of the __maybe_unused designations
once I had timestamp delegation support in place. Some of them can
certainly go away in advance of that though.

> ----
> 
> On an unrelated note, do you know of a plan to add delstid-related
> unit tests to pynfs ?
> 

No. That would be nice to have, but I'm not aware of anyone working on
it.

> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/Makefile      |   2 +-
> >  fs/nfsd/delstid_xdr.c | 464
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/nfsd/delstid_xdr.h | 102 +++++++++++
> >  fs/nfsd/nfs4xdr.c     |   1 +
> >  4 files changed, 568 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
> > index b8736a82e57c..187fa45640e6 100644
> > --- a/fs/nfsd/Makefile
> > +++ b/fs/nfsd/Makefile
> > @@ -18,7 +18,7 @@ nfsd-$(CONFIG_NFSD_V2) += nfsproc.o nfsxdr.o
> >  nfsd-$(CONFIG_NFSD_V2_ACL) += nfs2acl.o
> >  nfsd-$(CONFIG_NFSD_V3_ACL) += nfs3acl.o
> >  nfsd-$(CONFIG_NFSD_V4)	+= nfs4proc.o nfs4xdr.o nfs4state.o
> > nfs4idmap.o \
> > -			   nfs4acl.o nfs4callback.o nfs4recover.o
> > +			   nfs4acl.o nfs4callback.o nfs4recover.o
> > delstid_xdr.o
> >  nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
> >  nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
> >  nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
> > diff --git a/fs/nfsd/delstid_xdr.c b/fs/nfsd/delstid_xdr.c
> > new file mode 100644
> > index 000000000000..63494d14f5d2
> > --- /dev/null
> > +++ b/fs/nfsd/delstid_xdr.c
> > @@ -0,0 +1,464 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Generated by lkxdrgen, with hand-edits.
> > +// XDR specification modification time: Wed Aug 14 13:35:03 2024
> > +
> > +#include "delstid_xdr.h"
> > +
> > +static inline bool
> > +xdrgen_decode_void(struct xdr_stream *xdr)
> > +{
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_bool(struct xdr_stream *xdr, bool *ptr)
> > +{
> > +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*ptr = (*p != xdr_zero);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_int(struct xdr_stream *xdr, s32 *ptr)
> > +{
> > +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*ptr = be32_to_cpup(p);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_unsigned_int(struct xdr_stream *xdr, u32 *ptr)
> > +{
> > +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*ptr = be32_to_cpup(p);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_uint32_t(struct xdr_stream *xdr, u32 *ptr)
> > +{
> > +	return xdrgen_decode_unsigned_int(xdr, ptr);
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_long(struct xdr_stream *xdr, s32 *ptr)
> > +{
> > +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*ptr = be32_to_cpup(p);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_unsigned_long(struct xdr_stream *xdr, u32 *ptr)
> > +{
> > +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*ptr = be32_to_cpup(p);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_hyper(struct xdr_stream *xdr, s64 *ptr)
> > +{
> > +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT * 2);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*ptr = get_unaligned_be64(p);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_int64_t(struct xdr_stream *xdr, s64 *ptr)
> > +{
> > +	return xdrgen_decode_hyper(xdr, ptr);
> > +}
> > +
> > +static inline bool
> > +xdrgen_decode_unsigned_hyper(struct xdr_stream *xdr, u64 *ptr)
> > +{
> > +	__be32 *p = xdr_inline_decode(xdr, XDR_UNIT * 2);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*ptr = get_unaligned_be64(p);
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_opaque(struct xdr_stream *xdr, opaque *ptr, u32
> > maxlen)
> > +{
> > +	__be32 *p;
> > +	u32 len;
> > +
> > +	if (unlikely(xdr_stream_decode_u32(xdr, &len) !=
> > XDR_UNIT))
> > +		return false;
> > +	if (unlikely(maxlen && len > maxlen))
> > +		return false;
> > +	if (len != 0) {
> > +		p = xdr_inline_decode(xdr, len);
> > +		if (unlikely(!p))
> > +			return false;
> > +		ptr->data = (u8 *)p;
> > +	}
> > +	ptr->len = len;
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_string(struct xdr_stream *xdr, string *ptr, u32
> > maxlen)
> > +{
> > +	__be32 *p;
> > +	u32 len;
> > +
> > +	if (unlikely(xdr_stream_decode_u32(xdr, &len) !=
> > XDR_UNIT))
> > +		return false;
> > +	if (unlikely(maxlen && len > maxlen))
> > +		return false;
> > +	if (len != 0) {
> > +		p = xdr_inline_decode(xdr, len);
> > +		if (unlikely(!p))
> > +			return false;
> > +		ptr->data = (unsigned char *)p;
> > +	}
> > +	ptr->len = len;
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_void(struct xdr_stream *xdr)
> > +{
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_bool(struct xdr_stream *xdr, bool val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*p = val ? xdr_one : xdr_zero;
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_int(struct xdr_stream *xdr, s32 val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*p = cpu_to_be32(val);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_unsigned_int(struct xdr_stream *xdr, u32 val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*p = cpu_to_be32(val);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_uint32_t(struct xdr_stream *xdr, u32 val)
> > +{
> > +	return xdrgen_encode_unsigned_int(xdr, val);
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_long(struct xdr_stream *xdr, s32 val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*p = cpu_to_be32(val);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_unsigned_long(struct xdr_stream *xdr, u32 val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	*p = cpu_to_be32(val);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_hyper(struct xdr_stream *xdr, s64 val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT * 2);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	put_unaligned_be64(val, p);
> > +	return true;
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_int64_t(struct xdr_stream *xdr, s64 val)
> > +{
> > +	return xdrgen_encode_hyper(xdr, val);
> > +}
> > +
> > +static inline bool
> > +xdrgen_encode_unsigned_hyper(struct xdr_stream *xdr, u64 val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT * 2);
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	put_unaligned_be64(val, p);
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_opaque(struct xdr_stream *xdr, opaque val)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT +
> > xdr_align_size(val.len));
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	xdr_encode_opaque(p, val.data, val.len);
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_string(struct xdr_stream *xdr, string val, u32
> > maxlen)
> > +{
> > +	__be32 *p = xdr_reserve_space(xdr, XDR_UNIT +
> > xdr_align_size(val.len));
> > +
> > +	if (unlikely(!p))
> > +		return false;
> > +	xdr_encode_opaque(p, val.data, val.len);
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_fattr4_offline(struct xdr_stream *xdr,
> > fattr4_offline *ptr)
> > +{
> > +	return xdrgen_decode_bool(xdr, ptr);
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_bitmap4(struct xdr_stream *xdr, bitmap4 *ptr)
> > +{
> > +	if (xdr_stream_decode_u32(xdr, &ptr->count) < 0)
> > +		return false;
> > +	for (u32 i = 0; i < ptr->count; i++)
> > +		if (!xdrgen_decode_uint32_t(xdr, &ptr-
> > >element[i]))
> > +			return false;
> > +	return true;
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_open_arguments4(struct xdr_stream *xdr, struct
> > open_arguments4 *ptr)
> > +{
> > +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_access))
> > +		return false;
> > +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_share_deny))
> > +		return false;
> > +	if (!xdrgen_decode_bitmap4(xdr, &ptr-
> > >oa_share_access_want))
> > +		return false;
> > +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_open_claim))
> > +		return false;
> > +	if (!xdrgen_decode_bitmap4(xdr, &ptr->oa_create_mode))
> > +		return false;
> > +	return true;
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_open_args_share_access4(struct xdr_stream *xdr, enum
> > open_args_share_access4 *ptr)
> > +{
> > +	u32 val;
> > +
> > +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> > +		return false;
> > +	*ptr = val;
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_open_args_share_deny4(struct xdr_stream *xdr, enum
> > open_args_share_deny4 *ptr)
> > +{
> > +	u32 val;
> > +
> > +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> > +		return false;
> > +	*ptr = val;
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_open_args_share_access_want4(struct xdr_stream *xdr,
> > enum open_args_share_access_want4 *ptr)
> > +{
> > +	u32 val;
> > +
> > +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> > +		return false;
> > +	*ptr = val;
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_open_args_open_claim4(struct xdr_stream *xdr, enum
> > open_args_open_claim4 *ptr)
> > +{
> > +	u32 val;
> > +
> > +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> > +		return false;
> > +	*ptr = val;
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_open_args_createmode4(struct xdr_stream *xdr, enum
> > open_args_createmode4 *ptr)
> > +{
> > +	u32 val;
> > +
> > +	if (xdr_stream_decode_u32(xdr, &val) < 0)
> > +		return false;
> > +	*ptr = val;
> > +	return true;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_fattr4_open_arguments(struct xdr_stream *xdr,
> > fattr4_open_arguments *ptr)
> > +{
> > +	return xdrgen_decode_open_arguments4(xdr, ptr);
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_nfstime4(struct xdr_stream *xdr, struct _nfstime4
> > *ptr)
> > +{
> > +	if (!xdrgen_decode_int64_t(xdr, &ptr->seconds))
> > +		return false;
> > +	if (!xdrgen_decode_uint32_t(xdr, &ptr->nseconds))
> > +		return false;
> > +	return true;
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_fattr4_time_deleg_access(struct xdr_stream *xdr,
> > fattr4_time_deleg_access *ptr)
> > +{
> > +	return xdrgen_decode_nfstime4(xdr, ptr);
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_decode_fattr4_time_deleg_modify(struct xdr_stream *xdr,
> > fattr4_time_deleg_modify *ptr)
> > +{
> > +	return xdrgen_decode_nfstime4(xdr, ptr);
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_fattr4_offline(struct xdr_stream *xdr, const
> > fattr4_offline value)
> > +{
> > +	return xdrgen_encode_bool(xdr, value);
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_bitmap4(struct xdr_stream *xdr, const bitmap4 value)
> > +{
> > +	if (xdr_stream_encode_u32(xdr, value.count) != XDR_UNIT)
> > +		return false;
> > +	for (u32 i = 0; i < value.count; i++)
> > +		if (!xdrgen_encode_uint32_t(xdr,
> > value.element[i]))
> > +			return false;
> > +	return true;
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_open_arguments4(struct xdr_stream *xdr, const struct
> > open_arguments4 *value)
> > +{
> > +	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_access))
> > +		return false;
> > +	if (!xdrgen_encode_bitmap4(xdr, value->oa_share_deny))
> > +		return false;
> > +	if (!xdrgen_encode_bitmap4(xdr, value-
> > >oa_share_access_want))
> > +		return false;
> > +	if (!xdrgen_encode_bitmap4(xdr, value->oa_open_claim))
> > +		return false;
> > +	if (!xdrgen_encode_bitmap4(xdr, value->oa_create_mode))
> > +		return false;
> > +	return true;
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_open_args_share_access4(struct xdr_stream *xdr, enum
> > open_args_share_access4 value)
> > +{
> > +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_open_args_share_deny4(struct xdr_stream *xdr, enum
> > open_args_share_deny4 value)
> > +{
> > +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_open_args_share_access_want4(struct xdr_stream *xdr,
> > enum open_args_share_access_want4 value)
> > +{
> > +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_open_args_open_claim4(struct xdr_stream *xdr, enum
> > open_args_open_claim4 value)
> > +{
> > +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_open_args_createmode4(struct xdr_stream *xdr, enum
> > open_args_createmode4 value)
> > +{
> > +	return xdr_stream_encode_u32(xdr, value) == XDR_UNIT;
> > +}
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_fattr4_open_arguments(struct xdr_stream *xdr, const
> > fattr4_open_arguments *value)
> > +{
> > +	return xdrgen_encode_open_arguments4(xdr, value);
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_nfstime4(struct xdr_stream *xdr, const struct
> > _nfstime4 *value)
> > +{
> > +	if (!xdrgen_encode_int64_t(xdr, value->seconds))
> > +		return false;
> > +	if (!xdrgen_encode_uint32_t(xdr, value->nseconds))
> > +		return false;
> > +	return true;
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr,
> > const fattr4_time_deleg_access value)
> > +{
> > +	return xdrgen_encode_nfstime4(xdr, &value);
> > +};
> > +
> > +static bool __maybe_unused
> > +xdrgen_encode_fattr4_time_deleg_modify(struct xdr_stream *xdr,
> > const fattr4_time_deleg_modify value)
> > +{
> > +	return xdrgen_encode_nfstime4(xdr, &value);
> > +};
> > diff --git a/fs/nfsd/delstid_xdr.h b/fs/nfsd/delstid_xdr.h
> > new file mode 100644
> > index 000000000000..3ca8d0cc8569
> > --- /dev/null
> > +++ b/fs/nfsd/delstid_xdr.h
> > @@ -0,0 +1,102 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Generated by lkxdrgen, with hand edits. */
> > +/* XDR specification modification time: Wed Aug 14 13:35:03 2024
> > */
> > +
> > +#ifndef _DELSTID_H
> > +#define _DELSTID_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/sunrpc/xdr.h>
> > +#include <linux/sunrpc/svc.h>
> > +
> > +typedef struct {
> > +	u32 len;
> > +	unsigned char *data;
> > +} string;
> > +
> > +typedef struct {
> > +	u32 len;
> > +	u8 *data;
> > +} opaque;
> > +
> > +typedef struct {
> > +	u32 count;
> > +	uint32_t *element;
> > +} bitmap4;
> > +
> > +typedef struct _nfstime4 {
> > +	int64_t seconds;
> > +	uint32_t nseconds;
> > +} nfstime4;
> > +
> > +typedef bool fattr4_offline;
> > +
> > +#define FATTR4_OFFLINE (83)
> > +
> > +typedef struct open_arguments4 {
> > +	bitmap4 oa_share_access;
> > +	bitmap4 oa_share_deny;
> > +	bitmap4 oa_share_access_want;
> > +	bitmap4 oa_open_claim;
> > +	bitmap4 oa_create_mode;
> > +} open_arguments4;
> > +
> > +enum open_args_share_access4 {
> > +	OPEN_ARGS_SHARE_ACCESS_READ = 1,
> > +	OPEN_ARGS_SHARE_ACCESS_WRITE = 2,
> > +	OPEN_ARGS_SHARE_ACCESS_BOTH = 3,
> > +};
> > +
> > +enum open_args_share_deny4 {
> > +	OPEN_ARGS_SHARE_DENY_NONE = 0,
> > +	OPEN_ARGS_SHARE_DENY_READ = 1,
> > +	OPEN_ARGS_SHARE_DENY_WRITE = 2,
> > +	OPEN_ARGS_SHARE_DENY_BOTH = 3,
> > +};
> > +
> > +enum open_args_share_access_want4 {
> > +	OPEN_ARGS_SHARE_ACCESS_WANT_ANY_DELEG = 3,
> > +	OPEN_ARGS_SHARE_ACCESS_WANT_NO_DELEG = 4,
> > +	OPEN_ARGS_SHARE_ACCESS_WANT_CANCEL = 5,
> > +	OPEN_ARGS_SHARE_ACCESS_WANT_SIGNAL_DELEG_WHEN_RESRC_AVAIL
> > = 17,
> > +	OPEN_ARGS_SHARE_ACCESS_WANT_PUSH_DELEG_WHEN_UNCONTENDED =
> > 18,
> > +	OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 20,
> > +	OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION = 21,
> > +};
> > +
> > +enum open_args_open_claim4 {
> > +	OPEN_ARGS_OPEN_CLAIM_NULL = 0,
> > +	OPEN_ARGS_OPEN_CLAIM_PREVIOUS = 1,
> > +	OPEN_ARGS_OPEN_CLAIM_DELEGATE_CUR = 2,
> > +	OPEN_ARGS_OPEN_CLAIM_DELEGATE_PREV = 3,
> > +	OPEN_ARGS_OPEN_CLAIM_FH = 4,
> > +	OPEN_ARGS_OPEN_CLAIM_DELEG_CUR_FH = 5,
> > +	OPEN_ARGS_OPEN_CLAIM_DELEG_PREV_FH = 6,
> > +};
> > +
> > +enum open_args_createmode4 {
> > +	OPEN_ARGS_CREATEMODE_UNCHECKED4 = 0,
> > +	OPEN_ARGS_CREATE_MODE_GUARDED = 1,
> > +	OPEN_ARGS_CREATEMODE_EXCLUSIVE4 = 2,
> > +	OPEN_ARGS_CREATE_MODE_EXCLUSIVE4_1 = 3,
> > +};
> > +
> > +typedef open_arguments4 fattr4_open_arguments;
> > +
> > +#define FATTR4_OPEN_ARGUMENTS (86)
> > +
> > +#define OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION (0x200000)
> > +
> > +#define OPEN4_RESULT_NO_OPEN_STATEID (0x00000010)
> > +
> > +typedef nfstime4 fattr4_time_deleg_access;
> > +
> > +typedef nfstime4 fattr4_time_deleg_modify;
> > +
> > +#define FATTR4_TIME_DELEG_ACCESS (84)
> > +
> > +#define FATTR4_TIME_DELEG_MODIFY (85)
> > +
> > +#define OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS (0x100000)
> > +
> > +#endif /* _DELSTID_H */
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 643ca3f8ebb3..b3d2000c8a08 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -55,6 +55,7 @@
> >  #include "netns.h"
> >  #include "pnfs.h"
> >  #include "filecache.h"
> > +#include "delstid_xdr.h"
> >  
> >  #include "trace.h"
> >  
> > 
> > -- 
> > 2.46.0
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Chuck Lever]

On Fri, Aug 16, 2024 at 11:45:32AM -0400, Jeff Layton wrote:
> On Fri, 2024-08-16 at 11:17 -0400, Chuck Lever wrote:
> > On Fri, Aug 16, 2024 at 08:42:07AM -0400, Jeff Layton wrote:
> > > This adds support for the "delstid" draft:
> > > 
> > >     https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/
> > > 
> > > Most of this was autogenerated using Chuck's lkxdrgen tool with
> > > some
> > > by-hand tweaks to work around some symbol conflicts, and to add
> > > some
> > > missing pieces that were needed from nfs4_1.x.
> > 
> > I haven't read delstid closely enough to comment on the approach
> > you've taken in 3/3, but I do have some thoughts about code
> > organization here. I will try to study that draft soon.
> > 
> > And, I'm assuming you are continuing to evolve support for the draft
> > and will be growing this series. So I will hold off on careful
> > inspection of 3/3 for the moment.
> > 
> 
> Yes. The client pieces are already in place, and I read I get is that
> the draft is all but done at this point. 3/3 is probably pretty close
> to what should go in. There are really 3 parts to the delstid draft:
> 
> 1/ the OPEN_XOR_DELEGATION part, which allows the server to avoid
> giving out an open stateid when giving out a deleg.
> 
> 2/ delegated timestamp support (which is the part I'm still looking at)
> 
> 3/ FATTR4_OPEN_ARGUMENTS (which enables 1 and 2)
> 
> This patchset encompasses 1 & 3. Part 2 shouldn't have much bearing on
> this set. It's really a separate feature entirely that just happens to
> also depend on FATTR4_OPEN_ARGUMENTS support.
> 
> > First, I'm pleased that you found xdrgen useful for rapid
> > prototyping. That's not something I had envisioned when I created
> > the tool, but it's a good match, looks like.
> > 
> 
> Yeah. It has some bugs that still need fixing, but it was certainly
> better than having to roll all of that by hand.

I'm very interested to hear bug reports.


> > Here you add a separate set of source files for delstid XDR...  That
> > approach might not be scalable for adding subsequent new features in
> > general, it occurs to me.
> >
> > We might end up with a bunch of these little code blurbs with no
> > clear understanding of how they inter-relate.  Thoughts about how to
> > manage these are welcome: xdrgen could generate only header files
> > and then we would #include them where needed, for example.
> >
> > For now, I suggest folding the new generated XDR code into the
> > existing NFSv4 "open" XDR code in fs/nfsd/nfs4xdr.c, when you have
> > some time for cleaning up the patches. An alternative would be to
> > leave it and I can fold these together before committing.
> > 
> > (The long term, of course, will hopefully be generating all XDR code
> > automatically, but we're a ways out from that, IMO).
> > 
> 
> This is where I disagree.
> 
> The nice thing about lkxdrgen is that most of what it generates is
> fairly self-contained. I think we ought to try to keep the new (mostly
> autogenerated) and old code (hand-rolled) separate for now.
> 
> I'm not sure what makes the most sense for a new naming scheme, but I
> really don't think we should paste all of this into nfs4xdr.c, which is
> just a huge jumble of hand-rolled XDR code. 

nfs4xdr.c is a mix of stuff that I constructed by rote, which is
pretty clean, and stuff that mixes the "just serialize" logic with
"do the proc part as well" logic, which is more ugly. I had thought
that OPEN's XDR was in the former category, but I get it. 

So I still think there's a scalability problem with adding each new
feature in its own XDR .c and .h, but I don't mind keeping the
generated code separate from the legacy code. How about creating one
new file to collect the mostly- or all-generated XDR code?

I've been using fs/nfsd/nfs[34]gen_xdr.[ch] in my testing.

-- 
Chuck Lever

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton]

On Fri, 2024-08-16 at 12:16 -0400, Chuck Lever wrote:
> On Fri, Aug 16, 2024 at 11:45:32AM -0400, Jeff Layton wrote:
> > On Fri, 2024-08-16 at 11:17 -0400, Chuck Lever wrote:
> > > On Fri, Aug 16, 2024 at 08:42:07AM -0400, Jeff Layton wrote:
> > > > This adds support for the "delstid" draft:
> > > > 
> > > >    
> > > > https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/05/
> > > > 
> > > > Most of this was autogenerated using Chuck's lkxdrgen tool with
> > > > some
> > > > by-hand tweaks to work around some symbol conflicts, and to add
> > > > some
> > > > missing pieces that were needed from nfs4_1.x.
> > > 
> > > I haven't read delstid closely enough to comment on the approach
> > > you've taken in 3/3, but I do have some thoughts about code
> > > organization here. I will try to study that draft soon.
> > > 
> > > And, I'm assuming you are continuing to evolve support for the
> > > draft
> > > and will be growing this series. So I will hold off on careful
> > > inspection of 3/3 for the moment.
> > > 
> > 
> > Yes. The client pieces are already in place, and I read I get is
> > that
> > the draft is all but done at this point. 3/3 is probably pretty
> > close
> > to what should go in. There are really 3 parts to the delstid
> > draft:
> > 
> > 1/ the OPEN_XOR_DELEGATION part, which allows the server to avoid
> > giving out an open stateid when giving out a deleg.
> > 
> > 2/ delegated timestamp support (which is the part I'm still looking
> > at)
> > 
> > 3/ FATTR4_OPEN_ARGUMENTS (which enables 1 and 2)
> > 
> > This patchset encompasses 1 & 3. Part 2 shouldn't have much bearing
> > on
> > this set. It's really a separate feature entirely that just happens
> > to
> > also depend on FATTR4_OPEN_ARGUMENTS support.
> > 
> > > First, I'm pleased that you found xdrgen useful for rapid
> > > prototyping. That's not something I had envisioned when I created
> > > the tool, but it's a good match, looks like.
> > > 
> > 
> > Yeah. It has some bugs that still need fixing, but it was certainly
> > better than having to roll all of that by hand.
> 
> I'm very interested to hear bug reports.
> 

I should have been more diligent about collecting them! I'll do that
soon.

> 
> > > Here you add a separate set of source files for delstid XDR... 
> > > That
> > > approach might not be scalable for adding subsequent new features
> > > in
> > > general, it occurs to me.
> > > 
> > > We might end up with a bunch of these little code blurbs with no
> > > clear understanding of how they inter-relate.  Thoughts about how
> > > to
> > > manage these are welcome: xdrgen could generate only header files
> > > and then we would #include them where needed, for example.
> > > 
> > > For now, I suggest folding the new generated XDR code into the
> > > existing NFSv4 "open" XDR code in fs/nfsd/nfs4xdr.c, when you
> > > have
> > > some time for cleaning up the patches. An alternative would be to
> > > leave it and I can fold these together before committing.
> > > 
> > > (The long term, of course, will hopefully be generating all XDR
> > > code
> > > automatically, but we're a ways out from that, IMO).
> > > 
> > 
> > This is where I disagree.
> > 
> > The nice thing about lkxdrgen is that most of what it generates is
> > fairly self-contained. I think we ought to try to keep the new
> > (mostly
> > autogenerated) and old code (hand-rolled) separate for now.
> > 
> > I'm not sure what makes the most sense for a new naming scheme, but
> > I
> > really don't think we should paste all of this into nfs4xdr.c,
> > which is
> > just a huge jumble of hand-rolled XDR code. 
> 
> nfs4xdr.c is a mix of stuff that I constructed by rote, which is
> pretty clean, and stuff that mixes the "just serialize" logic with
> "do the proc part as well" logic, which is more ugly. I had thought
> that OPEN's XDR was in the former category, but I get it. 
> 
> So I still think there's a scalability problem with adding each new
> feature in its own XDR .c and .h, but I don't mind keeping the
> generated code separate from the legacy code. How about creating one
> new file to collect the mostly- or all-generated XDR code?
> 
> I've been using fs/nfsd/nfs[34]gen_xdr.[ch] in my testing.
> 

Sure, that sounds fine. I'll rename delstid_xdr.* to nfs4gen_xdr.*.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[NeilBrown]

On Fri, 16 Aug 2024, Jeff Layton wrote:

> +// Generated by lkxdrgen, with hand-edits.

I *really* don't like having code in the kernel that is partly
tool-generated and partly human-generated, and where the boundary isn't
obvious (like separate files).

If we cannot use tool-generated code as-is, then let's fix the tool.
If we cannot fix the tool, then include the raw output and a
human-generated patch which the makefile combines.

Ideally the tool should be in tools/, the .x file should be in fs/nfsd/
and the makefile should apply the one to the other.  We are going to
want to do that eventually and I think it should be priority.  The tool
doesn't have to be bug-free before it lands (nothing is).

A particular reason for this is that I cannot review tool-generated
hand-editted code.  It is too noisy and I don't know which parts are
worth closer inspection etc.

Thanks,
NeilBrown

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton]

On Mon, 2024-08-19 at 10:04 +1000, NeilBrown wrote:
> On Fri, 16 Aug 2024, Jeff Layton wrote:
> 
> > +// Generated by lkxdrgen, with hand-edits.
> 
> I *really* don't like having code in the kernel that is partly
> tool-generated and partly human-generated, and where the boundary isn't
> obvious (like separate files).
> 
> If we cannot use tool-generated code as-is, then let's fix the tool.
> If we cannot fix the tool, then include the raw output and a
> human-generated patch which the makefile combines.
> 
> Ideally the tool should be in tools/, the .x file should be in fs/nfsd/
> and the makefile should apply the one to the other.  We are going to
> want to do that eventually and I think it should be priority.  The tool
> doesn't have to be bug-free before it lands (nothing is).
> 
> A particular reason for this is that I cannot review tool-generated
> hand-editted code.  It is too noisy and I don't know which parts are
> worth closer inspection etc.

Fair point. Chuck made some similar comments to me privately, and it
looks like he has updated his xdrgen tool as well.

I'll plan to just respin that part from scratch and regenerate from the
.x files. I'll also keep my hand-edits in a separate commit for the
next version.

It'll probably take me a few days, but I'll try to have something to
resend within the next week or so.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Chuck Lever III]

> On Aug 19, 2024, at 7:44 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2024-08-19 at 10:04 +1000, NeilBrown wrote:
>> On Fri, 16 Aug 2024, Jeff Layton wrote:
>> 
>>> +// Generated by lkxdrgen, with hand-edits.
>> 
>> I *really* don't like having code in the kernel that is partly
>> tool-generated and partly human-generated, and where the boundary isn't
>> obvious (like separate files).
>> 
>> If we cannot use tool-generated code as-is, then let's fix the tool.
>> If we cannot fix the tool, then include the raw output and a
>> human-generated patch which the makefile combines.
>> 
>> Ideally the tool should be in tools/, the .x file should be in fs/nfsd/
>> and the makefile should apply the one to the other.  We are going to
>> want to do that eventually and I think it should be priority.  The tool
>> doesn't have to be bug-free before it lands (nothing is).
>> 
>> A particular reason for this is that I cannot review tool-generated
>> hand-editted code.  It is too noisy and I don't know which parts are
>> worth closer inspection etc.
> 
> Fair point. Chuck made some similar comments to me privately, and it
> looks like he has updated his xdrgen tool as well.
> 
> I'll plan to just respin that part from scratch and regenerate from the
> .x files. I'll also keep my hand-edits in a separate commit for the
> next version.
> 
> It'll probably take me a few days, but I'll try to have something to
> resend within the next week or so.

Meanwhile, I'll post the current xdrgen tool for review. It
already lives under tools/ as Neil suggested above.

I'm hoping that by providing the .x snippet used to generate the
source, and by adding one or two "pragma" annotations to the tool
to handle certain exceptions, no hand-edits or extra patching
will be needed.


--
Chuck Lever

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton]

On Mon, 2024-08-19 at 13:21 +0000, Chuck Lever III wrote:
> 
> > On Aug 19, 2024, at 7:44 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Mon, 2024-08-19 at 10:04 +1000, NeilBrown wrote:
> > > On Fri, 16 Aug 2024, Jeff Layton wrote:
> > > 
> > > > +// Generated by lkxdrgen, with hand-edits.
> > > 
> > > I *really* don't like having code in the kernel that is partly
> > > tool-generated and partly human-generated, and where the boundary isn't
> > > obvious (like separate files).
> > > 
> > > If we cannot use tool-generated code as-is, then let's fix the tool.
> > > If we cannot fix the tool, then include the raw output and a
> > > human-generated patch which the makefile combines.
> > > 
> > > Ideally the tool should be in tools/, the .x file should be in fs/nfsd/
> > > and the makefile should apply the one to the other.  We are going to
> > > want to do that eventually and I think it should be priority.  The tool
> > > doesn't have to be bug-free before it lands (nothing is).
> > > 
> > > A particular reason for this is that I cannot review tool-generated
> > > hand-editted code.  It is too noisy and I don't know which parts are
> > > worth closer inspection etc.
> > 
> > Fair point. Chuck made some similar comments to me privately, and it
> > looks like he has updated his xdrgen tool as well.
> > 
> > I'll plan to just respin that part from scratch and regenerate from the
> > .x files. I'll also keep my hand-edits in a separate commit for the
> > next version.
> > 
> > It'll probably take me a few days, but I'll try to have something to
> > resend within the next week or so.
> 
> Meanwhile, I'll post the current xdrgen tool for review. It
> already lives under tools/ as Neil suggested above.
> 
> I'm hoping that by providing the .x snippet used to generate the
> source, and by adding one or two "pragma" annotations to the tool
> to handle certain exceptions, no hand-edits or extra patching
> will be needed.
> 
> 

I'm playing with the new version now and it seems to be much improved.
Only two real bugs I've hit at this point:

1/ Some of the struct specifications need to be typedefs as well. For
instance, the delstid draft refers to "nfstime4", but the autogenerated
struct definition doesn't have the typedef for it. It may be best to
just add typedefs for all of these sorts of structs.

2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
xdrgen_encode_fattr4_time_deleg_modify try to pass it by value instead.

These are minor and easily fixed, obviously, but it would be nice to
not need to do that.

Nice work!
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Chuck Lever III]

> On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2024-08-19 at 13:21 +0000, Chuck Lever III wrote:
>> 
>>> On Aug 19, 2024, at 7:44 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> On Mon, 2024-08-19 at 10:04 +1000, NeilBrown wrote:
>>>> On Fri, 16 Aug 2024, Jeff Layton wrote:
>>>> 
>>>>> +// Generated by lkxdrgen, with hand-edits.
>>>> 
>>>> I *really* don't like having code in the kernel that is partly
>>>> tool-generated and partly human-generated, and where the boundary isn't
>>>> obvious (like separate files).
>>>> 
>>>> If we cannot use tool-generated code as-is, then let's fix the tool.
>>>> If we cannot fix the tool, then include the raw output and a
>>>> human-generated patch which the makefile combines.
>>>> 
>>>> Ideally the tool should be in tools/, the .x file should be in fs/nfsd/
>>>> and the makefile should apply the one to the other.  We are going to
>>>> want to do that eventually and I think it should be priority.  The tool
>>>> doesn't have to be bug-free before it lands (nothing is).
>>>> 
>>>> A particular reason for this is that I cannot review tool-generated
>>>> hand-editted code.  It is too noisy and I don't know which parts are
>>>> worth closer inspection etc.
>>> 
>>> Fair point. Chuck made some similar comments to me privately, and it
>>> looks like he has updated his xdrgen tool as well.
>>> 
>>> I'll plan to just respin that part from scratch and regenerate from the
>>> .x files. I'll also keep my hand-edits in a separate commit for the
>>> next version.
>>> 
>>> It'll probably take me a few days, but I'll try to have something to
>>> resend within the next week or so.
>> 
>> Meanwhile, I'll post the current xdrgen tool for review. It
>> already lives under tools/ as Neil suggested above.
>> 
>> I'm hoping that by providing the .x snippet used to generate the
>> source, and by adding one or two "pragma" annotations to the tool
>> to handle certain exceptions, no hand-edits or extra patching
>> will be needed.
>> 
>> 
> 
> I'm playing with the new version now and it seems to be much improved.
> Only two real bugs I've hit at this point:
> 
> 1/ Some of the struct specifications need to be typedefs as well. For
> instance, the delstid draft refers to "nfstime4", but the autogenerated
> struct definition doesn't have the typedef for it. It may be best to
> just add typedefs for all of these sorts of structs.
> 
> 2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
> autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
> xdrgen_encode_fattr4_time_deleg_modify try to pass it by value instead.
> 
> These are minor and easily fixed, obviously, but it would be nice to
> not need to do that.

That might be a bug in the spec, actually. I'll have a look.

--
Chuck Lever

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Chuck Lever III]

> On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> I'm playing with the new version now and it seems to be much improved.
> Only two real bugs I've hit at this point:
> 
> 1/ Some of the struct specifications need to be typedefs as well. For
> instance, the delstid draft refers to "nfstime4", but the autogenerated
> struct definition doesn't have the typedef for it. It may be best to
> just add typedefs for all of these sorts of structs.

What's the specific symptom? I've been able to catenate nfs4_1.x
and delstid.x, xdrgen builds the header and source without tossing
any exceptions, and gcc compiles it without complaint.

AFAICT, xdrgen will add "struct" where it's necessary.

I've been squirrelly about using "typedef" too often because
the Linux kernel's coding style is to avoid C typedefs for
shorthand structure names.


> 2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
> autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
> xdrgen_encode_fattr4_time_deleg_modify try to pass it by value instead.

Here's my generated copy of xdrgen_encode_fattr_time_deleg_access:

/* typedef fattr4_time_deleg_access */
static bool __maybe_unused                                                      xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const fattr4_time_deleg_access value)
{
	/* (basic) */
	return xdrgen_encode_nfstime4(xdr, &value);
};

Looks like it does the right thing...?


--
Chuck Lever

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton]

On Mon, 2024-08-19 at 19:50 +0000, Chuck Lever III wrote:
> 
> 
> > On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@kernel.org>
> > wrote:
> > 
> > I'm playing with the new version now and it seems to be much
> > improved.
> > Only two real bugs I've hit at this point:
> > 
> > 1/ Some of the struct specifications need to be typedefs as well.
> > For
> > instance, the delstid draft refers to "nfstime4", but the
> > autogenerated
> > struct definition doesn't have the typedef for it. It may be best
> > to
> > just add typedefs for all of these sorts of structs.
> 
> What's the specific symptom? I've been able to catenate nfs4_1.x
> and delstid.x, xdrgen builds the header and source without tossing
> any exceptions, and gcc compiles it without complaint.
> 


Basically, I was getting this when I'd convert nfs4_1.x to a header:

struct nfstime4 {
        int64_t seconds;
        uint32_t nseconds;
};

...but the delstid header has these:

typedef nfstime4 fattr4_time_deleg_access;

typedef nfstime4 fattr4_time_deleg_modify;


...nothing defined nfstime4 in this case.

> AFAICT, xdrgen will add "struct" where it's necessary.
> 
> I've been squirrelly about using "typedef" too often because
> the Linux kernel's coding style is to avoid C typedefs for
> shorthand structure names.
> 

Oh, ok. I didn't concatenate the files like you did and just generated
the delstid files separately from the nfs4_1 ones. I guess that throws
off the dependency tracking that you're doing here for typedefs.

> 
> > 2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
> > autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
> > xdrgen_encode_fattr4_time_deleg_modify try to pass it by value
> > instead.
> 
> Here's my generated copy of xdrgen_encode_fattr_time_deleg_access:
> 
> /* typedef fattr4_time_deleg_access */
> static bool
> __maybe_unused                                                     
> xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const
> fattr4_time_deleg_access value)
> {
> 	/* (basic) */
> 	return xdrgen_encode_nfstime4(xdr, &value);
> };
> 
> Looks like it does the right thing...?

Probably another side-effect of it not knowing what to do with nfstime4
when I convert the delstid draft. Concatenating them seems unwieldy but
I guess that would work. I do like being able to keep generated code
from different files separate though.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Chuck Lever III]

> On Aug 19, 2024, at 4:04 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2024-08-19 at 19:50 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@kernel.org>
>>> wrote:
>>> 
>>> I'm playing with the new version now and it seems to be much
>>> improved.
>>> Only two real bugs I've hit at this point:
>>> 
>>> 1/ Some of the struct specifications need to be typedefs as well.
>>> For
>>> instance, the delstid draft refers to "nfstime4", but the
>>> autogenerated
>>> struct definition doesn't have the typedef for it. It may be best
>>> to
>>> just add typedefs for all of these sorts of structs.
>> 
>> What's the specific symptom? I've been able to catenate nfs4_1.x
>> and delstid.x, xdrgen builds the header and source without tossing
>> any exceptions, and gcc compiles it without complaint.
>> 
> 
> 
> Basically, I was getting this when I'd convert nfs4_1.x to a header:
> 
> struct nfstime4 {
>        int64_t seconds;
>        uint32_t nseconds;
> };
> 
> ...but the delstid header has these:
> 
> typedef nfstime4 fattr4_time_deleg_access;
> 
> typedef nfstime4 fattr4_time_deleg_modify;
> 
> 
> ...nothing defined nfstime4 in this case.
> 
>> AFAICT, xdrgen will add "struct" where it's necessary.
>> 
>> I've been squirrelly about using "typedef" too often because
>> the Linux kernel's coding style is to avoid C typedefs for
>> shorthand structure names.
>> 
> 
> Oh, ok. I didn't concatenate the files like you did and just generated
> the delstid files separately from the nfs4_1 ones. I guess that throws
> off the dependency tracking that you're doing here for typedefs.

cat'ing the two files together is the spec-recommended approach,
but it assumes you're generating the whole protocol at once.
Here it was just a quick and dirty way for me to build a
reproducer.

For an initial fs/nfsd/nfs4_1.x file, I recommend starting with
delstid.x, and then add the pieces of the NFSv4_1 XDR until
xdrgen and gcc can make proper sense of it.

I can take a stab at that if you like, and send you something
tomorrow?


Sidebar: We could go with all typedefs for structs, unions, and
enums. That would make C code generation easier. Something like:

typedef struct {
	int64_t seconds;
	uint32_t nseconds;
} nfstime4;

But like I said, I expect that approach might be frowned upon.


>>> 2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
>>> autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
>>> xdrgen_encode_fattr4_time_deleg_modify try to pass it by value
>>> instead.
>> 
>> Here's my generated copy of xdrgen_encode_fattr_time_deleg_access:
>> 
>> /* typedef fattr4_time_deleg_access */
>> static bool
>> __maybe_unused                                                     
>> xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const
>> fattr4_time_deleg_access value)
>> {
>> /* (basic) */
>> return xdrgen_encode_nfstime4(xdr, &value);
>> };
>> 
>> Looks like it does the right thing...?
> 
> Probably another side-effect of it not knowing what to do with nfstime4
> when I convert the delstid draft. Concatenating them seems unwieldy but
> I guess that would work. I do like being able to keep generated code
> from different files separate though.

I don't think cat'ing the .x files is /required/, but it was a
quick way to get started.

Having a working nfs4_1.x that can generate the small piece of
XDR code that we need, in a separate file that can be augmented
over time, I think, is a win. I don't see that anything so far
is preventing that.

--
Chuck Lever

Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton]

On Mon, 2024-08-19 at 23:16 +0000, Chuck Lever III wrote:
> 
> > On Aug 19, 2024, at 4:04 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Mon, 2024-08-19 at 19:50 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@kernel.org>
> > > > wrote:
> > > > 
> > > > I'm playing with the new version now and it seems to be much
> > > > improved.
> > > > Only two real bugs I've hit at this point:
> > > > 
> > > > 1/ Some of the struct specifications need to be typedefs as well.
> > > > For
> > > > instance, the delstid draft refers to "nfstime4", but the
> > > > autogenerated
> > > > struct definition doesn't have the typedef for it. It may be best
> > > > to
> > > > just add typedefs for all of these sorts of structs.
> > > 
> > > What's the specific symptom? I've been able to catenate nfs4_1.x
> > > and delstid.x, xdrgen builds the header and source without tossing
> > > any exceptions, and gcc compiles it without complaint.
> > > 
> > 
> > 
> > Basically, I was getting this when I'd convert nfs4_1.x to a header:
> > 
> > struct nfstime4 {
> >        int64_t seconds;
> >        uint32_t nseconds;
> > };
> > 
> > ...but the delstid header has these:
> > 
> > typedef nfstime4 fattr4_time_deleg_access;
> > 
> > typedef nfstime4 fattr4_time_deleg_modify;
> > 
> > 
> > ...nothing defined nfstime4 in this case.
> > 
> > > AFAICT, xdrgen will add "struct" where it's necessary.
> > > 
> > > I've been squirrelly about using "typedef" too often because
> > > the Linux kernel's coding style is to avoid C typedefs for
> > > shorthand structure names.
> > > 
> > 
> > Oh, ok. I didn't concatenate the files like you did and just generated
> > the delstid files separately from the nfs4_1 ones. I guess that throws
> > off the dependency tracking that you're doing here for typedefs.
> 
> cat'ing the two files together is the spec-recommended approach,
> but it assumes you're generating the whole protocol at once.
> Here it was just a quick and dirty way for me to build a
> reproducer.
> 
> For an initial fs/nfsd/nfs4_1.x file, I recommend starting with
> delstid.x, and then add the pieces of the NFSv4_1 XDR until
> xdrgen and gcc can make proper sense of it.
> 
> I can take a stab at that if you like, and send you something
> tomorrow?
> 

I think that will probably fix the problem I was having before. I'll
respin that part of the set soon. It's probably better that I do it so
I figure this out. This is the "easy" XDR vs. CB_NOTIFY.

> 
> Sidebar: We could go with all typedefs for structs, unions, and
> enums. That would make C code generation easier. Something like:
> 
> typedef struct {
> 	int64_t seconds;
> 	uint32_t nseconds;
> } nfstime4;
> 
> But like I said, I expect that approach might be frowned upon.
> 

Agreed. I don't think it's needed. I was just using the tool wrong
before.

Thanks,

> 
> > > > 2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
> > > > autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
> > > > xdrgen_encode_fattr4_time_deleg_modify try to pass it by value
> > > > instead.
> > > 
> > > Here's my generated copy of xdrgen_encode_fattr_time_deleg_access:
> > > 
> > > /* typedef fattr4_time_deleg_access */
> > > static bool
> > > __maybe_unused                                                     
> > > xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const
> > > fattr4_time_deleg_access value)
> > > {
> > > /* (basic) */
> > > return xdrgen_encode_nfstime4(xdr, &value);
> > > };
> > > 
> > > Looks like it does the right thing...?
> > 
> > Probably another side-effect of it not knowing what to do with nfstime4
> > when I convert the delstid draft. Concatenating them seems unwieldy but
> > I guess that would work. I do like being able to keep generated code
> > from different files separate though.
> 
> I don't think cat'ing the .x files is /required/, but it was a
> quick way to get started.
> 
> Having a working nfs4_1.x that can generate the small piece of
> XDR code that we need, in a separate file that can be augmented
> over time, I think, is a win. I don't see that anything so far
> is preventing that.
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>