casaroli commented on code in PR #20702:
URL: https://github.com/apache/nuttx/pull/20702#discussion_r4398256279
##########
net/inet/inet_sockif.c:
##########
@@ -1839,20 +1839,60 @@ static ssize_t inet_sendto(FAR struct socket *psock,
FAR const void *buf,
static ssize_t inet_sendmsg(FAR struct socket *psock,
FAR const struct msghdr *msg, int flags)
{
- FAR void *buf = msg->msg_iov->iov_base;
- size_t len = msg->msg_iov->iov_len;
+ FAR void *buf;
+ size_t len;
FAR const struct sockaddr *to = msg->msg_name;
socklen_t tolen = msg->msg_namelen;
FAR const struct iovec *iov;
FAR const struct iovec *end;
int ret;
+ if (msg->msg_iovlen == 0)
+ {
+ return -EINVAL;
+ }
+
+ buf = msg->msg_iov->iov_base;
+ len = msg->msg_iov->iov_len;
+
if (msg->msg_iovlen == 1)
{
return to ? inet_sendto(psock, buf, len, flags, to, tolen) :
inet_send(psock, buf, len, flags);
}
+ /* If this is a connected stream socket (TCP), we can send each iov chunk
+ * sequentially without allocating a temporary memory buffer.
+ */
+
+ if (psock->s_type == SOCK_STREAM && to == NULL)
+ {
+ ssize_t total = 0;
+
+ end = &msg->msg_iov[msg->msg_iovlen];
+ for (iov = msg->msg_iov; iov != end; iov++)
+ {
+ if (iov->iov_len == 0 || iov->iov_base == NULL)
+ {
+ continue;
+ }
+
+ ret = inet_send(psock, iov->iov_base, iov->iov_len, flags);
Review Comment:
With CONFIG_NET_TCP_WRITE_BUFFERS=n (the default), psock_tcp_send() returns
only after the peer ACKs all of its data (tcp_send_unbuffered.c:211). One
inet_send() per iov thus sends each iov in its own segments and waits one round
trip, or the peer's delayed-ACK time, for each iov. The old code sent the whole
message with one call. Also, each iov takes the connection lock separately, so
a send() from another thread can put its data between the iovs of one sendmsg().
Fix: keep the gather-into-one-buffer path for unbuffered TCP, or send all
iovs under one lock with one wait.
##########
net/local/local_recvmsg.c:
##########
@@ -75,6 +75,10 @@ static int psock_fifo_read(FAR struct socket *psock, FAR
void *buf,
ret = file_ioctl(&conn->lc_infile, PIPEIOC_PEEK,
(unsigned long)((uintptr_t)&peek));
+ if (ret >= 0)
+ {
+ *readlen = ret;
Review Comment:
PIPEIOC_PEEK does not block. With `*readlen = ret`, a blocking
recv(MSG_PEEK) on an empty stream socket now returns 0, which the caller reads
as end of file. This line also changes the MSG_PEEK result of
psock_dgram_recvfrom().
Fix: put this change in its own commit, make a blocking peek on an empty
stream wait for data (or return -EAGAIN with MSG_DONTWAIT), and test it.
##########
net/inet/inet_sockif.c:
##########
@@ -1839,20 +1839,60 @@ static ssize_t inet_sendto(FAR struct socket *psock,
FAR const void *buf,
static ssize_t inet_sendmsg(FAR struct socket *psock,
FAR const struct msghdr *msg, int flags)
{
- FAR void *buf = msg->msg_iov->iov_base;
- size_t len = msg->msg_iov->iov_len;
+ FAR void *buf;
+ size_t len;
FAR const struct sockaddr *to = msg->msg_name;
socklen_t tolen = msg->msg_namelen;
FAR const struct iovec *iov;
FAR const struct iovec *end;
int ret;
+ if (msg->msg_iovlen == 0)
Review Comment:
POSIX gives EMSGSIZE for msg_iovlen <= 0, not EINVAL. On master, a TCP
recvmsg() with msg_iovlen == 0 and a valid msg_iov returns 0, because
psock_tcp_recvfrom() does not enter its loop; line 2210 changes that to an
error. The same check is at can_recvmsg.c:435, can_sendmsg_buffered.c:226,
udp_recvfrom.c:654 and local_recvmsg.c:552.
Fix: return -EMSGSIZE at all six places, or keep the master behaviour for
msg_iovlen == 0.
##########
net/local/local_recvmsg.c:
##########
@@ -540,29 +546,86 @@ ssize_t local_recvmsg(FAR struct socket *psock, FAR
struct msghdr *msg,
{
FAR socklen_t *fromlen = &msg->msg_namelen;
FAR struct sockaddr *from = msg->msg_name;
- FAR void *buf = msg->msg_iov->iov_base;
- size_t len = msg->msg_iov->iov_len;
+ FAR void *buf;
+ ssize_t len;
- if (msg->msg_iovlen != 1)
+ if (msg->msg_iovlen == 0)
{
- return -ENOTSUP;
+ return -EINVAL;
}
- DEBUGASSERT(buf);
+ buf = msg->msg_iov->iov_base;
+ len = msg->msg_iov->iov_len;
/* Check for a stream socket */
#ifdef CONFIG_NET_LOCAL_STREAM
if (psock->s_type == SOCK_STREAM)
{
- len = psock_stream_recvfrom(psock, buf, len, flags, from, fromlen);
+ size_t peek_offset = 0;
+ ssize_t total_recv = 0;
+ ssize_t ret;
+ int cur_flags;
+ int i;
+
+ for (i = 0; i < msg->msg_iovlen; i++)
+ {
+ if (msg->msg_iov[i].iov_len == 0 ||
+ msg->msg_iov[i].iov_base == NULL)
+ {
+ continue;
+ }
+
+ cur_flags = flags;
+
+ /* If data has already been received and MSG_WAITALL is not set,
+ * do not block waiting for more data in subsequent iovecs.
+ */
+
+ if (total_recv > 0 && !(flags & MSG_WAITALL))
+ {
+ cur_flags |= MSG_DONTWAIT;
+ }
+
+ ret = psock_stream_recvfrom(psock, msg->msg_iov[i].iov_base,
+ msg->msg_iov[i].iov_len, peek_offset,
+ cur_flags, from, fromlen);
+ if (ret <= 0)
+ {
+ if (total_recv == 0)
+ {
+ total_recv = ret;
+ }
+
+ break;
+ }
+
+ total_recv += ret;
+ if (flags & MSG_PEEK)
+ {
+ peek_offset += ret;
+ }
+
+ if (!(flags & MSG_WAITALL) && ret < msg->msg_iov[i].iov_len)
Review Comment:
psock_stream_recvfrom() reads with once=true and can return less than
iov_len. With MSG_WAITALL the loop then goes to the next iov and leaves the
rest of iov[i] empty, so the next bytes go to iov[i + 1] while the return value
counts them as contiguous.
Fix: read again into the rest of iov[i] until it is full before you go to
iov[i + 1].
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
