sock: change "no timeout" value from 0 to UINT32_MAX #5929

kaspar030 · 2016-10-11T21:25:33Z

IMHO specifying a timeout of "0" for "don't wait at all" makes much more sense.

OlegHahm · 2016-10-12T07:25:58Z

I must confess to know next to nothing about sock, but this change makes sense to me.

miri64 · 2016-10-12T07:39:49Z

What does 0 then do?

OlegHahm · 2016-10-12T07:41:33Z

IMHO specifying a timeout of "0" for "don't wait at all" makes much more sense.

OlegHahm · 2016-10-12T07:42:30Z

I.e. return immediately - in any case.

miri64 · 2016-10-12T07:44:55Z

IMHO specifying a timeout of "0" for "don't wait at all" makes much more sense.

I should read more...

Nevertheless, I still don't think it's a good idea. 0 means to me no timeout at all, not 0 sec timeout. Also, though my assembly is quite rusty, I think if (t == 0) is translated into fewer instructions than if (t < UINT_MAX32).

miri64 · 2016-10-12T07:46:05Z

I.e. return immediately - in any case.

I wouldn't know how to implement such a case with stacks where 0 means no timeout... I would set it to one, maybe...

miri64 · 2016-10-12T07:58:13Z

sys/include/net/sock/ip.h

- *                      @ref sys_xtimer module is not present or the
- *                      implementation does not support timeouts on its own.
- *                      May be 0 for no timeout.
+ *                      If 0 and no data is available, return immediately.


You mean "the operation is non-blocking"? "return immediately" sounds more like implementation-speak...

Yes, better. I'll change if you approve the change in general. (edit before here was a reply to another comment)

Yes, better

Then this is null and void:

I wouldn't know how to implement such a case with stacks where 0 means no timeout... I would set it to one, maybe...

Then please add the return value -EWOULDBLOCK or -EAGAIN.

I'll change if you approve the change in general.

I still see some implementation confusions, but I guess that is the reviewers job to spot these ;-)

kaspar030 · 2016-10-12T08:30:25Z

0 means to me no timeout at all, not 0 sec timeout.

If you read the parameter as "how much time to wait at most until data arrives", "0" meaning "don't wait at all" is quite natural, don't you think?

Also, though my assembly is quite rusty, I think if (t == 0) is translated into fewer instructions than if (t < UINT_MAX32).

You might save one instruction...

kaspar030 · 2016-10-12T08:35:01Z

I wouldn't know how to implement such a case with stacks where 0 means no timeout... I would set it to one, maybe...

Just add timeout += 1; at a strategically useful location. ;)

miri64 · 2016-10-12T10:16:23Z

Marginally related: do you remember, why we decided against a configurable timeout for sock_accept()? It's basically the receive function of a sock_tcp_queue_t.

kaspar030 · 2016-10-12T10:20:03Z

Marginally related: do you remember, why we decided against a configurable timeout for sock_accept()? It's basically the receive function of a sock_tcp_queue_t.

Actually, no, don't even remember discussing it. Seems to be missing.

miri64 · 2016-10-12T10:20:38Z

I have a slither of a memory, that Simon brought it up...

kaspar030 · 2016-10-12T10:30:50Z

You mean "the operation is non-blocking"? "return immediately" sounds more like implementation-speak...

Actually "the operation is non-blocking" seems less clear to me than "returns immediately". Would that extra s for return change your mind, too?

miri64 · 2016-10-12T10:31:36Z

Yes.

miri64 · 2016-10-12T10:32:56Z

sys/include/net/sock/ip.h

- * value `> 0`. If an error occurs on receive we just ignore it and continue
- * looping.
+ * value `!= SOCK_NO_TIMEOUT`. If an error occurs on receive we just ignore it
+ * and continue looping.


Reminder to self: fix this for #5923.

kaspar030 · 2016-10-12T10:34:08Z

@miri64 I think I got all your comments?

miri64

One last wording issue. Other then that I'm fine with this PR.

miri64 · 2016-10-12T10:34:54Z

sys/include/net/sock/ip.h

- *                      @ref sys_xtimer module is not present or the
- *                      implementation does not support timeouts on its own.
- *                      May be 0 for no timeout.
+ *                      If 0 and no data is available, returns immediately.


