On 09.09.25 11:38, Daniel P. Berrangé wrote:
On Wed, Sep 03, 2025 at 12:44:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
First, qio_channel_readv_full() already guarantees BLOCKING and
CLOEXEC states for incoming descriptors, no reason call extra
ioctls.
Second, current implementation calls _set_block() and _set_cloexec()
again on old descriptors on failure path - we fix this too.
Finally, handling errors exactly after qio_channel_readv_full() call
looks more readable.
Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
---
chardev/char-socket.c | 37 +++++++++++++------------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1e8313915b..5b9b19ba8b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -293,6 +293,18 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf,
size_t len)
0, &err);
}
+ if (ret == QIO_CHANNEL_ERR_BLOCK) {
+ errno = EAGAIN;
+ return -1;
+ } else if (ret == -1) {
+ trace_chr_socket_recv_err(chr, chr->label, error_get_pretty(err));
+ error_free(err);
+ errno = EIO;
+ return -1;
+ }
+
+ assert(ret >= 0);
Moving this logic to here means that in the blocking I/O, and/or error
paths, we are not clearing out the previously read s->read_msgfds_num
These should be considered obsolete at the point we start a new read
call, regardless of its success, hence why we had the code ordered in
this way.
Oops, thanks! That's not obvious, I'll add a comment.
+
if (msgfds_num) {
/* close and clean read_msgfds */
for (i = 0; i < s->read_msgfds_num; i++) {
@@ -307,30 +319,7 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf,
size_t len)
s->read_msgfds_num = msgfds_num;
}
- for (i = 0; i < s->read_msgfds_num; i++) {
- int fd = s->read_msgfds[i];
- if (fd < 0) {
- continue;
- }
-
- /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
- qemu_socket_set_block(fd);
-
-#ifndef MSG_CMSG_CLOEXEC
- qemu_set_cloexec(fd);
-#endif
- }
This for(){} removal is fnie.
-
- if (ret == QIO_CHANNEL_ERR_BLOCK) {
- errno = EAGAIN;
- ret = -1;
- } else if (ret == -1) {
- trace_chr_socket_recv_err(chr, chr->label, error_get_pretty(err));
- error_free(err);
- errno = EIO;
- } else if (ret == 0) {
- trace_chr_socket_recv_eof(chr, chr->label);
- }
+ trace_chr_socket_recv_eof(chr, chr->label);
return ret;
}
--
2.48.1
With regards,
Daniel
--
Best regards,
Vladimir
