[v1] io: deal with blocking/non-blocking fds

[PATCH 02/10] char-socket: rework tcp_chr_recv()

Posted by Vladimir Sementsov-Ogievskiy 5 months, 1 week ago

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 <vsementsov@yandex-team.ru>
---
 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);
+
     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
-    }
-
-    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

Re: [PATCH 02/10] char-socket: rework tcp_chr_recv()

Posted by Daniel P. Berrangé 5 months ago

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 <vsementsov@yandex-team.ru>
> ---
>  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.


> +
>      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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 02/10] char-socket: rework tcp_chr_recv()

Posted by Vladimir Sementsov-Ogievskiy 5 months ago

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 <vsementsov@yandex-team.ru>
>> ---
>>   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

Re: [PATCH 02/10] char-socket: rework tcp_chr_recv()

Posted by Peter Xu 5 months ago

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 <vsementsov@yandex-team.ru>
> ---
>  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);
> +
>      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
> -    }
> -
> -    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);

This tracepoint may still need to be put into a ret==0 check.

Looks reasonable other than that..

>  
>      return ret;
>  }
> -- 
> 2.48.1
> 

-- 
Peter Xu

Re: [PATCH 02/10] char-socket: rework tcp_chr_recv()

Posted by Vladimir Sementsov-Ogievskiy 5 months ago

On 09.09.25 00:53, Peter Xu 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 <vsementsov@yandex-team.ru>
>> ---
>>   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);
>> +
>>       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
>> -    }
>> -
>> -    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);
> 
> This tracepoint may still need to be put into a ret==0 check.
> 
> Looks reasonable other than that..
> 

Oh, right. Will fix.

>>   
>>       return ret;
>>   }
>> -- 
>> 2.48.1
>>
> 


-- 
Best regards,
Vladimir