-
Notifications
You must be signed in to change notification settings - Fork 3k
[ONME-2844] Avoid option level collisions #3458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@hasnainvirk Can you please look at continuous-integration/jenkins/pr-head why it fails? |
The original motivation was to allow users to dynamically configure network stacks the same way they can configure network sockets. Unlike most socket APIs we provide an abstract nework stack, so there needs to be a different route for stack specific options. Linux has the Most stack configuration is currently static at compile time to take advantage of memory savings, so the Tangentially, this pr seems like a good idea. Maybe it'd be better to use an offset of 3000 to match the error codes? |
Hmm, I was totally unaware of setstackopt - never noticed it, or had forgotten. Seems like a reasonable concept. Nanostack has a number of run-time config options. So if NSAPI_SOCKET and NSAPI_STACK aren't two values for setsockopt (as I assumed) they aren't necessarily part of the same number space - they could possibly both be the same value. I note the comments next to the option enums all say "stack options" rather than "socket options" - needs a little fixing and clarifying I think. |
@0xc0170 It isn't failing anymore. |
Kicking off the bot and requesting reviews! /morph test-nightly |
Was my original suggestion, so I'm fine with the concept, but I think we do need to fix the comments up a bit. NSAPI_STACK isn't doing anything at the moment as it's a "standardised level" for getstackopt without any actual options defined, which caused my confusion. Anyone got a suggestion of some standardised stack option they might want so we can put it in as a placeholder in a new stack options enum? "IP reassembly size" might be topical. (Nanostack's |
@hasnainvirk, sorry for adding work to your pr, but would it be too difficult to change the "stack options" to "socket options" in the neighboring lines? We can probably hold off on adding stack options until they are added to an implementation. Maybe the mru setting should be added to the NanostackEthernetInterface? The flag could be added to the option enum in the same pr. |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
To allow a network stack to support both NSAPI and its own options, try to make sure the NSAPI levels don't collide with level numbers likely to be used by network stacks. Distinguish between socket and stack options, and tighten up documentation. Add IP MRU stack options as an example (implementation not immediately planned for any stack, but could be useful).
16d5c10
to
fcd55fe
Compare
@geky can you be please kind enough to review this PR again ? |
This looks reasonable to me 👍 |
/morph test-nightly |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
@kjbracey-arm @c1728p9 Happy with this patch? |
Looks good to me 👍 |
To allow a network stack to support both NSAPI and its own options, try to make
sure the NSAPI levels don't collide with level numbers likely to be used by
network stacks.
Distinguish between socket and stack options, and tighten up documentation. Add
IP MRU stack options as an example (implementation not immediately planned for
any stack, but could be useful).