Re: [PATCH 1/3] nfsd: bring in support for delstid draft XDR encoding

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-08-19 at 23:16 +0000, Chuck Lever III wrote:
> 
> > On Aug 19, 2024, at 4:04 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Mon, 2024-08-19 at 19:50 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@kernel.org>
> > > > wrote:
> > > > 
> > > > I'm playing with the new version now and it seems to be much
> > > > improved.
> > > > Only two real bugs I've hit at this point:
> > > > 
> > > > 1/ Some of the struct specifications need to be typedefs as well.
> > > > For
> > > > instance, the delstid draft refers to "nfstime4", but the
> > > > autogenerated
> > > > struct definition doesn't have the typedef for it. It may be best
> > > > to
> > > > just add typedefs for all of these sorts of structs.
> > > 
> > > What's the specific symptom? I've been able to catenate nfs4_1.x
> > > and delstid.x, xdrgen builds the header and source without tossing
> > > any exceptions, and gcc compiles it without complaint.
> > > 
> > 
> > 
> > Basically, I was getting this when I'd convert nfs4_1.x to a header:
> > 
> > struct nfstime4 {
> >        int64_t seconds;
> >        uint32_t nseconds;
> > };
> > 
> > ...but the delstid header has these:
> > 
> > typedef nfstime4 fattr4_time_deleg_access;
> > 
> > typedef nfstime4 fattr4_time_deleg_modify;
> > 
> > 
> > ...nothing defined nfstime4 in this case.
> > 
> > > AFAICT, xdrgen will add "struct" where it's necessary.
> > > 
> > > I've been squirrelly about using "typedef" too often because
> > > the Linux kernel's coding style is to avoid C typedefs for
> > > shorthand structure names.
> > > 
> > 
> > Oh, ok. I didn't concatenate the files like you did and just generated
> > the delstid files separately from the nfs4_1 ones. I guess that throws
> > off the dependency tracking that you're doing here for typedefs.
> 
> cat'ing the two files together is the spec-recommended approach,
> but it assumes you're generating the whole protocol at once.
> Here it was just a quick and dirty way for me to build a
> reproducer.
> 
> For an initial fs/nfsd/nfs4_1.x file, I recommend starting with
> delstid.x, and then add the pieces of the NFSv4_1 XDR until
> xdrgen and gcc can make proper sense of it.
> 
> I can take a stab at that if you like, and send you something
> tomorrow?
> 

I think that will probably fix the problem I was having before. I'll
respin that part of the set soon. It's probably better that I do it so
I figure this out. This is the "easy" XDR vs. CB_NOTIFY.

> 
> Sidebar: We could go with all typedefs for structs, unions, and
> enums. That would make C code generation easier. Something like:
> 
> typedef struct {
> 	int64_t seconds;
> 	uint32_t nseconds;
> } nfstime4;
> 
> But like I said, I expect that approach might be frowned upon.
> 

Agreed. I don't think it's needed. I was just using the tool wrong
before.

Thanks,

> 
> > > > 2/ xdrgen_encode_nfstime4 want a pointer to the nfstime4, but the
> > > > autogenerated code for xdrgen_encode_fattr4_time_deleg_access and
> > > > xdrgen_encode_fattr4_time_deleg_modify try to pass it by value
> > > > instead.
> > > 
> > > Here's my generated copy of xdrgen_encode_fattr_time_deleg_access:
> > > 
> > > /* typedef fattr4_time_deleg_access */
> > > static bool
> > > __maybe_unused                                                     
> > > xdrgen_encode_fattr4_time_deleg_access(struct xdr_stream *xdr, const
> > > fattr4_time_deleg_access value)
> > > {
> > > /* (basic) */
> > > return xdrgen_encode_nfstime4(xdr, &value);
> > > };
> > > 
> > > Looks like it does the right thing...?
> > 
> > Probably another side-effect of it not knowing what to do with nfstime4
> > when I convert the delstid draft. Concatenating them seems unwieldy but
> > I guess that would work. I do like being able to keep generated code
> > from different files separate though.
> 
> I don't think cat'ing the .x files is /required/, but it was a
> quick way to get started.
> 
> Having a working nfs4_1.x that can generate the small piece of
> XDR code that we need, in a separate file that can be augmented
> over time, I think, is a win. I don't see that anything so far
> is preventing that.
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>