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.

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

pkg/lwip / pkg/tinydtls: use new mbox_avail() / mbox_unset() function. #15484

Merged
merged 2 commits into from
Nov 20, 2020
Merged

pkg/lwip / pkg/tinydtls: use new mbox_avail() / mbox_unset() function. #15484

merged 2 commits into from
Nov 20, 2020

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 20, 2020 •
edited
Loading

Contribution description

Follow-up on #15478

Testing procedure

The following tests should still compile and run successfully:

  • tests/lwip
  • tests/lwip_sock_ip (both with LWIP_IPV4=1 and LWIP_IPV6=1 and their combination)
  • tests/lwip_sock_tcp (both with LWIP_IPV4=1 and LWIP_IPV6=1 and their combination)
  • tests/lwip_sock_udp (both with LWIP_IPV4=1 and LWIP_IPV6=1 and their combination)
  • tests/pkg_tinydtls_sock_async/

Issues/PRs references

Follow-up on #15478

@miri64 miri64 added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Nov 20, 2020
@miri64 miri64 requested a review from kaspar030 November 20, 2020 12:36
@miri64 miri64 mentioned this pull request Nov 20, 2020
Merged
@miri64 miri64 changed the title Mbox/enh/use new api pkg/lwip / pkg/tinydtls: use new mbox_avail() function. Nov 20, 2020
kaspar030
kaspar030 previously approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK. Straight forward replace of cib_avail(&mbox->cib) with mbox_avail(mbox)

@miri64 miri64 changed the title pkg/lwip / pkg/tinydtls: use new mbox_avail() function. pkg/lwip / pkg/tinydtls: use new mbox_avail() / mbox_unset() function. Nov 20, 2020
@miri64 miri64 dismissed kaspar030’s stale review November 20, 2020 12:43

Sorry, decided last minute to also create and use mbox_unset()

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 20, 2020
kaspar030
kaspar030 reviewed Nov 20, 2020
View reviewed changes
core/include/mbox.h Outdated
@@ -185,6 +185,17 @@ static inline size_t mbox_avail(mbox_t *mbox)
return cib_avail(&mbox->cib);
}

/**
* @brief Unset's the mbox, effectively deinitializing invalidating it.
Copy link
Contributor

Choose a reason for hiding this comment

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

both deinitializing and invalidating? Comma missing? :)

This leaks a possibly supplied queue. Maybe better call it mbox_release_queue?
The mbox actually works without queue, it'd just be synchronous from that point on.

This is also a bit dangerous, if this is called with messages in the queue, they get lost. If there's a sender that was previously waiting, it would be stuck there until the queue transitions from full to empty again.

Why is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I suggest splitting, the other changes were already ACKed...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this needed?

Scroll the code and you see, that lwIP requires a way to invalidate their mboxes and check the validiy.

RIOT/pkg/lwip/include/arch/sys_arch.h

Lines 101 to 111 in dd1de91

static inline bool sys_mbox_valid(sys_mbox_t *mbox)
{
return (mbox != NULL) && (mbox->mbox.cib.mask != 0);
}
static inline void sys_mbox_set_invalid(sys_mbox_t *mbox)
{
if (mbox != NULL) {
mbox->mbox.cib.mask = 0;
}
}

I agree that the current approach is not the correct one, maybe. Any idea how we should proceed with that?

(I suggest splitting, the other changes were already ACKed...)

Can do.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #15485

@miri64 miri64 mentioned this pull request Nov 20, 2020
Merged
@miri64 miri64 force-pushed the mbox/enh/use-new-api branch from 22ec43a to ef5e034 Compare November 20, 2020 14:10
@miri64
Copy link
Member Author

miri64 commented Nov 20, 2020

Reverted to ACK'd state, as suggested in #15484 (comment)

kaspar030
kaspar030 approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

re-ACK.

@miri64 miri64 merged commit d107822 into RIOT-OS:master Nov 20, 2020
@miri64 miri64 deleted the mbox/enh/use-new-api branch November 20, 2020 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri 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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

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