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

[Chuck Lever III <chuck.lever@oracle.com>]

> On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2024-08-19 at 13:21 +0000, Chuck Lever III wrote:
>> 
>>> On Aug 19, 2024, at 7:44 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> On Mon, 2024-08-19 at 10:04 +1000, NeilBrown wrote:
>>>> On Fri, 16 Aug 2024, Jeff Layton wrote:
>>>> 
>>>>> +// Generated by lkxdrgen, with hand-edits.
>>>> 
>>>> I *really* don't like having code in the kernel that is partly
>>>> tool-generated and partly human-generated, and where the boundary isn't
>>>> obvious (like separate files).
>>>> 
>>>> If we cannot use tool-generated code as-is, then let's fix the tool.
>>>> If we cannot fix the tool, then include the raw output and a
>>>> human-generated patch which the makefile combines.
>>>> 
>>>> Ideally the tool should be in tools/, the .x file should be in fs/nfsd/
>>>> and the makefile should apply the one to the other.  We are going to
>>>> want to do that eventually and I think it should be priority.  The tool
>>>> doesn't have to be bug-free before it lands (nothing is).
>>>> 
>>>> A particular reason for this is that I cannot review tool-generated
>>>> hand-editted code.  It is too noisy and I don't know which parts are
>>>> worth closer inspection etc.
>>> 
>>> Fair point. Chuck made some similar comments to me privately, and it
>>> looks like he has updated his xdrgen tool as well.
>>> 
>>> I'll plan to just respin that part from scratch and regenerate from the
>>> .x files. I'll also keep my hand-edits in a separate commit for the
>>> next version.
>>> 
>>> It'll probably take me a few days, but I'll try to have something to
>>> resend within the next week or so.
>> 
>> Meanwhile, I'll post the current xdrgen tool for review. It
>> already lives under tools/ as Neil suggested above.
>> 
>> I'm hoping that by providing the .x snippet used to generate the
>> source, and by adding one or two "pragma" annotations to the tool
>> to handle certain exceptions, no hand-edits or extra patching
>> will be needed.
>> 
>> 
> 
> 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.
> 
> 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.
> 
> These are minor and easily fixed, obviously, but it would be nice to
> not need to do that.

That might be a bug in the spec, actually. I'll have a look.

--
Chuck Lever