Page MenuHomeFreeBSD

Move inner loop logic out of sysctl_sysctl_next_ls()
Needs RevisionPublic

Authored by melifaro on Fri, Oct 30, 9:21 PM.

Details

Reviewers
freqlabs
imp
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
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 34838
Build 31868: 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
1243

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–1211

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.

1209

Worth an assert IMO.

sys/kern/kern_sysctl.c
1103

Update ITER_SIBLINGS comment.

This revision is now accepted and ready to land.Mon, Nov 16, 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.Mon, Nov 16, 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.