On Thu, 2024-08-29 at 11:13 -0400, Chuck Lever wrote:
> On Thu, Aug 29, 2024 at 09:26:46AM -0400, Jeff Layton wrote:
> > Long term, we'd like to move to autogenerating a lot of our XDR code.
> > Both the client and server include include/linux/nfs4.h. That file is
> > hand-rolled and some of the symbols in it conflict with the
> > autogenerated symbols from the spec.
> >
> > Move nfs4_1.x to Documentation/sunrpc/xdr.
>
> I can change 2/2 in the xdrgen series to land this file under
> Documentation/sunrpc/xdr.
>
Thanks, and also thanks for making the timestamp handling functions
public too.
>
> > Create a new include/linux/sunrpc/xdrgen directory in which we can
> > keep autogenerated header files.
>
> I think the header files will have different content for the client
> and server. For example, the server-side header has function
> declarations for the procedure argument and result XDR functions,
> the client doesn't (because those functions are all static on the
> client side).
>
> Not sure we're ready for this level of sharing between client and
> server.
>
Does that matter though? Sure the client might be exposed to some
server encoding/decoding functions, but it doesn't have to use them.
The constant and type definitions are the same between the client and
server. I think there is value in having a single source for that, like
we have today with nfs4.h.
If we do decide that it's a problem, we can just split things up
further:
1. one header file with constant, struct and type definitions
2. one with server-side encode/decode prototypes that includes #1
3. one with client-side encode/decode prototypes that includes #1
include/linux/nfs4.h could still include #1 as well, but the client and
server could include the appropriate headers instead of or in addition
to it.
> > Move the new, generated nfs4xdr_gen.h file to nfs4_1.h in
> > that directory.
>
> I'd rather keep the current file name to indicate that it's
> generated code.
>
I figured the "xdrgen" directory name would convey that. This naming
also makes it clearer that it's generated from nfs4_1.x. That said, I'm
not too particular here. Can you lay out how you think we ought to
arrange things?
>
> > Have include/linux/nfs4.h include the newly renamed file
> > and then remove conflicting definitions from it and nfs_xdr.h.
> >
> > For now, the .x file from which we're generating the header is fairly
> > small and just covers the delstid draft, but we can expand that in the
> > future and just remove conflicting definitions as we go.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > {fs/nfsd => Documentation/sunrpc/xdr}/nfs4_1.x | 0
> > MAINTAINERS | 1 +
> > fs/nfsd/nfs4xdr_gen.c | 2 +-
> > include/linux/nfs4.h | 7 +------
> > include/linux/nfs_xdr.h | 5 -----
> > fs/nfsd/nfs4xdr_gen.h => include/linux/sunrpc/xdrgen/nfs4_1.h | 6 +++---
> > 6 files changed, 6 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4_1.x b/Documentation/sunrpc/xdr/nfs4_1.x
> > similarity index 100%
> > rename from fs/nfsd/nfs4_1.x
> > rename to Documentation/sunrpc/xdr/nfs4_1.x
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a70b7c9c3533..e85114273238 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12175,6 +12175,7 @@ S: Supported
> > B: https://bugzilla.kernel.org
> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> > F: Documentation/filesystems/nfs/
> > +F: Documentation/sunrpc/xdr/
> > F: fs/lockd/
> > F: fs/nfs_common/
> > F: fs/nfsd/
> > diff --git a/fs/nfsd/nfs4xdr_gen.c b/fs/nfsd/nfs4xdr_gen.c
> > index 6833d0ad35a8..00e803781c87 100644
> > --- a/fs/nfsd/nfs4xdr_gen.c
> > +++ b/fs/nfsd/nfs4xdr_gen.c
> > @@ -2,7 +2,7 @@
> > // Generated by xdrgen. Manual edits will be lost.
> > // XDR specification modification time: Wed Aug 28 09:57:28 2024
> >
> > -#include "nfs4xdr_gen.h"
> > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
>
> Please don't hand-edit these files. That makes it impossible to just
> run the xdrgen tool and get a new version, which is the real goal.
>
> If you need different generated content, change the tool to generate
> what you need (or feel free to ask me to get out my whittling
> knife).
>
>
No problem. This part is a Q&D hack job to get everything working with
minimal changes. Changing the tool to generate the right thing would be
a better long-term solution (once we settle on where these files will
go, etc.)
> > static bool __maybe_unused
> > xdrgen_decode_int64_t(struct xdr_stream *xdr, int64_t *ptr)
> > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> > index 8d7430d9f218..b90719244775 100644
> > --- a/include/linux/nfs4.h
> > +++ b/include/linux/nfs4.h
> > @@ -17,6 +17,7 @@
> > #include <linux/uidgid.h>
> > #include <uapi/linux/nfs4.h>
> > #include <linux/sunrpc/msg_prot.h>
> > +#include <linux/sunrpc/xdrgen/nfs4_1.h>
> >
> > enum nfs4_acl_whotype {
> > NFS4_ACL_WHO_NAMED = 0,
> > @@ -512,12 +513,6 @@ enum {
> > FATTR4_XATTR_SUPPORT = 82,
> > };
> >
> > -enum {
> > - FATTR4_TIME_DELEG_ACCESS = 84,
> > - FATTR4_TIME_DELEG_MODIFY = 85,
> > - FATTR4_OPEN_ARGUMENTS = 86,
> > -};
> > -
> > /*
> > * The following internal definitions enable processing the above
> > * attribute bits within 32-bit word boundaries.
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index 45623af3e7b8..d3fe47baf110 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -1315,11 +1315,6 @@ struct nfs4_fsid_present_res {
> >
> > #endif /* CONFIG_NFS_V4 */
> >
> > -struct nfstime4 {
> > - u64 seconds;
> > - u32 nseconds;
> > -};
> > -
> > #ifdef CONFIG_NFS_V4_1
> >
> > struct pnfs_commit_bucket {
> > diff --git a/fs/nfsd/nfs4xdr_gen.h b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > similarity index 96%
> > rename from fs/nfsd/nfs4xdr_gen.h
> > rename to include/linux/sunrpc/xdrgen/nfs4_1.h
> > index 5465db4fb32b..5faee67281b8 100644
> > --- a/fs/nfsd/nfs4xdr_gen.h
> > +++ b/include/linux/sunrpc/xdrgen/nfs4_1.h
> > @@ -2,8 +2,8 @@
> > /* Generated by xdrgen. Manual edits will be lost. */
> > /* XDR specification modification time: Wed Aug 28 09:57:28 2024 */
> >
> > -#ifndef _LINUX_NFS4_XDRGEN_H
> > -#define _LINUX_NFS4_XDRGEN_H
> > +#ifndef _LINUX_XDRGEN_NFS4_H
> > +#define _LINUX_XDRGEN_NFS4_H
>
> Ditto. Resist The Urge (tm).
>
>
> > #include <linux/types.h>
> > #include <linux/sunrpc/svc.h>
> > @@ -103,4 +103,4 @@ enum { FATTR4_TIME_DELEG_MODIFY = 85 };
> >
> > enum { OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS = 0x100000 };
> >
> > -#endif /* _LINUX_NFS4_XDRGEN_H */
> > +#endif /* _LINUX_XDRGEN_NFS4_H */
> >
> > --
> > 2.46.0
> >
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
