Skip to content

Clarify TCPSocket::recv() and UDPSocket::recvfrom() documentation. #5623

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

Merged
merged 1 commit into from
Jan 15, 2018

Conversation

SeppoTakalo
Copy link
Contributor

  • Clarify recv() call to return zero on end-of-stream.
  • Clarify recvfrom() to actually return size of datagram, not some arbitrary size.

CC: @kjbracey-arm @sg-

* non-blocking or times out, NSAPI_ERROR_WOULD_BLOCK is returned
* immediately.
*
* @param address Destination for the source address or NULL
* @param data Destination buffer for data received from the host
* @param size Size of the buffer in bytes
* @return Number of received bytes on success, negative error
* @return Size of received datagram, negative error
Copy link
Contributor

@kjbracey kjbracey Dec 1, 2017

Choose a reason for hiding this comment

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

Unfortunately, this wording is wrong in a different way - if the datagram is bigger than your buffer, it should return the bytes actually read - it's quietly truncated. (Which means applications have to provide known-to-be-big-enough buffers for whatever protocol they're implementing).

The body is too wooly about "receiving data", but at least the brief is already correct, I see - it does say "Receive a packet", compared to "Receive data" on TCPSocket::recv.

How about transforming to "datagram" everywhere:

/** Receive a datagram over a UDP socket
 *
 *  Receives a datagram and stores the source address in address if address
 *  is not NULL. Returns the number of bytes written into the buffer. If the
 *  datagram is larger than the buffer, the excess data is silently discarded.
 *
 *  By default, recvfrom blocks until a datagram is received. If socket is set to
 *  non-blocking or times out with no datagram, NSAPI_ERROR_WOULD_BLOCK
 *  is returned.
 *
 *  @param address  Destination for the source address or NULL
 *  @param data     Destination buffer for datagram received from the host
 *  @param size     Size of the buffer in bytes
 *  @return         Number of received bytes on success, negative error
 *                  code on failure
 */

@SeppoTakalo
Copy link
Contributor Author

Updated the UDPSocket::recvfrom() documentation as Kevin suggested.

I was not sure about the truncating so I left it out originally. Good that it is now specified.

@@ -111,7 +111,9 @@ class TCPSocket : public Socket {
* @param data Destination buffer for data received from the host
* @param size Size of the buffer in bytes
* @return Number of received bytes on success, negative error
* code on failure
* code on failure. If no messages are available to be received
Copy link
Contributor

Choose a reason for hiding this comment

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

Conversely to the UDP case, this should be "no [more?] data is available", rather than introducing "messages".

* code on failure
* code on failure. If no messages are available to be received
* and the peer has performed an orderly shutdown,
* recv() shall return 0.
Copy link
Contributor

Choose a reason for hiding this comment

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

"returns 0" - everything else is present tense.

@theotherjimmy
Copy link
Contributor

@AnotherButler Docs!

* By default, recvfrom blocks until data is sent. If socket is set to
* non-blocking or times out, NSAPI_ERROR_WOULD_BLOCK is returned
* immediately.
* By default, recvfrom blocks until a datagram is received. If socket is set to
Copy link
Contributor

Choose a reason for hiding this comment

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

Query: Who or what receives the datagram? Please clarify for active voice.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about "blocks until a datagram arrives"?

(POSIX version is much more wordy and specific, with a couple of "if no messages are available..." Don't know if it's worth spelling that out. Could compromise with "has arrived", to imply it may have already arrived before the call.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like "blocks until a datagram arrives".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see much difference between "blocks until a datagram is received" and "blocks until a datagram arrives".

It might even confuse more. Datagram can arrive earlier that user calls this function. Then user receives the datagram from the network stack's buffer. So "datagram arrives" is not totally accurate. Should we clarify this a bit? I found it already clear for me, so I assume SW engineer understand accurately it already.

For a comparison, POSIX standard definition of this same function http://pubs.opengroup.org/onlinepubs/009695399/functions/recvfrom.html

And as this function is recvfrom() then the answer to "who receives" is just who ever calls this function.

@AnotherButler
Copy link
Contributor

Query: Do we need to update or add to any of the text on either of these pages?

https://os.mbed.com/docs/v5.7/reference/tcpsocket.html
https://os.mbed.com/docs/v5.7/reference/udpsocket.html

The actual class reference is transcluded, so it will change automatically. Does any of the rest of it need changing?

@cmonr
Copy link
Contributor

cmonr commented Jan 11, 2018

@AnotherButler Who were you asking?

@AnotherButler
Copy link
Contributor

@cmonr Anyone who feels confident answering.

@cmonr
Copy link
Contributor

cmonr commented Jan 11, 2018

@AnotherButler I think those pages should be fine as long as this PR is transcluded when rebuilt.

@AnotherButler
Copy link
Contributor

@cmonr Awesome, thanks 👍

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 12, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jan 12, 2018

Build : SUCCESS

Build number : 845
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5623/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Jan 12, 2018

@mbed-ci
Copy link

mbed-ci commented Jan 12, 2018

@SeppoTakalo
Copy link
Contributor Author

I updated the TCPSocket documentation based on Kevin's feedback.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jan 15, 2018

Build : SUCCESS

Build number : 861
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5623/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Jan 15, 2018

@mbed-ci
Copy link

mbed-ci commented Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants