mbox: provide function to unset initialized mbox

[miri64]

Contribution description

Split out of #15484, as suggested in #15484 (comment).

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)

Issues/PRs references

Split out of #15484

[miri64]

From #15484 (comment)

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.

The sys_mbox_set_invalid() function is called whenever a netconn (lwIP's sock equivalent) is closed or finished, so this isn't a risk, but maybe that function should be fixed in the way that readers and writers are also released and removed somehow.

[kaspar030]

So I still see issues:

• as is, this is not thread safe, something could send/rx a message between those two lines. I think mbox breaks if the array is NULL but cib mask not. (this could be mitigated by changing the order, but that seems hacky).
• any waiters on that mbox would be blocked indefinitely.
  I don't see any nice way around this. even while (mbox_try_ receive(tmp) {} would be racey.

I'm fine adding this function (if needed), but maybe those should be fixed or mentioned?

[benpicco]

Please squash, changes are minimal and make sense.

[miri64]

I squashed, but have you read my own worries in #15485 (comment)?

[benpicco]

Hm it's a strange low-level function, if it's useful there is no harm in adding it.
If you doubt it's usefulness, we shouldn't add it.

Maybe you want something that can take a callback function and iterates over the mbox, calling each element with that function if it's not NULL?

But if it's not currently useful that way, we shouldn't merge a bad API.

open for discussion

[miri64]

It's not really about usefulness, but about exposing such a feature in the first place.

[miri64]

Useful proper encapsulation is always ;-)

[miri64]

Ping @kaspar030?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[miri64]

Ping @kaspar030 the second ;-)

[miri64]

@kaspar030 do you have something like this on your radar for the rework?

[kaspar030]

ACK.

[kaspar030]

@kaspar030 do you have something like this on your radar for the rework?

rework postponed ...

[OlegHahm]

This PR seems mostly ready to go if I'm not mistaken!?

[Teufelchen1]

@miri64 ping

[miri64]

@miri64 ping

I agree with @OlegHahm. What do I need to do, to get this in? @kaspar030 approved this, there seems to be no merge conflict, I guess all this needs is a second ACK due to the (inconsequential) core change?

[Teufelchen1]

LGTM

[miri64]

Just to be sure, I ran the testing procedures I provided on native and samr21-xpro (where possible) rebased on top of current master and they are still succeeding.

BOARD=native make -C tests/pkg/lwip -j flash test
for ipv4 in 0 1; do
    for ipv6 in 0 1; do
        for board in native samr21-xpro; do
            for app in lwip_sock_ip lwip_sock_tcp lwip_sock_udp; do
                if [ $ipv4 == 0 ] && [ $ipv6 == 0 ]; then
                    continue
                fi
                BOARD=$board LWIP_IPV4=$ipv4 LWIP_IPV6=$ipv6 make -C tests/pkg/${app} -j flash test
            done
        done
    done
done

[miri64]

I think I need to rebase here, too, just for the Github Actions to succeeds though :-/

[miri64]

$ git range-diff upstream/master e22e1df 6229fda
1:  0334583749 = 1:  12194ad9e6 mbox: provide function to unset initialized mbox
2:  e22e1dfecf = 2:  6229fda7bd lwip: use new mbox_unset() function

;-) so no changes to the commits themselves ;-)

[riot-ci]

Murdock results

✔️ PASSED

6229fda lwip: use new mbox_unset() function

Success	Failures	Total	Runtime
10009	0	10009	07m:43s

Artifacts