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

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

Merged
merged 1 commit into from Jan 15, 2018

Conversation

Projects
None yet
7 participants
@SeppoTakalo
Contributor

SeppoTakalo commented Nov 30, 2017

  • 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-

features/netsocket/UDPSocket.h Outdated
* 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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Dec 1, 2017

Contributor

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 SeppoTakalo force-pushed the SeppoTakalo:clarify_socket_docs branch to 6ad6ff7 Dec 1, 2017

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Dec 1, 2017

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Dec 4, 2017

Contributor

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.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Dec 4, 2017

Contributor

"returns 0" - everything else is present tense.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Dec 22, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 8, 2018

* 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

This comment has been minimized.

@AnotherButler

AnotherButler Jan 9, 2018

Contributor

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 9, 2018

Contributor

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.)

This comment has been minimized.

@AnotherButler

AnotherButler Jan 9, 2018

Contributor

I like "blocks until a datagram arrives".

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 12, 2018

Contributor

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

This comment has been minimized.

Contributor

AnotherButler commented Jan 9, 2018

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

This comment has been minimized.

Contributor

cmonr commented Jan 11, 2018

@AnotherButler Who were you asking?

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Jan 11, 2018

@cmonr Anyone who feels confident answering.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 11, 2018

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

@cmonr cmonr added ready for merge and removed needs: work labels Jan 11, 2018

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Jan 11, 2018

@cmonr Awesome, thanks 👍

@0xc0170 0xc0170 added needs: CI and removed ready for merge labels Jan 12, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 12, 2018

/morph build

@mbed-ci

This comment has been minimized.

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@SeppoTakalo SeppoTakalo force-pushed the SeppoTakalo:clarify_socket_docs branch from 6ad6ff7 to 6bf0611 Jan 12, 2018

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Jan 12, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 15, 2018

/morph build

@mbed-ci

This comment has been minimized.

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit 663a6d8 into ARMmbed:master Jan 15, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment