Page MenuHomeFreeBSD

Move inner loop logic out of sysctl_sysctl_next_ls()
ClosedPublic

Authored by melifaro on Oct 30 2020, 9:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 6:10 AM
Unknown Object (File)
Fri, Mar 22, 6:06 AM
Unknown Object (File)
Fri, Mar 22, 6:06 AM
Unknown Object (File)
Fri, Mar 22, 6:06 AM
Unknown Object (File)
Fri, Mar 22, 6:06 AM
Unknown Object (File)
Fri, Mar 22, 6:06 AM
Unknown Object (File)
Fri, Mar 22, 6:02 AM
Unknown Object (File)
Fri, Mar 22, 6:02 AM
Subscribers

Details

Summary

Refactor sysctl_sysctl_next_ls().

  • Move huge inner loop out of sysctl_sysctl_next_ls() into a separate non-recursive function, returning the next step to be taken.
  • Update resulting node oid parts only on successful lookup
  • Make sysctl_sysctl_next_ls() return boolean success/failure instead of errno, slightly simplifying logic
Test Plan
7:40 [1] m@devel0 sysctl -aN | wc -l
   13333
7:41 [1] m@devel0 wc -l ~/devel0_stock.txt
   13333 /home/melifaro/devel0_stock.txt

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 34793
Build 31842: arc lint + arc unit

Event Timeline

melifaro edited the test plan for this revision. (Show Details)

I like that deciding what action to take at the current node is able to be split out into its own function. I don't find sysctl_sysctl_next_node to be a properly descriptive name. It kept confusing me that the "next node" function doesn't do what it sounds like. Changing that to something like sysctl_sysctl_next_action and using an enum for the different actions would avoid the confusion. The variable its return value is stored in could be called action instead of ret. Consider for example enum { ITER_STOP, ITER_SIBLINGS, ITER_CHILDREN };.

sys/kern/kern_sysctl.c
1244

You could return (success); to shave off the extra return above.

Address freqlabs comments, rebase to latest HEAD.

I like that deciding what action to take at the current node is able to be split out into its own function. I don't find sysctl_sysctl_next_node to be a properly descriptive name. It kept confusing me that the "next node" function doesn't do what it sounds like. Changing that to something like sysctl_sysctl_next_action and using an enum for the different actions would avoid the confusion. The variable its return value is stored in could be called action instead of ret. Consider for example enum { ITER_STOP, ITER_SIBLINGS, ITER_CHILDREN };.

Thank you for the suggestion, that's a good one!
Does the current code look better?

sys/kern/kern_sysctl.c
1205

ITER_SIBLINGS would be a better fit than ITER_STOP. We aren't stopping iteration, we're continuing to iterate through the siblings of this node.

1210

Worth an assert IMO.

sys/kern/kern_sysctl.c
1104

Update ITER_SIBLINGS comment.

This revision is now accepted and ready to land.Nov 16 2020, 10:10 PM
freqlabs added inline comments.
sys/kern/kern_sysctl.c
1182

Actually I think this should be ITER_SIBLINGS, and below as well, to match the continues that were previously here.

This revision now requires changes to proceed.Nov 16 2020, 10:13 PM
sys/kern/kern_sysctl.c
1180

In other words, we have a match at the current level but not a complete match.

Update to address freqlabs comments.

This revision is now accepted and ready to land.Nov 30 2020, 10:42 AM
This revision was automatically updated to reflect the committed changes.
melifaro marked 2 inline comments as done.