Okay, now I'm seeing this I immediately wonder: "What returns immediately" (the half-sentence is missing a subject ;-)).

so "the function returns immediately." ?

miri64 · 2016-10-12T10:35:03Z

sys/include/net/sock/tcp.h

- *                      @ref sys_xtimer module is not present and the
- *                      implementation does not support timeouts on its own.
- *                      May be 0 for no timeout.
+ *                      If 0 and no data is available, returns immediately.


miri64 · 2016-10-12T10:35:09Z

sys/include/net/sock/udp.h

- *                      @ref sys_xtimer module is not present or the
- *                      implementation does not support timeouts on its own.
- *                      May be 0 for no timeout.
+ *                      If 0 and no data is available, returns immediately.


miri64 · 2016-10-12T10:36:37Z

sys/include/net/sock/udp.h

- * alternatively set the `timeout` parameter of @ref sock_udp_recv() to a
- * value `> 0`. If an error occurs on receive we just ignore it and continue
- * looping.
+ * The application then waits indefinitely for an incoming message in `buf`


Minor question: Why did you change the line wrapping here?

I usually leave the line wrapping to vim. Mark the paragraph, press "gq", good.

miri64 · 2016-10-12T10:37:01Z

sys/include/net/sock/udp.h

- * looping.
+ * The application then waits indefinitely for an incoming message in `buf`
+ * from `remote`. If we want to timeout this wait period we could alternatively
+ * set the `timeout` parameter of @ref sock_udp_recv() to a value != @ref


I prefer the @ref and SOCK_NO_TIMEOUT to be on the same line

Yeah, but together with the line change above the line would cross the 80 characters mark...

You could put the line change before the @ref or even the value != ;-)

miri64 · 2016-10-12T10:43:38Z

sys/include/net/sock/ip.h

@@ -407,6 +407,7 @@ int sock_ip_get_remote(sock_ip_t *sock, sock_ip_ep_t *ep);
 * @return  -EPROTO, if source address of received packet did not equal
 *          the remote of @p sock.
 * @return  -ETIMEDOUT, if @p timeout expired.
+ * @return  -EAGAIN, if @p timeout is `0` and no data is available.


Nitpick-alert: The other return-values are lexicographically ordered ;-)

miri64 · 2016-10-12T10:43:53Z

sys/include/net/sock/tcp.h

@@ -207,6 +206,7 @@ int sock_tcp_accept(sock_tcp_queue_t *queue, sock_tcp_t **sock);
 *          point of @p sock.
 * @return  -ENOTCONN, when @p sock is not connected to a remote end point.
 * @return  -ETIMEDOUT, if @p timeout expired.
+ * @return  -EAGAIN, if @p timeout is `0` and no data is available.


Nitpick-alert: The other return-values are lexicographically ordered ;-)

miri64 · 2016-10-12T10:44:00Z

sys/include/net/sock/udp.h

@@ -395,6 +394,7 @@ int sock_udp_get_remote(sock_udp_t *sock, sock_udp_ep_t *ep);
 * @return  -EPROTO, if source address of received packet did not equal
 *          the remote of @p sock.
 * @return  -ETIMEDOUT, if @p timeout expired.
+ * @return  -EAGAIN, if @p timeout is `0` and no data is available.


Nitpick-alert: The other return-values are lexicographically ordered ;-)

miri64 · 2016-10-12T10:51:26Z

sys/include/net/sock/ip.h

- * value `> 0`. If an error occurs on receive we just ignore it and continue
- * looping.
+ * value `!= SOCK_NO_TIMEOUT`. If an error occurs on receive we just ignore it
+ * and continue looping.


Wait... the code above needs adaption to to be in line with that text.

In the snippet directly above too, please.

miri64 · 2016-10-12T10:51:46Z

sys/include/net/sock/udp.h

- * alternatively set the `timeout` parameter of @ref sock_udp_recv() to a
- * value `> 0`. If an error occurs on receive we just ignore it and continue
- * looping.
+ * The application then waits indefinitely for an incoming message in `buf`


miri64 · 2016-10-12T10:52:22Z

