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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZtCTsNfht6O7uxev@tissot.1015granger.net>
Date: Thu, 29 Aug 2024 11:28:48 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>
Cc: Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        Trond Myklebust <trondmy@...nel.org>, Anna Schumaker <anna@...nel.org>,
        Olga Kornievskaia <okorniev@...hat.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Jonathan Corbet <corbet@....net>, Tom Haynes <loghyr@...il.com>,
        linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 08/13] nfs_common: make nfs4.h include generated
 nfs4_1.h

On Thu, Aug 29, 2024 at 11:13:49AM -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.
> 
> 
> > 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.
> 
> 
> > 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.
> 
> 
> > 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@...nel.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).
> 
> 
> >  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).

Actually, renaming the header guard macro makes sense. I can adjust
xdrgen to do that.


> >  #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 */

-- 
Chuck Lever