Re: [PATCH v3 08/13] nfs_common: make nfs4.h include generated nfs4_1.h

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>
> > ---
> >  {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@kernel.org>