I hope I found everything now ^^"

kaspar030 · 2016-10-12T11:10:43Z

I hope I found everything now ^^"

Could we leave the formatting to "fill lines up to 80 chars, wrap before word"?

miri64 · 2016-10-12T12:52:58Z

Yapp

miri64

You forgot the snippets.

miri64 · 2016-10-12T12:53:47Z

sys/include/net/sock/ip.h

- * value `> 0`. If an error occurs on receive we just ignore it and continue
- * looping.
+ * value `!= SOCK_NO_TIMEOUT`. If an error occurs on receive we just ignore it
+ * and continue looping.


In the snippet directly above too, please.

miri64 · 2016-10-12T12:54:03Z

sys/include/net/sock/udp.h

- * alternatively set the `timeout` parameter of @ref sock_udp_recv() to a
- * value `> 0`. If an error occurs on receive we just ignore it and continue
- * looping.
+ * The application then waits indefinitely for an incoming message in `buf`


kaspar030 · 2016-10-12T13:02:22Z

I don't see what you mean. Could you point me to file and line?

miri64 · 2016-10-12T13:06:08Z

I don't see what you mean. Could you point me to file and line?

Oops, sorry. My bad... You already changed it but I was confused due to the diff layout.

miri64

ACK, please squash.

Follow-up on RIOT-OS#5929

kaspar030 added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Oct 11, 2016

kaspar030 assigned miri64 Oct 11, 2016

miri64 requested changes Oct 12, 2016

View reviewed changes

miri64 reviewed Oct 12, 2016

View reviewed changes

miri64 requested changes Oct 12, 2016

View reviewed changes

miri64 reviewed Oct 12, 2016

View reviewed changes

miri64 requested changes Oct 12, 2016

View reviewed changes

miri64 approved these changes Oct 12, 2016

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Oct 12, 2016

kaspar030 force-pushed the sock_change_no_timeout_value branch from cb4178c to aace136 Compare October 12, 2016 13:19

sock: change "no timeout" value from 0 to UINT32_MAX

aace136

miri64 mentioned this pull request Oct 12, 2016

sock: timeout/non-blocking behavior for send #5932

Open

miri64 merged commit 8c56c90 into RIOT-OS:master Oct 12, 2016

This was referenced Oct 12, 2016

pkg: provide sock support for lwIP #5802

Closed

sock_tcp: Add missing function and doc #5923

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 12, 2016

sock: Some formatting fixes

b9330d4

Follow-up on RIOT-OS#5929

This was referenced Oct 12, 2016

sock: Some formatting fixes #5933

Merged

gnrc_sock: provide port for sock_ip and sock_udp #5772

Merged

miri64 added this to the Release 2016.10 milestone Oct 17, 2016

neiljay pushed a commit to neiljay/RIOT that referenced this pull request Nov 4, 2016

sock: Some formatting fixes

06845c7

Follow-up on RIOT-OS#5929

kaspar030 deleted the sock_change_no_timeout_value branch February 7, 2017 22:27

sock: change "no timeout" value from 0 to UINT32_MAX #5929

sock: change "no timeout" value from 0 to UINT32_MAX #5929

Conversation

kaspar030 commented Oct 11, 2016

OlegHahm commented Oct 12, 2016

miri64 commented Oct 12, 2016

OlegHahm commented Oct 12, 2016

OlegHahm commented Oct 12, 2016

miri64 commented Oct 12, 2016

miri64 commented Oct 12, 2016

Choose a reason for hiding this comment

kaspar030 Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Oct 12, 2016

kaspar030 commented Oct 12, 2016

miri64 commented Oct 12, 2016

kaspar030 commented Oct 12, 2016

miri64 commented Oct 12, 2016

kaspar030 commented Oct 12, 2016

miri64 commented Oct 12, 2016

Choose a reason for hiding this comment

kaspar030 commented Oct 12, 2016

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Oct 12, 2016

kaspar030 commented Oct 12, 2016

miri64 commented Oct 12, 2016

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Oct 12, 2016

miri64 commented Oct 12, 2016

miri64 left a comment

Choose a reason for hiding this comment

kaspar030 Oct 12, 2016 •

edited

Loading