Skip to content

sock: amend API to iterate over stack-internal buffer chunks #13779

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 1, 2020
Merged

sock: amend API to iterate over stack-internal buffer chunks #13779

merged 4 commits into from
Apr 1, 2020

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Apr 1, 2020 •
edited
Loading

Contribution description

With lwIP we have a chunked UDP payload, so just providing the stack-internal buffer is not possible. To be able to iterate over such a chunked payload, this change allows the sock_*_recv_buf() functions to use the internal buffer context as an iteration state. Since the newly required handling of the context requires an iteration anyways, sock_recv_buf_free() becomes unnecessary.

As this function is still experimental (I piggy-backed the badge for that) this API change only should require 1 ACK.

Testing procedure

Read the doc and

for test in tests/gnrc_sock_{ip,udp}; do
    make -C ${test} flash test
done

should still pass.

Issues/PRs references

Needed for #13701.

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Apr 1, 2020
@miri64 miri64 added this to the Release 2020.04 milestone Apr 1, 2020
@miri64 miri64 requested a review from benpicco April 1, 2020 10:30
benpicco
benpicco previously approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API design looks sound to me.

@benpicco
Copy link
Contributor

benpicco commented Apr 1, 2020

Since GNRC does not chunk the UDP payload, an adaptation for this API change is not required.

That's not entirely true - it must now return 0 when called with *buf_ctx != NULL

@benpicco benpicco dismissed their stale review April 1, 2020 10:47

You should adapt the existing implementation to the API change.

@benpicco
Copy link
Contributor

benpicco commented Apr 1, 2020

I think the API would be easier to use if instead you introduce an additional bool *more parameter.
That way the user doesn't have to call the function twice to find out if there is more data.

@miri64
Copy link
Member Author

miri64 commented Apr 1, 2020

I think the API would be easier to use if instead you introduce an additional bool *more parameter.
That way the user doesn't have to call the function twice to find out if there is more data.

I'd like to keep the API minimal. The correct way to use it would be in a while-loop anyway.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Apr 1, 2020
benpicco
benpicco previously approved these changes Apr 1, 2020
View reviewed changes
@benpicco
Copy link
Contributor

benpicco commented Apr 1, 2020

Just noticed - you should probably also amend tests/gnrc_sock_udp/main.c

@miri64
Copy link
Member Author

miri64 commented Apr 1, 2020

I just had another idea 😅

@miri64 miri64 dismissed benpicco’s stale review April 1, 2020 11:33

To prevent accidental merges

@miri64
Copy link
Member Author

miri64 commented Apr 1, 2020

Ok, I restructured it a bit and now it somewhat addresses your comment about having to call this function more than once. This is still the case, however, the second call will release the internal context implicitly (what was required to before explicitly using sock_recv_buf_free(). So you still need an extra call, but do not require to call sock_recv_buf_free() in the end :-).

@benpicco
Copy link
Contributor

benpicco commented Apr 1, 2020

Nice. That 'forces' the user to always use the API properly and recuses the API surface.

I reckon there is no use for the content of buf_ctx outside that function?

@benpicco
Copy link
Contributor

benpicco commented Apr 1, 2020

(Murdock has found some issues)

@miri64
Copy link
Member Author

miri64 commented Apr 1, 2020

I reckon there is no use for the content of buf_ctx outside that function?

Mostly yes. For the lwIP implementation I use the struct netconn directly to determine the total length for -ENOBUFS, but that stays internal to the lwIP wrapper implementation.

@miri64
Copy link
Member Author

miri64 commented Apr 1, 2020

(Murdock has found some issues)

Huh? The change did not run yet.

@miri64 miri64 mentioned this pull request Apr 1, 2020
Merged
@benpicco
Copy link
Contributor

benpicco commented Apr 1, 2020

Oh sorry, I confused this with #13701

benpicco
benpicco reviewed Apr 1, 2020
View reviewed changes
@benpicco
Copy link
Contributor

benpicco commented Apr 1, 2020

This looks good now - please squash.

miri64 added 4 commits April 1, 2020 15:50
@miri64
sock: amend API to iterate over stack-internal chunks
f9e17ac 
With lwIP we have a chunked UDP payload, so just providing the
stack-internal buffer is not possible. To be able to iterate over such
a chunked payload, this change allows the `sock_*_recv_buf()` functions
to use the internal buffer context as an iteration state.

As the internal buffer space can be released when the function would
return 0, `sock_recv_buf_free()` becomes unnecessary.
@miri64
sock: sock_*_recv_buf: mark as experimental
58685af
@miri64
gnrc_sock: adapt for *_recv_buf() API change
d934579
@miri64
tests/gnrc_sock_{ip,udp}: adapt tests for sock_*_recv_buf API change
e4c4320
@miri64 miri64 force-pushed the sock/enh/sock_buf_recv-iterate-chunks branch from 32efaa2 to e4c4320 Compare April 1, 2020 13:51
@miri64
Copy link
Member Author

miri64 commented Apr 1, 2020

This looks good now - please squash.

Done. Also fixed some doc issues while I was at it ;-).

benpicco
benpicco approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API change makes sense and enforces the right behavior of the caller.

@benpicco benpicco added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Apr 1, 2020
@miri64 miri64 merged commit 76acde0 into RIOT-OS:master Apr 1, 2020
@miri64 miri64 deleted the sock/enh/sock_buf_recv-iterate-chunks branch April 1, 2020 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
2 participants
@miri64 @benpicco