Skip to content
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

sys/net/sock_udp: Add missing const qualifier #20863

Closed

Conversation

maribu
Copy link
Member

@maribu maribu commented Sep 17, 2024

Contribution description

The APIs sock_udp_get_local() and sock_udp_get_remote() are expected to be simple getters that do not modify the socket in any way. Hence, the should use cost sock_udp_t * rather than sock_udp_t * as first argument.

This fixes the issue.

Note: Since C callers will not be affected by this change, I will not add the API change label.

Testing procedure

The CI should be green.

Issues/PRs references

None

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 17, 2024
@github-actions github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System labels Sep 17, 2024
@riot-ci
Copy link

riot-ci commented Sep 17, 2024

Murdock results

✔️ PASSED

a74c7df sys/net/sock_udp: Add missing const qualifier

Success Failures Total Runtime
10197 0 10197 19m:55s

Artifacts

@maribu maribu requested review from kfessel and fabian18 September 17, 2024 11:36
The APIs `sock_udp_get_local()` and `sock_udp_get_remote()` are expected
to be simple getters that do not modify the socket in any way. Hence,
the should use `cost sock_udp_t *` rather than `sock_udp_t *` as first
argument.

This fixes the issue.
@maribu maribu force-pushed the sys/net/sock_udp/add-missing-const branch from 08cc31e to a74c7df Compare September 17, 2024 11:38
@miri64
Copy link
Member

miri64 commented Sep 17, 2024

Are you sure that a change (even if small) to a major front-facing API can be classified as "minor impact"?

@miri64
Copy link
Member

miri64 commented Sep 17, 2024

Also: why only change sock_udp? What about sock_tcp, sock_ip, and sock_dtls?

@maribu
Copy link
Member Author

maribu commented Sep 17, 2024

Are you sure that a change (even if small) to a major front-facing API can be classified as "minor impact"?

Yes, as C callers will not be affected. (Converting a foo * to const foo * works implicitly, as not writing to rw memory is not an issue. Dropping const would be an issue, as converting const foo * to foo * will not work implicitly, as writing to ro memory is an issue.)

Also: why only change sock_udp? What about sock_tcp, sock_ip, and sock_dtls?

I can take a look if similar issues are there as well. But let's fix one bug at a time.

@miri64
Copy link
Member

miri64 commented Sep 17, 2024

Also: why only change sock_udp? What about sock_tcp, sock_ip, and sock_dtls?

I can take a look if similar issues are there as well. But let's fix one bug at a time.

To me its the same API, so IMHO it should be fixed in one go... But sock_udp seems to be the preferred API to work on anyways, so 🤷‍♀️.

@maribu
Copy link
Member Author

maribu commented Sep 17, 2024

For TCP making the sock_tcp_t * parameter const is not possible with the GNRC implementation, as sock_tcp_get_local() does indeed write to the socket (by locking a mutex allocated in the socket).

I'm closing this for now.

@maribu maribu closed this Sep 17, 2024
@maribu
Copy link
Member Author

maribu commented Dec 2, 2024

Revisiting this: I believe it is justified to have the TCP socket write-able: The connection sockets are not owned by the user of the API, but by the listen/queue socket that will freely reuse them when new connections get accepted. The mutex does only solve a data race, though: E.g. code may still assume that a socket is still belonging to connection A when it is calling sock_tcp_get_local_address(), but in fact the socket may already be reused for connection B. But there is only so much one can do with mutexes.

For UDP, the story is different. A user of the API will create the socket, and it can be assumed that the endpoint will not be changed while the caller is still owning the socket. I assume the same to be true for DTLS.

I suggest that I change the parameter to const therefore for UDP and DTLS only, and not for TCP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants