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

[Jeff Layton <jlayton@kernel.org>]

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.

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.

We'd need Trond and Anna to buy in on that though.

> 
> > Also, as a side note:
> > 
> > fs/nfsd/nfs4xdr.c: In function
> > ‘nfsd4_encode_fattr4_open_arguments’:
> > fs/nfsd/nfs4xdr.c:3446:55: error: incompatible type for argument 2
> > of ‘xdrgen_encode_fattr4_open_arguments’
> >  3446 |         if (!xdrgen_encode_fattr4_open_arguments(xdr,
> > &nfsd_open_arguments))
> > 
> > 
> > OPEN_ARGUMENTS4 is a large structure with 5 different bitmaps in
> > it. We
> > probably don't want to pass that by value. When the tool is dealing
> > with a struct, we should have it generate functions that take a
> > pointer
> > instead (IMO).
> 
> The decoders are passed structs by reference already, fwiw. I had
> been considering the same for the encoder functions, for efficiency.
> I can give it a try.
> 

Sounds good.
-- 
Jeff Layton <jlayton@kernel.org>