Re: [RFC PATCH 2/2] NFSD: Create an initial nfs4_1.x file

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-08-21 at 13:03 -0400, Chuck Lever wrote:
> On Wed, Aug 21, 2024 at 12:51:51PM -0400, Jeff Layton wrote:
> > On Wed, 2024-08-21 at 10:38 -0400, Chuck Lever wrote:
> > > On Wed, Aug 21, 2024 at 10:22:15AM -0400, Jeff Layton wrote:
> > > > On Tue, 2024-08-20 at 10:46 -0400, cel@kernel.org wrote:
> > > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > > 
> > > > > Build an NFSv4 protocol snippet to support the delstid
> > > > > extensions.
> > > > > The new fs/nfsd/nfs4_1.x file can be added to over time as
> > > > > other
> > > > > parts of NFSD's XDR functions are converted to machine-
> > > > > generated
> > > > > code.
> > > > > 
> > > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > > ---
> > > > >  fs/nfsd/nfs4_1.x      | 164 +++++++++++++++++++++++++++++
> > > > >  fs/nfsd/nfs4xdr_gen.c | 236
> > > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > >  fs/nfsd/nfs4xdr_gen.h | 113 ++++++++++++++++++++
> > > > >  3 files changed, 513 insertions(+)
> > > > >  create mode 100644 fs/nfsd/nfs4_1.x
> > > > >  create mode 100644 fs/nfsd/nfs4xdr_gen.c
> > > > >  create mode 100644 fs/nfsd/nfs4xdr_gen.h
> > > > > 
> > > > 
> > > > I see the patches in your lkxdrgen branch. I gave this a try
> > > > and
> > > > started rebasing my delstid work on top of it, but I hit the
> > > > same
> > > > symbol conflicts I hit before once I started trying to include
> > > > the
> > > > full-blown nfs4xdr_gen.h header:
> > > > 
> > > > ------------------------8<---------------------------
> > > > In file included from fs/nfsd/nfs4xdr.c:58:
> > > > fs/nfsd/nfs4xdr_gen.h:86:9: error: redeclaration of enumerator
> > > > ‘FATTR4_OPEN_ARGUMENTS’
> > > >    86 |         FATTR4_OPEN_ARGUMENTS = 86
> > > >       |         ^~~~~~~~~~~~~~~~~~~~~
> > > > In file included from fs/nfsd/nfsfh.h:15,
> > > >                  from fs/nfsd/state.h:41,
> > > >                  from fs/nfsd/xdr4.h:40,
> > > >                  from fs/nfsd/nfs4xdr.c:51:
> > > > ./include/linux/nfs4.h:518:9: note: previous definition of
> > > > ‘FATTR4_OPEN_ARGUMENTS’ with type ‘enum <anonymous>’
> > > >   518 |         FATTR4_OPEN_ARGUMENTS           = 86,
> > > >       |         ^~~~~~~~~~~~~~~~~~~~~
> > > > fs/nfsd/nfs4xdr_gen.h:102:9: error: redeclaration of enumerator
> > > > ‘FATTR4_TIME_DELEG_ACCESS’
> > > >   102 |         FATTR4_TIME_DELEG_ACCESS = 84
> > > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/nfs4.h:516:9: note: previous definition of
> > > > ‘FATTR4_TIME_DELEG_ACCESS’ with type ‘enum <anonymous>’
> > > >   516 |         FATTR4_TIME_DELEG_ACCESS        = 84,
> > > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~
> > > > fs/nfsd/nfs4xdr_gen.h:106:9: error: redeclaration of enumerator
> > > > ‘FATTR4_TIME_DELEG_MODIFY’
> > > >   106 |         FATTR4_TIME_DELEG_MODIFY = 85
> > > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/nfs4.h:517:9: note: previous definition of
> > > > ‘FATTR4_TIME_DELEG_MODIFY’ with type ‘enum <anonymous>’
> > > >   517 |         FATTR4_TIME_DELEG_MODIFY        = 85,
> > > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~
> > > > ------------------------8<---------------------------
> > > > 
> > > > I'm not sure of the best way to work around this, unless we
> > > > want to
> > > > try
> > > > to split up nfs4.h.
> > > 
> > > That header is shared with the client, so I consider it immutable
> > > for our purposes here.
> > > 
> > > 
> > > One option would be to namespace the generated data items. Eg,
> > > name
> > > them:
> > > 
> > > 	XG_FATTR4_TIME_DELEG_ACCESS
> > > 	XG_FATTR4_TIME_DELEG_MODIFY
> > > 
> > > That way they don't conflict with existing definitions.
> > > 
> > 
> > I'd rather avoid that if we can. Having things named exactly the
> > same
> > as in the spec makes the code easier to read and understand.
> 
> Agreed, matching names is one of the reasons I started this work.
> 
> My thought was that the prefixed names could be fixed eventually
> after the hand-rolled code has been replaced.
> 
> 
> > What might be best is to autogenerate a header from a (combined)
> > nfs4.x, and then have the old nfs4.h file include it. Then we'd
> > just
> > have to eliminate anything from nfs4.h that conflicts with the
> > autogenerated symbols.
> > 
> > That's definitely not treating include/linux/nfs4.h as immutable,
> > but
> > we might still be able to avoid a lot of changes to the client code
> > that way.
> 
> Include the current nfs4xdr_gen.h in nfs4.h, and then remove from
> nfs4.h anything that has a naming conflict?
> 
> The downside of that is that approach mixes generated and
> hand-rolled code. Not an objection from me, but it was something
> that made Neil uncomfortable.
> 

Not exactly. The hand-rolled code would become dependent on the
generated headers, but we shouldn't need make changes to the generated
headers themselves.

> 
> > We'd need Trond and Anna to buy in on that though.
> 
> Or the client can continue to use include/linux/nfs4.h and the
> server can start using its own copy of that header.
> 

Sure, that would work too.
-- 
Jeff Layton <jlayton@kernel.org>