gnrc_sock_udp: add sendto

[smlng]

This PR started with the following error when calling make scan-build-analyze:

/RIOT/sys/net/gnrc/sock/udp/gnrc_sock_udp.c:252:10: warning: Dereference of null pointer
        (sock->remote.family != AF_UNSPEC)) {
         ^~~~~~~~~~~~~~~~~~~
1 warning generated.

Trying to resolve this issue I thought it might be nice to have an explicit sock_udp_sendto call, then than the rather complicated checking in sock_udp_send. This PR adds such function without breaking API and existing applications using it.

However, if approved I would like to remove the remote parameter from sock_udp_send and adapt applications to use sendto where required instead. Which means sock_udp_send would only be used for connected UDP socks and sendto otherwise.

[kaspar030]

Could you elaborate on why you think having another sendto call improves the API? Are you aware of the discussion in #5758?

[smlng]

As said above, the original send function tries to handle all possible cases with an abundance of if ... else if ... else statements, which make it hard to read or keep track if really every possbile side case is covered and checked for errors in the given parameters. And as the static analyzer revealed, in one case there was possible null pointer derefence.

Introducing a sendto with a clear semantik make it easier to understand the code and check for errors. In a further step, send could be simplied to be only used with connected socks getting rid of the remote parameter.

Regarding #5758: was there a discussion on sendto, haven't found anything?!

[miri64]

/RIOT/sys/net/gnrc/sock/udp/gnrc_sock_udp.c:252:10: warning: Dereference of null pointer
        (sock->remote.family != AF_UNSPEC)) {
         ^~~~~~~~~~~~~~~~~~~
1 warning generated.

That looks more like a bug in the sock application of GNRC (one that would be easily fixable with an easy (sock == NULL) || check...). Except for maybe simplifying the code a bit for the GNRC-specific implementation it would significantly change the API (which is why I changed the label from minor to major) and make it more complex, so I'm not convinced sock would benefit from a separate sendto.

[OlegHahm]

connected UDP socks

A connected connectionless transport?

[miri64]

#6203 offers a less intrusive fix for the error reported by the static code analyzer.

[smlng]

connected UDP socks

A connected connectionless transport?

@OlegHahm, maybe I should've put that in italics or something, however, isn't that how they call these in the BSD/POSIX socket API? You can run connect() on UDP sockets there, and afterwards you can use send() on UDP socket, otherwise you have to use sendto(...) with UDP sockets.

@miri64 for this PR sendto is an enhancement without breaking existing code, as send remains unchanged - I proposed to change the latter, if approved. But this may need more thinking ...

However, if I understood you correctly in #5758, you are argued in favor of a user/developer friendly API instead of aligning the API to the network stack; sendto has a clearer semantic than send - which is IMHO more user friendly. I also found the meeting minutes in the wiki where you renamed the sendto into send, but I cannot follow the reasoning.

Regarding #6203, my point here was not about providing a (minimal intrusive) fix by adding/checking another condition, but arguing in favor of a API semantics.

[miri64]

@miri64 for this PR sendto is an enhancement without breaking existing code, as send remains unchanged - I proposed to change the latter, if approved. But this may need more thinking ...

For this the PR changes a lot... Don't get me wrong, if you see a benefit in e.g. constraining the port range for an unbound sock, I happily take it in a separate PR, but I don't see any benefit in splitting up the funtcion and making the API more complicated.

However, if I understood you correctly in #5758, you are argued in favor of a user/developer friendly API instead of aligning the API to the network stack; sendto has a clearer semantic than send - which is IMHO more user friendly. I also found the meeting minutes in the wiki where you renamed the sendto into send, but I cannot follow the reasoning.

A very simple reason: you can use it as send with remote == NULL and a "connected" sock given, so we choose the shorter name to make it easier on the user. Same goes for recv/recvfrom btw.

Speaking of: how does it fit into your API proposal? And how does sock_ip's equivalents?

Regarding #6203, my point here was not about providing a (minimal intrusive) fix by adding/checking another condition, but arguing in favor of a API semantics.

See, this is where I'm massively confused, because the semantics of your sendto are already incorporated into the API (the signatures are even the same)... So why add another whole new function for it? If the API's names are the problem, why not have it (as a sketch; I don't actually want this API change).

int sendto(s, buf, remote);

static inline int send(s, buf)
{
    return sendto(s, buf, NULL);
}

If my implementation of the API for GNRC is the problem, we also could implement a "sendto" there and have it called when send() is called with remote != NULL.

After all the problem is: A change to sock does not just include GNRC, but also other stacks (both done and in the making), so we need to be very careful about changing it, even if it is just a name change.

[smlng]

@miri64: ah, I forgot about the port range stuff I added here as well. The idea behind this was to eliminate the need to call random and specify an allowed port range, excluding possible collisions, e.g., with typically, reserved system ports < 1000. Leaving send vs. sendto aside, would this be worth considering in a separate PR?

[OlegHahm]

Wait a second: do I understand it correctly that the current implementation does not chose the source port from the ephemeral ports? If the answer is yes, then this should clearly be changed in a separate PR.

[smlng]

@OlegHahm: nope random it is: see here.

In this PR I also introduced some helper functions to correct that, if there is no consensus regarding send vs. sendto. I will make a PR just for the port range stuff introduced here.

[kaspar030]

@miri64 for this PR sendto is an enhancement without breaking existing code, as send remains unchanged - I proposed to change the latter, if approved. But this may need more thinking ...

I don't see the extra function as an API enhancement.
GNRC's implementation does look a bit confusing, but I'm sure that can be fixed, if necessary.

However, if I understood you correctly in #5758, you are argued in favor of a user/developer friendly API instead of aligning the API to the network stack;

To which network stack?

The network stack side has to be used once for every stack, the application side has to be used once per application. So maybe currently the ease of use is on the wrong side, but that will change as soon as there's a second application actually utilizing sock.

sendto has a clearer semantic than send - which is IMHO more user friendly. I also found the meeting minutes in the wiki where you renamed the sendto into send, but I cannot follow the reasoning.

What is unclear about sock's current UDP send() semantics? Why is "sendto" clearer, more user friendly?

[smlng]

@kaspar030: its my personal, subjective opinion that a sendto semantically implies a mandatory remote (or whatever name it has) parameter, while I would not expect a send to have such anyway. That said, I understand your (and @miri64) concerns - so we may agree to disagree here in a way. But, as I haven't participated in the original discussion when this API calls were born, I'm fine with send as is.

However, as suggested here (and I think @OlegHahm agrees) the source port handling should be changed. I'll prepare a PR for that, and we may close this one afterwards ...

[miri64]

However, as suggested here (and I think @OlegHahm agrees) the source port handling should be changed. I'll prepare a PR for that, and we may close this one afterwards ...

*the source port handling for GNRC. ;-)

[smlng]

Closing, superseded by #6203 and #6212.