Message-ID: <5DFBDAE5-9272-4335-95FD-AD4F00326E43@oracle.com>
Date: Thu, 29 Aug 2024 19:02:19 +0000
From: Chuck Lever III <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>,
        Al 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 Mailing List
	<linux-kernel@...r.kernel.org>,
        Linux NFS Mailing List
	<linux-nfs@...r.kernel.org>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        "linux-doc@...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 Aug 29, 2024, at 2:26 PM, Jeff Layton <jlayton@...nel.org> wrote:
> 
> 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:
>> 
>>> 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?

IMHO it does. Client and server are rather separate
code bases, and when we have attempted to share things
extensively between them in the past, it locks the other
side into something that doesn't fit well.

I'd rather be further along with client side code
generation before making decisions about how to fit
these pieces together. What I've toyed with so far
suggests there are going to be a few substantive
differences with the server side.


> 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:

The tool already does that now:

  xdrgen header --definitions

gives you the protocol pieces, and

  xdrgen header --declarations

gives you the function declarations. Specify both
together and you get what you have in nfs4xdr_gen.h
today. It might not be the most natural way to split
these up, but <shrug>.

(Note that this is going to get worse when Rust is
introduced into the mix).


> 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

Right, that's already the direction I'm going with
xdrgen. I think we're on the same page with that.


> 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.

That should work.


>>> 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.

It does only if we move the header file to
include/linux/sunrpc/xdrgen. I'm not sold on that idea yet
though I guess it works for the protocol pieces that the
Linux community doesn't directly control (ie --definitions).


> This naming also makes it clearer that it's generated from
> nfs4_1.x.

Well the generated boilerplate could contain the spec file
name -- it has the spec file's timestamp already as a kind
of janky version number. Commit hash might be better.

Note that the spec authors expect implementations to catenate
all the disparate .x pieces (ie, all the minor versions and
all the pNFS layout types) together into a single all-singing-
all-dancing nfs4.x. Looking at it now, I'm not convinced we
want to stay with the name nfs4_1.x. Also that's why I used
the output file name nfs4xdr_gen.[ch].


> That said, I'm not too particular here. Can you lay out
> how you think we ought to arrange things?

I'm still musing about it. I'll mock something up in the
2/2 xdrgen patch for more feedback.

Now that we've got most of the review of LOCALIO done, I'll
have a bit more time to help you get xdrgen ready for server-
side delstid. I will try not to hold you up.


--
Chuck Lever

Powered by blists - more mailing lists