Refactor makesyscalls.lua

[agge3]

Refactor of makesyscalls.lua to be a library
Drop-in replacement to makesyscalls.lua

Google Summer of Code 2024 Final Work Product
Project description: Refactor Syscall Creation Script
Mentors: Warner Losh (bsdimp), Kyle Evans (kevans)

Sponsored by: Google (GSoC 2024)

[bsdimp]

Adding @jrtc27 and @arichardson since I can't do that in the reviewers setting....

Note to reviewers: This is my GSoC student's libification of makesyscalls.lua. At this stage, it's a drop in replacement. I'll work with my student on the reviews / commits aspect of the review. I'm looking for reviews on the final product. It should be 'no diffs' or nearly so (I think there's extra comments in a few places still, but I've not checked this final version). I'll also focus initially on the no-diff aspect, and hope the other reviewers have time to review this as to structure, lua goodness, etc.

It specifically doesn't have anything relating to reducing the number of files we have to check in as we can generate them on the fly now since (a) make is better able to handle that (b) machines are faster than in the 90s and (c) dependency madness has receeded since the 90s for many (but not all) of these files. These issues will be worked out (and the details and extent litigated) in phase 2.

Also, lsg was my first, half-completed attempt at this. He finished it off and got it to this point. Yea!

[agge3]

Yes. Thank you Warner! I'm excited to get this where it needs to be. I'll appreciate any time and effort spent.

There are bits and pieces that are still being iterated on, but the guts are there. Files are being generated (mostly) similar.

-Tyler

[brooksdavis]

This is quite hard to review in any kind of meaningful way. There needs to be some sort of overview of the structure in a README.md as well as the commit message.

What I'll want to do before a merge is to take something closer to final and bring it over to CheriBSD so I can apply our local diffs and see how that works in practice.

Some comments:

• Despite all the indentation commits (that need to be squashed), indentation is wildly inconsistent even within files.
• This is missing some commits to makesyscalls.lua (FREEBSD14 is the canary, but everything since the original split needs a review and merge).
• If there are any differences in generated output they need to be very well documented. The desired number is zero.
• The code might benefit from some subdirectories to differentiate code that generates specific files from library code.
• There should probably be three total commits: add new code, switch what the Makefile uses, remove old code.

[kevans91]

Initial pass with some feedback; apologies for largely being a ghost this term. I'll try to scrape some time together in the next week or so to do a more thorough review.

[kevans91]

Let's just consolidate the comment on this line up with that above the function. Maybe:

Can also specify which order (level) to store in, which defaults to one if not provided.

[agge3]

There are other comments that could use a similar suggestion. I've changed those to match this suggestion (and this one). Please inform me if you see anything else.

[jlduran]

It would be good to first address luacheck's suggestions:

luacheck sys/tools/syscalls

[bsdimp]

I've noticed 2 real differences so far (the other differences are comments or sorting deltas):

We have #define nosys linux_nosys added to the linux files. what is this done? It doesn't seem to be needed.
The return type of linux_exit changes from int to void, which is incorrect.

There may be other issues as well.

[brooksdavis]

I like the code reorganization. Much improved!

My main gripe: pick an indent style and use it consistently. I personally prefer a tabbed style following our C practices, but 4-spaces would be fine.

[brooksdavis]

Support for capabilities.conf should be dropped as the last consumer went away with be67ea4. It should provably not be removed as part of the refactor, but done on one side or the other.

[agge3]

I was unsure of this. We'll be able to easily approach this after the refactor, in a separate PR. This is an easy change to make.

[brooksdavis]

more spaces. There are several more instances in this file and presumably others, but I'm going to stop commenting here.

[agge3]

In regards to spaces that should be tabs. I've changed all of them to tabs--but programmatically. If I catch any (which I haven't) I will continue to change. This should be done.

[brooksdavis]

I'm not sure what this sentence is trying to say.

[agge3]

I've reworded. Please let me know if it is still unclear.

[agge3]

It would be good to first address luacheck's suggestions:

luacheck sys/tools/syscalls

This has been followed up on. All luacheck suggestions are addressed. Thanks for the suggestion! This will be the precedent moving forward.

[agge3]

I like the code reorganization. Much improved!

My main gripe: pick an indent style and use it consistently. I personally prefer a tabbed style following our C practices, but 4-spaces would be fine.

