> On Aug 19, 2024, at 9:26 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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
