| Message ID | 20240816-delstid-v1-1-c221c3dc14cd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: implement OPEN_XOR_DELEGATION part of delstid draft | expand |
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 >
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 > > >
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.
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.*.
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
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.
> 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
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!
> 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
> 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
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.
> 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
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 > >
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"
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(-)