Great!! I agree with this sentiment. I will be following your preference. I did not catch that (also the source of why there's differences, as code from makesyscalls.lua followed FreeBSD C practices). It's going to be something I do as I correct the diffs, so it probably won't be changed right away, but it should follow a consistent indentation when it's ready.

[brooksdavis]

I've updated my branch with a bunch of whitespace fixes and a few fixups of my prior commits. Except for the extra file generation bits the commits on my branch can be seen as reviews in commit form and squashed directly into your commit without credit. The other two add enough content that a Co-authored-by: credit might be nice if squashed.

https://github.com/brooksdavis/freebsd/tree/refactor-makesyscalls.lua

[brooksdavis]

I've added a proposed rebasing and squashing of the new code with follow on commits to connect the new code at https://github.com/brooksdavis/freebsd/tree/refactor-makesyscalls.lua-rebased. Happy to take any feedback about the organization or contents.

If it looks good, I'll plan to push to FreeBSD once the current stabilization week ends.

[agge3]

I've added a proposed rebasing and squashing of the new code with follow on commits to connect the new code at https://github.com/brooksdavis/freebsd/tree/refactor-makesyscalls.lua-rebased. Happy to take any feedback about the organization or contents.

If it looks good, I'll plan to push to FreeBSD once the current stabilization week ends.

Sorry I didn't follow-up more properly on your previous work. Last week, and this week, are very busy for me. I'll be less busy after this week.

This is great!!! Thank you for your help on the final stretch! I appreciate the work!! I'm glad you are pleased with the library. I'm happy to contribute and I really value the effort you've put into this.

I've merged 505d596 from that branch to be part of this commit.

On my part, I've went through everything thoroughly. All changes are minor or non-invasive. Some final changes that I'd like to add (currently squashed into this commit):

• Commit message includes Kyle as co-author. Him and Warner were both my mentors during GSoC, so that seems fitting with Warner as a co-author.
• Commit message includes a Signed-off-by agge3 as FreeBSD GitHub documentation instructs.
• Removed syscall:comment() as it's broken/unimplemented and there's no longer a use for it.
• Removed other XXX remarks that are implementation relics and should not be included.
• Added syscall:validate() to both branches of FreeBSDSyscall:parseSysfile(). That's correct and numbers should be validated in either case.
• audit_idx constant in sysproto_h is now just a decimal number. Following the goal that the refactor should be more readable, hexadecimal seems unfitting here.
• syscall_mk has config as a parameter once again, the same as you've done with libsys.h and syscalls.map. Luacheck complains about unused parameter. Having config available in every module is more intuitive and semantically makes sense.
• enforced style.lua(9) on outside credited functions, for consistency.
• Punctuation (periods) were inconsistent in comments; comment punctuation is now consistent.
• Semantics in comments are more consistent. I opted for Doxygen-style comments in some places, without a strict enforcement everywhere. I see that you continued the Doxygen-style. I've made comments more consistent, but this divide still exists.

Additionally, I noticed that "converstion" in systrace generation is misspelled. I've changed to be spelled correctly. Considering how minor, now seems like a good time to change that. Pending feedback, I've left that as a second commit on this PR (which can be squashed).

On the weekend (before next week), I'm hoping to go through again. But I'm happy with this being my final input.

In terms of this PR, would you like me to pull your commits over, or would you prefer I leave it how it is?

[brooksdavis]

I've rebased my branch with your changes (including the typo fix squashed into the main commit). I also fixed the typo upstream to reduce diffs in generated files.

I've added commits removing capabilities.conf and capenabled= support. I think it's ok to commit with the support and them immediately remove it on the grounds that someone might be using them downstream and this would make it easier to restore that functionality temporarily.

I'm happy to do another update Monday if you want to take another pass through the code over the weekend. It's probably somewhat easier if you don't incorporate my changes, but if there are any you want to take as part of the main commit, go ahead.

[agge3]

I've rebased my branch with your changes (including the typo fix squashed into the main commit). I also fixed the typo upstream to reduce diffs in generated files.

I've added commits removing capabilities.conf and capenabled= support. I think it's ok to commit with the support and them immediately remove it on the grounds that someone might be using them downstream and this would make it easier to restore that functionality temporarily.

I'm happy to do another update Monday if you want to take another pass through the code over the weekend. It's probably somewhat easier if you don't incorporate my changes, but if there are any you want to take as part of the main commit, go ahead.

Great. Sounds good!

Yes, I'll take another pass over the weekend.

I'll report any changes I have (if any at all), so everything's ready for Monday.

In that case, that will be a bit easier. That's probably what I'll opt for.

[agge3]

Okay, great!

I've went through everything thoroughly again.

Final changes I have are:

• More minor consistency changes in comments, as a result of going through it consecutively.
• (Notable) config.merge() claimed and was intended to return nil and a message if file was nil. It did not. It now does, and as a happy side-effect is less pushed over to the right side of the terminal.

I've regression tested locally to confirm everything's working as it should. If you're ready to commit, I don't see more areas I would like to address before that.

I've opted for keeping the PR as the singular commit that is mine. I appreciate the help and that makes the most sense. It is a lot easier, so thanks!

Thank you for the thorough review!

I'd like to stay in the loop and continue to maintain (and iterate, if necessary) areas revolving around this library. I'd like to consider myself knowledgeable in the library and would be excited to continue.

• Tyler

[brooksdavis]

Merged as commit 9ded074