Use common qemu_set_blocking() instead.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
contrib/ivshmem-server/ivshmem-server.c | 5 ++++-
hw/hyperv/syndbg.c | 4 +++-
hw/virtio/vhost-user.c | 5 ++++-
include/qemu/sockets.h | 1 -
io/channel-socket.c | 7 +++----
net/dgram.c | 16 +++++++++++++---
net/l2tpv3.c | 5 +++--
net/socket.c | 20 ++++++++++++++++----
qga/channel-posix.c | 7 ++++++-
tests/unit/socket-helpers.c | 5 ++++-
tests/unit/test-crypto-tlssession.c | 8 ++++----
util/oslib-posix.c | 7 -------
util/oslib-win32.c | 5 -----
util/vhost-user-server.c | 4 ++--
14 files changed, 62 insertions(+), 37 deletions(-)
diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
index 2f3c7320a6..9ccd436ee4 100644
--- a/contrib/ivshmem-server/ivshmem-server.c
+++ b/contrib/ivshmem-server/ivshmem-server.c
@@ -146,7 +146,10 @@ ivshmem_server_handle_new_conn(IvshmemServer *server)
return -1;
}
- qemu_socket_set_nonblock(newfd);
+ if (!qemu_set_blocking(newfd, false, NULL)) {
+ close(newfd);
+ return -1;
+ }
IVSHMEM_SERVER_DEBUG(server, "accept()=%d\n", newfd);
/* allocate new structure for this peer */
diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
index ac7e15f6f1..bcdfdf6af7 100644
--- a/hw/hyperv/syndbg.c
+++ b/hw/hyperv/syndbg.c
@@ -338,7 +338,9 @@ static void hv_syndbg_realize(DeviceState *dev, Error **errp)
return;
}
- qemu_socket_set_nonblock(syndbg->socket);
+ if (!qemu_set_blocking(syndbg->socket, false, errp)) {
+ return;
+ }
syndbg->servaddr.sin_port = htons(syndbg->host_port);
syndbg->servaddr.sin_family = AF_INET;
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1e1d6b0d6e..36c9c2e04d 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2039,7 +2039,10 @@ static int vhost_user_postcopy_advise(struct vhost_dev *dev, Error **errp)
error_setg(errp, "%s: Failed to get ufd", __func__);
return -EIO;
}
- qemu_socket_set_nonblock(ufd);
+ if (!qemu_set_blocking(ufd, false, errp)) {
+ close(ufd);
+ return -EINVAL;
+ }
/* register ufd with userfault thread */
u->postcopy_fd.fd = ufd;
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index c562690d89..6477f90b9e 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -48,7 +48,6 @@ int socket_set_cork(int fd, int v);
int socket_set_nodelay(int fd);
void qemu_socket_set_block(int fd);
int qemu_socket_try_set_nonblock(int fd);
-void qemu_socket_set_nonblock(int fd);
int socket_set_fast_reuse(int fd);
#ifdef WIN32
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 3b7ca924ff..e77602a22e 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -820,11 +820,10 @@ qio_channel_socket_set_blocking(QIOChannel *ioc,
{
QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
- if (enabled) {
- qemu_socket_set_block(sioc->fd);
- } else {
- qemu_socket_set_nonblock(sioc->fd);
+ if (!qemu_set_blocking(sioc->fd, enabled, errp)) {
+ return -1;
}
+
return 0;
}
diff --git a/net/dgram.c b/net/dgram.c
index 48f653bceb..fb9ded30df 100644
--- a/net/dgram.c
+++ b/net/dgram.c
@@ -226,7 +226,10 @@ static int net_dgram_mcast_create(struct sockaddr_in *mcastaddr,
}
}
- qemu_socket_set_nonblock(fd);
+ if (!qemu_set_blocking(fd, false, errp)) {
+ goto fail;
+ }
+
return fd;
fail:
if (fd >= 0) {
@@ -504,7 +507,11 @@ int net_init_dgram(const Netdev *netdev, const char *name,
close(fd);
return -1;
}
- qemu_socket_set_nonblock(fd);
+
+ if (!qemu_set_blocking(fd, false, errp)) {
+ close(fd);
+ return -1;
+ }
dest_len = sizeof(raddr_in);
dest_addr = g_malloc(dest_len);
@@ -551,7 +558,10 @@ int net_init_dgram(const Netdev *netdev, const char *name,
close(fd);
return -1;
}
- qemu_socket_set_nonblock(fd);
+ if (!qemu_set_blocking(fd, false, errp)) {
+ close(fd);
+ return -1;
+ }
dest_len = sizeof(raddr_un);
dest_addr = g_malloc(dest_len);
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index b5547cb917..cdfc641aa6 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -648,6 +648,9 @@ int net_init_l2tpv3(const Netdev *netdev,
error_setg(errp, "could not bind socket err=%i", errno);
goto outerr;
}
+ if (!qemu_set_blocking(fd, false, errp)) {
+ goto outerr;
+ }
freeaddrinfo(result);
@@ -709,8 +712,6 @@ int net_init_l2tpv3(const Netdev *netdev,
s->vec = g_new(struct iovec, MAX_L2TPV3_IOVCNT);
s->header_buf = g_malloc(s->header_size);
- qemu_socket_set_nonblock(fd);
-
s->fd = fd;
s->counter = 0;
diff --git a/net/socket.c b/net/socket.c
index 784dda686f..db25e3d9ae 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -295,7 +295,10 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr,
}
}
- qemu_socket_set_nonblock(fd);
+ if (!qemu_set_blocking(fd, false, errp)) {
+ goto fail;
+ }
+
return fd;
fail:
if (fd >= 0)
@@ -508,7 +511,10 @@ static int net_socket_listen_init(NetClientState *peer,
error_setg_errno(errp, errno, "can't create stream socket");
return -1;
}
- qemu_socket_set_nonblock(fd);
+ if (!qemu_set_blocking(fd, false, errp)) {
+ close(fd);
+ return -1;
+ }
socket_set_fast_reuse(fd);
@@ -556,7 +562,10 @@ static int net_socket_connect_init(NetClientState *peer,
error_setg_errno(errp, errno, "can't create stream socket");
return -1;
}
- qemu_socket_set_nonblock(fd);
+ if (!qemu_set_blocking(fd, false, errp)) {
+ close(fd);
+ return -1;
+ }
connected = 0;
for(;;) {
@@ -671,7 +680,10 @@ static int net_socket_udp_init(NetClientState *peer,
close(fd);
return -1;
}
- qemu_socket_set_nonblock(fd);
+ if (!qemu_set_blocking(fd, false, errp)) {
+ close(fd);
+ return -1;
+ }
s = net_socket_fd_init_dgram(peer, model, name, fd, 0, NULL, errp);
if (!s) {
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 465d688ecb..ddf8ebdc5e 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -28,6 +28,7 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
GAChannel *c = data;
int ret, client_fd;
bool accepted = false;
+ Error *err = NULL;
g_assert(channel != NULL);
@@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
g_warning("error converting fd to gsocket: %s", strerror(errno));
goto out;
}
- qemu_socket_set_nonblock(client_fd);
+ if (!qemu_set_blocking(client_fd, false, &err)) {
+ g_warning("errer: %s", error_get_pretty(err));
+ error_free(err);
+ goto out;
+ }
ret = ga_channel_client_add(c, client_fd);
if (ret) {
g_warning("error setting up connection");
diff --git a/tests/unit/socket-helpers.c b/tests/unit/socket-helpers.c
index 37db24f72a..1b7e283f24 100644
--- a/tests/unit/socket-helpers.c
+++ b/tests/unit/socket-helpers.c
@@ -88,7 +88,10 @@ static int socket_can_bind_connect(const char *hostname, int family)
goto cleanup;
}
- qemu_socket_set_nonblock(cfd);
+ if (!qemu_set_blocking(cfd, false, NULL)) {
+ goto cleanup;
+ }
+
if (connect(cfd, (struct sockaddr *)&ss, sslen) < 0) {
if (errno == EINPROGRESS) {
check_soerr = true;
diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
index 554054e934..61311cbe6e 100644
--- a/tests/unit/test-crypto-tlssession.c
+++ b/tests/unit/test-crypto-tlssession.c
@@ -112,8 +112,8 @@ static void test_crypto_tls_session_psk(void)
* thread, so we need these non-blocking to avoid deadlock
* of ourselves
*/
- qemu_socket_set_nonblock(channel[0]);
- qemu_socket_set_nonblock(channel[1]);
+ qemu_set_blocking(channel[0], false, &error_abort);
+ qemu_set_blocking(channel[1], false, &error_abort);
clientCreds = test_tls_creds_psk_create(
QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
@@ -264,8 +264,8 @@ static void test_crypto_tls_session_x509(const void *opaque)
* thread, so we need these non-blocking to avoid deadlock
* of ourselves
*/
- qemu_socket_set_nonblock(channel[0]);
- qemu_socket_set_nonblock(channel[1]);
+ qemu_set_blocking(channel[0], false, &error_abort);
+ qemu_set_blocking(channel[1], false, &error_abort);
#define CLIENT_CERT_DIR "tests/test-crypto-tlssession-client/"
#define SERVER_CERT_DIR "tests/test-crypto-tlssession-server/"
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index e473938195..dc23b33210 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -272,13 +272,6 @@ int qemu_socket_try_set_nonblock(int fd)
return g_unix_set_fd_nonblocking(fd, true, NULL) ? 0 : -errno;
}
-void qemu_socket_set_nonblock(int fd)
-{
- int f;
- f = qemu_socket_try_set_nonblock(fd);
- assert(f == 0);
-}
-
int socket_set_fast_reuse(int fd)
{
int val = 1, ret;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 03044f5b59..1566eb57e7 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -211,11 +211,6 @@ int qemu_socket_try_set_nonblock(int fd)
return 0;
}
-void qemu_socket_set_nonblock(int fd)
-{
- (void)qemu_socket_try_set_nonblock(fd);
-}
-
int socket_set_fast_reuse(int fd)
{
/* Enabling the reuse of an endpoint that was used by a socket still in
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index b19229074a..8dcd32dc65 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -78,7 +78,7 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg)
}
for (i = 0; i < vmsg->fd_num; i++) {
- qemu_socket_set_nonblock(vmsg->fds[i]);
+ qemu_set_blocking(vmsg->fds[i], false, &error_abort);
}
}
@@ -303,7 +303,7 @@ set_watch(VuDev *vu_dev, int fd, int vu_evt,
vu_fd_watch->fd = fd;
vu_fd_watch->cb = cb;
- qemu_socket_set_nonblock(fd);
+ qemu_set_blocking(fd, false, &error_abort);
aio_set_fd_handler(server->ctx, fd, kick_handler,
NULL, NULL, NULL, vu_fd_watch);
vu_fd_watch->vu_dev = vu_dev;
--
2.48.1On Wed, Sep 03, 2025 at 12:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Use common qemu_set_blocking() instead.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> contrib/ivshmem-server/ivshmem-server.c | 5 ++++-
> hw/hyperv/syndbg.c | 4 +++-
> hw/virtio/vhost-user.c | 5 ++++-
> include/qemu/sockets.h | 1 -
> io/channel-socket.c | 7 +++----
> net/dgram.c | 16 +++++++++++++---
> net/l2tpv3.c | 5 +++--
> net/socket.c | 20 ++++++++++++++++----
> qga/channel-posix.c | 7 ++++++-
> tests/unit/socket-helpers.c | 5 ++++-
> tests/unit/test-crypto-tlssession.c | 8 ++++----
> util/oslib-posix.c | 7 -------
> util/oslib-win32.c | 5 -----
> util/vhost-user-server.c | 4 ++--
> 14 files changed, 62 insertions(+), 37 deletions(-)
>
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index 465d688ecb..ddf8ebdc5e 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -28,6 +28,7 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
> GAChannel *c = data;
> int ret, client_fd;
> bool accepted = false;
> + Error *err = NULL;
>
> g_assert(channel != NULL);
>
> @@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
> g_warning("error converting fd to gsocket: %s", strerror(errno));
> goto out;
> }
> - qemu_socket_set_nonblock(client_fd);
> + if (!qemu_set_blocking(client_fd, false, &err)) {
> + g_warning("errer: %s", error_get_pretty(err));
s/errer/error/
This is a pre-existing problem, but none of this code should be using
g_warning. g_printerr() should have been used for printing error
messages. I'm not expecting you to fix that, just an observation.
> + error_free(err);
> + goto out;
> + }
> ret = ga_channel_client_add(c, client_fd);
> if (ret) {
> g_warning("error setting up connection");
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 :|
On 10.09.25 12:44, Daniel P. Berrangé wrote:
>> @@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
>> g_warning("error converting fd to gsocket: %s", strerror(errno));
>> goto out;
>> }
>> - qemu_socket_set_nonblock(client_fd);
>> + if (!qemu_set_blocking(client_fd, false, &err)) {
>> + g_warning("errer: %s", error_get_pretty(err));
> s/errer/error/
>
>
> This is a pre-existing problem, but none of this code should be using
> g_warning. g_printerr() should have been used for printing error
> messages. I'm not expecting you to fix that, just an observation.
Why not g_error()? I see some g_warnings in qga code a correct "warnings", not "errors".. And if we use sometimes g_warning, the g_error is more correct pair for it.
Or we don't want any of g_error / g_warning in QEMU code?
--
Best regards,
Vladimir
On Wed, Sep 10, 2025 at 08:55:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 10.09.25 12:44, Daniel P. Berrangé wrote:
> > > @@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
> > > g_warning("error converting fd to gsocket: %s", strerror(errno));
> > > goto out;
> > > }
> > > - qemu_socket_set_nonblock(client_fd);
> > > + if (!qemu_set_blocking(client_fd, false, &err)) {
> > > + g_warning("errer: %s", error_get_pretty(err));
> > s/errer/error/
> >
> >
> > This is a pre-existing problem, but none of this code should be using
> > g_warning. g_printerr() should have been used for printing error
> > messages. I'm not expecting you to fix that, just an observation.
>
>
> Why not g_error()? I see some g_warnings in qga code a correct "warnings", not "errors".. And if we use sometimes g_warning, the g_error is more correct pair for it.
>
> Or we don't want any of g_error / g_warning in QEMU code?
g_error will call abort() after printing the message, which will
prevent graceful cleanup and result in a core file, so is not
very desirable to use.
g_warning will also turn into g_error if G_DEBUG=fatal-warnings
is set.
We really just want a plain message printed on the console with
no side effects, and g_printerr gives us that.
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 :|
On 10.09.25 21:15, Daniel P. Berrangé wrote:
> On Wed, Sep 10, 2025 at 08:55:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 10.09.25 12:44, Daniel P. Berrangé wrote:
>>>> @@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
>>>> g_warning("error converting fd to gsocket: %s", strerror(errno));
>>>> goto out;
>>>> }
>>>> - qemu_socket_set_nonblock(client_fd);
>>>> + if (!qemu_set_blocking(client_fd, false, &err)) {
>>>> + g_warning("errer: %s", error_get_pretty(err));
>>> s/errer/error/
>>>
>>>
>>> This is a pre-existing problem, but none of this code should be using
>>> g_warning. g_printerr() should have been used for printing error
>>> messages. I'm not expecting you to fix that, just an observation.
>>
>>
>> Why not g_error()? I see some g_warnings in qga code a correct "warnings", not "errors".. And if we use sometimes g_warning, the g_error is more correct pair for it.
>>
>> Or we don't want any of g_error / g_warning in QEMU code?
>
> g_error will call abort() after printing the message, which will
> prevent graceful cleanup and result in a core file, so is not
> very desirable to use.
Oh, right, I was stupid)
>
> g_warning will also turn into g_error if G_DEBUG=fatal-warnings
> is set.
>
> We really just want a plain message printed on the console with
> no side effects, and g_printerr gives us that.
>
>
Still, it's strange for me, that we just not use error_reprot()/warn_report() everywhere.
I see we have underlying error_vprintf() realization for code without monitor in stubs/error-printf.c..
--
Best regards,
Vladimir
On Wed, Sep 10, 2025 at 09:52:40PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 10.09.25 21:15, Daniel P. Berrangé wrote:
> > On Wed, Sep 10, 2025 at 08:55:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 10.09.25 12:44, Daniel P. Berrangé wrote:
> > > > > @@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
> > > > > g_warning("error converting fd to gsocket: %s", strerror(errno));
> > > > > goto out;
> > > > > }
> > > > > - qemu_socket_set_nonblock(client_fd);
> > > > > + if (!qemu_set_blocking(client_fd, false, &err)) {
> > > > > + g_warning("errer: %s", error_get_pretty(err));
> > > > s/errer/error/
> > > >
> > > >
> > > > This is a pre-existing problem, but none of this code should be using
> > > > g_warning. g_printerr() should have been used for printing error
> > > > messages. I'm not expecting you to fix that, just an observation.
> > >
> > >
> > > Why not g_error()? I see some g_warnings in qga code a correct "warnings", not "errors".. And if we use sometimes g_warning, the g_error is more correct pair for it.
> > >
> > > Or we don't want any of g_error / g_warning in QEMU code?
> >
> > g_error will call abort() after printing the message, which will
> > prevent graceful cleanup and result in a core file, so is not
> > very desirable to use.
>
> Oh, right, I was stupid)
>
> >
> > g_warning will also turn into g_error if G_DEBUG=fatal-warnings
> > is set.
> >
> > We really just want a plain message printed on the console with
> > no side effects, and g_printerr gives us that.
> >
> >
>
> Still, it's strange for me, that we just not use error_reprot()/warn_report() everywhere.
Those would be viable alternatives too. Basically anything is better
than 'g_warning' for this scenario :-)
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 :|
On 10.09.25 12:44, Daniel P. Berrangé wrote:
> On Wed, Sep 03, 2025 at 12:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Use common qemu_set_blocking() instead.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> contrib/ivshmem-server/ivshmem-server.c | 5 ++++-
>> hw/hyperv/syndbg.c | 4 +++-
>> hw/virtio/vhost-user.c | 5 ++++-
>> include/qemu/sockets.h | 1 -
>> io/channel-socket.c | 7 +++----
>> net/dgram.c | 16 +++++++++++++---
>> net/l2tpv3.c | 5 +++--
>> net/socket.c | 20 ++++++++++++++++----
>> qga/channel-posix.c | 7 ++++++-
>> tests/unit/socket-helpers.c | 5 ++++-
>> tests/unit/test-crypto-tlssession.c | 8 ++++----
>> util/oslib-posix.c | 7 -------
>> util/oslib-win32.c | 5 -----
>> util/vhost-user-server.c | 4 ++--
>> 14 files changed, 62 insertions(+), 37 deletions(-)
>>
>
>> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
>> index 465d688ecb..ddf8ebdc5e 100644
>> --- a/qga/channel-posix.c
>> +++ b/qga/channel-posix.c
>> @@ -28,6 +28,7 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
>> GAChannel *c = data;
>> int ret, client_fd;
>> bool accepted = false;
>> + Error *err = NULL;
>>
>> g_assert(channel != NULL);
>>
>> @@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
>> g_warning("error converting fd to gsocket: %s", strerror(errno));
>> goto out;
>> }
>> - qemu_socket_set_nonblock(client_fd);
>> + if (!qemu_set_blocking(client_fd, false, &err)) {
>> + g_warning("errer: %s", error_get_pretty(err));
>
> s/errer/error/
>
>
> This is a pre-existing problem, but none of this code should be using
> g_warning. g_printerr() should have been used for printing error
> messages. I'm not expecting you to fix that, just an observation.
Sure.
>
>> + error_free(err);
>> + goto out;
>> + }
>> ret = ga_channel_client_add(c, client_fd);
>> if (ret) {
>> g_warning("error setting up connection");
>
>
> With regards,
> Daniel
--
Best regards,
Vladimir
On 10.09.25 13:33, Vladimir Sementsov-Ogievskiy wrote:
> On 10.09.25 12:44, Daniel P. Berrangé wrote:
>> On Wed, Sep 03, 2025 at 12:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> Use common qemu_set_blocking() instead.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>> contrib/ivshmem-server/ivshmem-server.c | 5 ++++-
>>> hw/hyperv/syndbg.c | 4 +++-
>>> hw/virtio/vhost-user.c | 5 ++++-
>>> include/qemu/sockets.h | 1 -
>>> io/channel-socket.c | 7 +++----
>>> net/dgram.c | 16 +++++++++++++---
>>> net/l2tpv3.c | 5 +++--
>>> net/socket.c | 20 ++++++++++++++++----
>>> qga/channel-posix.c | 7 ++++++-
>>> tests/unit/socket-helpers.c | 5 ++++-
>>> tests/unit/test-crypto-tlssession.c | 8 ++++----
>>> util/oslib-posix.c | 7 -------
>>> util/oslib-win32.c | 5 -----
>>> util/vhost-user-server.c | 4 ++--
>>> 14 files changed, 62 insertions(+), 37 deletions(-)
>>>
>>
>>> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
>>> index 465d688ecb..ddf8ebdc5e 100644
>>> --- a/qga/channel-posix.c
>>> +++ b/qga/channel-posix.c
>>> @@ -28,6 +28,7 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
>>> GAChannel *c = data;
>>> int ret, client_fd;
>>> bool accepted = false;
>>> + Error *err = NULL;
>>> g_assert(channel != NULL);
>>> @@ -36,7 +37,11 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
>>> g_warning("error converting fd to gsocket: %s", strerror(errno));
>>> goto out;
>>> }
>>> - qemu_socket_set_nonblock(client_fd);
>>> + if (!qemu_set_blocking(client_fd, false, &err)) {
>>> + g_warning("errer: %s", error_get_pretty(err));
>>
>> s/errer/error/
>>
>>
>> This is a pre-existing problem, but none of this code should be using
>> g_warning. g_printerr() should have been used for printing error
>> messages. I'm not expecting you to fix that, just an observation.
>
> Sure.
Ha, some how I've read "not expecting" with opposite meaning) So by "sure", I meant "will fix".
Still now looking at code and your comment more carefully, this means one more patch, as
adding g_prinerr among two g_warnings is mess.
One more patch won't hurt I think.
>
>>
>>> + error_free(err);
>>> + goto out;
>>> + }
>>> ret = ga_channel_client_add(c, client_fd);
>>> if (ret) {
>>> g_warning("error setting up connection");
>>
>>
>> With regards,
>> Daniel
>
>
--
Best regards,
Vladimir
On Wed, Sep 03, 2025 at 12:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Use common qemu_set_blocking() instead.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Posix's qemu_socket_set_nonblock() asserts the retval.. While Windows's one
doesn't. IIUC that's the only reason you provided the generic error
path in all callers, just in case some of them might fail on Windows?
I wished Windows's one has an assertion from the start too. Maybe none of
the failure path would really trigger.. Not a big deal:
Reviewed-by: Peter Xu <peterx@redhat.com>
One nitpick below:
> ---
> contrib/ivshmem-server/ivshmem-server.c | 5 ++++-
> hw/hyperv/syndbg.c | 4 +++-
> hw/virtio/vhost-user.c | 5 ++++-
> include/qemu/sockets.h | 1 -
> io/channel-socket.c | 7 +++----
> net/dgram.c | 16 +++++++++++++---
> net/l2tpv3.c | 5 +++--
> net/socket.c | 20 ++++++++++++++++----
> qga/channel-posix.c | 7 ++++++-
> tests/unit/socket-helpers.c | 5 ++++-
> tests/unit/test-crypto-tlssession.c | 8 ++++----
> util/oslib-posix.c | 7 -------
> util/oslib-win32.c | 5 -----
> util/vhost-user-server.c | 4 ++--
> 14 files changed, 62 insertions(+), 37 deletions(-)
>
> diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
> index 2f3c7320a6..9ccd436ee4 100644
> --- a/contrib/ivshmem-server/ivshmem-server.c
> +++ b/contrib/ivshmem-server/ivshmem-server.c
> @@ -146,7 +146,10 @@ ivshmem_server_handle_new_conn(IvshmemServer *server)
> return -1;
> }
>
> - qemu_socket_set_nonblock(newfd);
> + if (!qemu_set_blocking(newfd, false, NULL)) {
Better if with a IVSHMEM_SERVER_DEBUG(), which follows the original lines.
> + close(newfd);
> + return -1;
> + }
> IVSHMEM_SERVER_DEBUG(server, "accept()=%d\n", newfd);
>
> /* allocate new structure for this peer */
--
Peter Xu
On 09.09.25 01:16, Peter Xu wrote:
> On Wed, Sep 03, 2025 at 12:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Use common qemu_set_blocking() instead.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Posix's qemu_socket_set_nonblock() asserts the retval.. While Windows's one
> doesn't. IIUC that's the only reason you provided the generic error
> path in all callers, just in case some of them might fail on Windows?
Honestly, I thought that checking error on Linux is good too.. It may fail,
why not to check, where possible?
>
> I wished Windows's one has an assertion from the start too. Maybe none of
> the failure path would really trigger.. Not a big deal:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One nitpick below:
>
>> ---
>> contrib/ivshmem-server/ivshmem-server.c | 5 ++++-
>> hw/hyperv/syndbg.c | 4 +++-
>> hw/virtio/vhost-user.c | 5 ++++-
>> include/qemu/sockets.h | 1 -
>> io/channel-socket.c | 7 +++----
>> net/dgram.c | 16 +++++++++++++---
>> net/l2tpv3.c | 5 +++--
>> net/socket.c | 20 ++++++++++++++++----
>> qga/channel-posix.c | 7 ++++++-
>> tests/unit/socket-helpers.c | 5 ++++-
>> tests/unit/test-crypto-tlssession.c | 8 ++++----
>> util/oslib-posix.c | 7 -------
>> util/oslib-win32.c | 5 -----
>> util/vhost-user-server.c | 4 ++--
>> 14 files changed, 62 insertions(+), 37 deletions(-)
>>
>> diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
>> index 2f3c7320a6..9ccd436ee4 100644
>> --- a/contrib/ivshmem-server/ivshmem-server.c
>> +++ b/contrib/ivshmem-server/ivshmem-server.c
>> @@ -146,7 +146,10 @@ ivshmem_server_handle_new_conn(IvshmemServer *server)
>> return -1;
>> }
>>
>> - qemu_socket_set_nonblock(newfd);
>> + if (!qemu_set_blocking(newfd, false, NULL)) {
>
> Better if with a IVSHMEM_SERVER_DEBUG(), which follows the original lines.
Will add
>
>> + close(newfd);
>> + return -1;
>> + }
>> IVSHMEM_SERVER_DEBUG(server, "accept()=%d\n", newfd);
>>
>> /* allocate new structure for this peer */
>
Thanks for reviewing my patches!
--
Best regards,
Vladimir
On Tue, Sep 09, 2025 at 11:19:39AM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 09.09.25 01:16, Peter Xu wrote: > > On Wed, Sep 03, 2025 at 12:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > Use common qemu_set_blocking() instead. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > > Posix's qemu_socket_set_nonblock() asserts the retval.. While Windows's one > > doesn't. IIUC that's the only reason you provided the generic error > > path in all callers, just in case some of them might fail on Windows? > > Honestly, I thought that checking error on Linux is good too.. It may fail, > why not to check, where possible? Yep, it diagnoses the case where the FD might be invalid, or where QEMU might not have access to it. This could potentially avoid killing the VM if a FD was passed to QEMU over monitor that had access limited by SELinux. 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 :|
On 10.09.25 12:41, Daniel P. Berrangé wrote: > On Tue, Sep 09, 2025 at 11:19:39AM +0300, Vladimir Sementsov-Ogievskiy wrote: >> On 09.09.25 01:16, Peter Xu wrote: >>> On Wed, Sep 03, 2025 at 12:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>> Use common qemu_set_blocking() instead. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>> >>> Posix's qemu_socket_set_nonblock() asserts the retval.. While Windows's one >>> doesn't. IIUC that's the only reason you provided the generic error >>> path in all callers, just in case some of them might fail on Windows? >> >> Honestly, I thought that checking error on Linux is good too.. It may fail, >> why not to check, where possible? > > Yep, it diagnoses the case where the FD might be invalid, or where > QEMU might not have access to it. This could potentially avoid killing > the VM if a FD was passed to QEMU over monitor that had access limited > by SELinux. > Good reason! I'll add it commit msg. -- Best regards, Vladimir
© 2016 - 2026 Red Hat, Inc.