Add support for setting rcvbuf buffer size on a socket #907
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fadushin
force-pushed
the
set-recv-buffer-size
branch
4 times, most recently
from
d196d2a
to
ea77a28
Compare
fadushin
force-pushed
the
set-recv-buffer-size
branch
from
ea77a28
to
1b951a5
Compare
pguyot
reviewed
Nov 1, 2023
pguyot
reviewed
Nov 1, 2023
bettio
reviewed
Nov 2, 2023
bettio
requested changes
Nov 2, 2023
fadushin
force-pushed
the
set-recv-buffer-size
branch
from
1b951a5
to
7b44a4b
Compare
fadushin
commented
Nov 3, 2023
src/libAtomVM/otp_socket.c
Outdated
| ssize_t buffer_size = len == 0 ? (ssize_t) res : MIN((size_t) res, len); | ||
| // user-supplied len has higher precedence than the default buffer size, but we also | ||
| // want the configured default buffer size to be a lower bound on anything we peek | ||
| ssize_t buffer_size = len == 0 ? MIN((size_t) res, rsrc_obj->buf_size) : MIN((size_t) res, len); |
There was a problem hiding this comment.
Aren't we doing MIN(len == 0 ? rsrc_obj->buf_size : len, res) ?
There was a problem hiding this comment.
This change is also a good chance to fix a warning we have:
AtomVM/src/libAtomVM/otp_socket.c:1665:42: warning: operand of ‘?:’ changes signedness from ‘long int’ to ‘long unsigned int’ due to unsignedness of other operand [-Wsign-compare]
1665 | ssize_t buffer_size = len == 0 ? (ssize_t) res : MIN((size_t) res, len);
| ^~~~~~~~~~~~~
There was a problem hiding this comment.
Aren't we doing
MIN(len == 0 ? rsrc_obj->buf_size : len, res)?
I believe these are equivalent, right? (assuming MIN is commutative) Changed to your version, with changes to address any compiler warnings.
Scanning dependencies of target libAtomVMLinux-x86_64
[ 28%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/mapped_file.c.o
[ 28%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/platform_defaultatoms.c.o
[ 28%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/smp.c.o
[ 28%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/platform_nifs.c.o
[ 28%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/socket_driver.c.o
[ 28%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/otp_socket_platform.c.o
[ 29%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/sys.c.o
[ 29%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/__/__/__/libAtomVM/inet.c.o
[ 29%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/__/__/__/libAtomVM/otp_net.c.o
[ 29%] Building C object src/platforms/generic_unix/lib/CMakeFiles/libAtomVMLinux-x86_64.dir/__/__/__/libAtomVM/otp_socket.c.o
[ 29%] Linking C static library liblibAtomVMLinux-x86_64.a
[ 29%] Built target libAtomVMLinux-x86_64
fadushin
commented
Nov 3, 2023
fadushin
force-pushed
the
set-recv-buffer-size
branch
from
7b44a4b
to
56f6d31
Compare
fadushin
force-pushed
the
set-recv-buffer-size
branch
11 times, most recently
from
369f53c
to
0bfe2af
Compare
pguyot
reviewed
Nov 17, 2023
src/libAtomVM/otp_socket.c
Outdated
| ssize_t buffer_size = len == 0 ? (ssize_t) res : MIN((size_t) res, len); | ||
| // user-supplied len has higher precedence than the default buffer size, but we also | ||
| // want the configured default buffer size to be a lower bound on anything we peek | ||
| ssize_t buffer_size = len == 0 ? MIN((size_t) res, rsrc_obj->buf_size) : MIN((size_t) res, len); |
There was a problem hiding this comment.
Aren't we doing MIN(len == 0 ? rsrc_obj->buf_size : len, res) ?
pguyot
reviewed
Nov 23, 2023
doc/src/programmers-guide.md
Outdated
| @@ -1804,12 +1804,14 @@ Currently, the following options are supported: | |||
| |------------|--------------|-------------| | |||
| | `{socket, reuseaddr}` | `boolean()` | Sets `SO_REUSEADDR` on the socket. | | |||
| | `{socket, linger}` | `#{onoff => boolean(), linger => non_neg_integer()}` | Sets `SO_LINGER` on the socekt. | | |||
| | `{otp, rcvbuf}` | `non_neg_integer()` | Sets the default buffer size (in bytes) on receive calls. This value is only used if the `Length` parameter of the `socket:recv` family of functions has the value `0`; otherwise, the specified non-zero length in the `socket:recv` takes precendence. | | |||
There was a problem hiding this comment.
Can we have 0 for this value?
OTP specifies default | integer()>0
There was a problem hiding this comment.
I wonder what the meaning of default is? Would that be for the case in which you set the buffer size to be some value, and then want to set it back to default at a later time?
I'd say in that case we either:
- defer implementing that use case (not sure how useful it is) with a TODO
- Add support for setting it back to default, if desired (would be easy to do)
There was a problem hiding this comment.
Added note to table and TODO in the code. Not sure why CI did not kick off.
fadushin
force-pushed
the
set-recv-buffer-size
branch
from
0bfe2af
to
c2356cc
Compare
fadushin
force-pushed
the
set-recv-buffer-size
branch
from
c2356cc
to
29b291a
Compare
Signed-off-by: Fred Dushin <fred@dushin.net>
fadushin
force-pushed
the
set-recv-buffer-size
branch
from
29b291a
to
5fc7732
Compare
bettio
approved these changes
Dec 6, 2023
bettio
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR allows users to set the buffer size on receive operations via the
socket:setopt/3function, using the{otp, rcvbuf}configuration option.This PR addresses Issue #896
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later