wengzhe commented on code in PR #10925:
URL: https://github.com/apache/nuttx/pull/10925#discussion_r1366374367
##########
net/tcp/tcp_conn.c:
##########
@@ -579,27 +579,26 @@ int tcp_selectport(uint8_t domain,
uint16_t portno)
{
static uint16_t g_last_tcp_port;
- ssize_t ret;
/* Generate port base dynamically */
if (g_last_tcp_port == 0)
{
- ret = getrandom(&g_last_tcp_port, sizeof(uint16_t), 0);
+#if defined(CONFIG_DEV_URANDOM) || defined(CONFIG_DEV_RANDOM)
+ ssize_t ret = getrandom(&g_last_tcp_port, sizeof(uint16_t), 0);
if (ret < 0)
{
ret = getrandom(&g_last_tcp_port, sizeof(uint16_t), GRND_RANDOM);
}
if (ret != sizeof(uint16_t))
+#endif
{
- g_last_tcp_port = clock_systime_ticks() % 32000;
- }
- else
- {
- g_last_tcp_port = g_last_tcp_port % 32000;
+ g_last_tcp_port = (uint16_t)clock_systime_ticks();
}
+ g_last_tcp_port = g_last_tcp_port % 32000;
Review Comment:
But I'm not sure if it's worthwhile to involve more checks, the previous
code can already work with error codes, but now we need to do an extra check of
the macros. I regard it as a negative impact that we may need to suffer if we
want to check random devices during code linking (Only the codes designed to be
able to work without `getrandom` need to add this check).
##########
net/tcp/tcp_conn.c:
##########
@@ -579,27 +579,26 @@ int tcp_selectport(uint8_t domain,
uint16_t portno)
{
static uint16_t g_last_tcp_port;
- ssize_t ret;
/* Generate port base dynamically */
if (g_last_tcp_port == 0)
{
- ret = getrandom(&g_last_tcp_port, sizeof(uint16_t), 0);
+#if defined(CONFIG_DEV_URANDOM) || defined(CONFIG_DEV_RANDOM)
+ ssize_t ret = getrandom(&g_last_tcp_port, sizeof(uint16_t), 0);
if (ret < 0)
{
ret = getrandom(&g_last_tcp_port, sizeof(uint16_t), GRND_RANDOM);
}
if (ret != sizeof(uint16_t))
+#endif
{
- g_last_tcp_port = clock_systime_ticks() % 32000;
- }
- else
- {
- g_last_tcp_port = g_last_tcp_port % 32000;
+ g_last_tcp_port = (uint16_t)clock_systime_ticks();
}
+ g_last_tcp_port = g_last_tcp_port % 32000;
Review Comment:
Yes, they're different in distribution, but it's intentional because we only
use it once per boot and don't need to use an exactly uniform distribution.
This logic was first introduced by https://github.com/apache/nuttx/pull/6154
to avoid tcp port from being the same after a reset, distribution is not a
problem, we just need it to be different. cc @XinStellaris
BTW, the previous distribution is also not a uniform distribution (because
we don't allow port numbers less than 4096 in L603-606)
https://gist.github.com/wengzhe/01f82a96d66a6b0cf87229c2e02de268
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
