Details

Reviewers

freqlabs
imp

Commits

rS368199: Move inner loop logic out of sysctl_sysctl_next_ls().

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 created this revision.Oct 30 2020, 9:21 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 30 2020, 9:22 PM

melifaro requested review of this revision.Oct 30 2020, 9:22 PM

Harbormaster completed remote builds in B34521: Diff 78993.Oct 30 2020, 9:22 PM

Update.

Harbormaster completed remote builds in B34522: Diff 78995.Oct 30 2020, 10:01 PM

update,

Harbormaster completed remote builds in B34523: Diff 78996.Oct 30 2020, 10:31 PM

update wording.

Harbormaster completed remote builds in B34524: Diff 78997.Oct 30 2020, 10:37 PM

update.

Harbormaster completed remote builds in B34525: Diff 78998.Oct 30 2020, 11:03 PM

melifaro edited the summary of this revision. (Show Details)Oct 30 2020, 11:10 PM

melifaro added reviewers: freqlabs, imp.

asiciliano added a subscriber: asiciliano.Oct 31 2020, 2:50 AM

Further simplify.

Harbormaster completed remote builds in B34528: Diff 79003.Oct 31 2020, 7:35 AM

melifaro edited the summary of this revision. (Show Details)Oct 31 2020, 7:41 AM

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

melifaro added a child revision: D27035: Add versioning support for kernel<>userland sysctl interface.Nov 1 2020, 11:28 AM

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.

Harbormaster completed remote builds in B34793: Diff 79531.Nov 13 2020, 11:55 PM

In D27029#604261, @freqlabs wrote:

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?

freqlabs added inline comments.Nov 16 2020, 9:42 PM

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.