[PATCH 0/2] A fix and cleanup around qio_channel_socket_connect_sync()

[PATCH 0/2] A fix and cleanup around qio_channel_socket_connect_sync()

[Markus Armbruster]

Markus Armbruster (2):
  i386/kvm/vmsr_energy: Plug memory leak on failure to connect socket
  vfio scsi ui: Error-check qio_channel_socket_connect_sync() the same
    way

 hw/vfio-user/proxy.c          | 2 +-
 scsi/pr-manager-helper.c      | 9 ++-------
 target/i386/kvm/vmsr_energy.c | 6 +-----
 ui/input-barrier.c            | 5 +----
 4 files changed, 5 insertions(+), 17 deletions(-)

-- 
2.49.0

[PATCH 1/2] i386/kvm/vmsr_energy: Plug memory leak on failure to connect socket

[Markus Armbruster]

vmsr_open_socket() leaks the Error set by
qio_channel_socket_connect_sync().  Plug the leak by not creating the
Error.

Fixes: 0418f90809ae (Add support for RAPL MSRs in KVM/Qemu)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/i386/kvm/vmsr_energy.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/i386/kvm/vmsr_energy.c b/target/i386/kvm/vmsr_energy.c
index 58ce3df53a..890322ae37 100644
--- a/target/i386/kvm/vmsr_energy.c
+++ b/target/i386/kvm/vmsr_energy.c
@@ -57,13 +57,9 @@ QIOChannelSocket *vmsr_open_socket(const char *path)
     };
 
     QIOChannelSocket *sioc = qio_channel_socket_new();
-    Error *local_err = NULL;
 
     qio_channel_set_name(QIO_CHANNEL(sioc), "vmsr-helper");
-    qio_channel_socket_connect_sync(sioc,
-                                    &saddr,
-                                    &local_err);
-    if (local_err) {
+    if (qio_channel_socket_connect_sync(sioc, &saddr, NULL) < 0) {
         /* Close socket. */
         qio_channel_close(QIO_CHANNEL(sioc), NULL);
         object_unref(OBJECT(sioc));
-- 
2.49.0

[PATCH 2/2] vfio scsi ui: Error-check qio_channel_socket_connect_sync() the same way

[Markus Armbruster]

qio_channel_socket_connect_sync() returns 0 on success, and -1 on
failure, with errp set.  Some callers check the return value, and some
check whether errp was set.

For consistency, always check the return value, and always check it's
negative.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/vfio-user/proxy.c     | 2 +-
 scsi/pr-manager-helper.c | 9 ++-------
 ui/input-barrier.c       | 5 +----
 3 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
index 2275d3fe39..2c03d49f97 100644
--- a/hw/vfio-user/proxy.c
+++ b/hw/vfio-user/proxy.c
@@ -885,7 +885,7 @@ VFIOUserProxy *vfio_user_connect_dev(SocketAddress *addr, Error **errp)
 
     sioc = qio_channel_socket_new();
     ioc = QIO_CHANNEL(sioc);
-    if (qio_channel_socket_connect_sync(sioc, addr, errp)) {
+    if (qio_channel_socket_connect_sync(sioc, addr, errp) < 0) {
         object_unref(OBJECT(ioc));
         return NULL;
     }
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index 6b86f01b01..aea751fb04 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -105,20 +105,15 @@ static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
         .u.q_unix.path = path
     };
     QIOChannelSocket *sioc = qio_channel_socket_new();
-    Error *local_err = NULL;
-
     uint32_t flags;
     int r;
 
     assert(!pr_mgr->ioc);
     qio_channel_set_name(QIO_CHANNEL(sioc), "pr-manager-helper");
-    qio_channel_socket_connect_sync(sioc,
-                                    &saddr,
-                                    &local_err);
+    r = qio_channel_socket_connect_sync(sioc, &saddr, errp);
     g_free(path);
-    if (local_err) {
+    if (r < 0) {
         object_unref(OBJECT(sioc));
-        error_propagate(errp, local_err);
         return -ENOTCONN;
     }
 
diff --git a/ui/input-barrier.c b/ui/input-barrier.c
index 9793258aac..0a2198ca50 100644
--- a/ui/input-barrier.c
+++ b/ui/input-barrier.c
@@ -490,7 +490,6 @@ static gboolean input_barrier_event(QIOChannel *ioc G_GNUC_UNUSED,
 static void input_barrier_complete(UserCreatable *uc, Error **errp)
 {
     InputBarrier *ib = INPUT_BARRIER(uc);
-    Error *local_err = NULL;
 
     if (!ib->name) {
         error_setg(errp, QERR_MISSING_PARAMETER, "name");
@@ -506,9 +505,7 @@ static void input_barrier_complete(UserCreatable *uc, Error **errp)
     ib->sioc = qio_channel_socket_new();
     qio_channel_set_name(QIO_CHANNEL(ib->sioc), "barrier-client");
 
-    qio_channel_socket_connect_sync(ib->sioc, &ib->saddr, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (qio_channel_socket_connect_sync(ib->sioc, &ib->saddr, errp) < 0) {
         return;
     }
 
-- 
2.49.0

Re: [PATCH 0/2] A fix and cleanup around qio_channel_socket_connect_sync()

[Markus Armbruster]

Ping... your chance to review, silence will be met with a PR ;)

Re: [PATCH 1/2] i386/kvm/vmsr_energy: Plug memory leak on failure to connect socket

[Zhao Liu]

On Wed, Jul 23, 2025 at 03:32:56PM +0200, Markus Armbruster wrote:
> Date: Wed, 23 Jul 2025 15:32:56 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 1/2] i386/kvm/vmsr_energy: Plug memory leak on failure to
>  connect socket
> 
> vmsr_open_socket() leaks the Error set by
> qio_channel_socket_connect_sync().  Plug the leak by not creating the
> Error.
> 
> Fixes: 0418f90809ae (Add support for RAPL MSRs in KVM/Qemu)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target/i386/kvm/vmsr_energy.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 2/2] vfio scsi ui: Error-check qio_channel_socket_connect_sync() the same way

[Zhao Liu]

On Wed, Jul 23, 2025 at 03:32:57PM +0200, Markus Armbruster wrote:
> Date: Wed, 23 Jul 2025 15:32:57 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 2/2] vfio scsi ui: Error-check
>  qio_channel_socket_connect_sync() the same way
> 
> qio_channel_socket_connect_sync() returns 0 on success, and -1 on
> failure, with errp set.  Some callers check the return value, and some
> check whether errp was set.
> 
> For consistency, always check the return value, and always check it's
> negative.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/vfio-user/proxy.c     | 2 +-
>  scsi/pr-manager-helper.c | 9 ++-------
>  ui/input-barrier.c       | 5 +----
>  3 files changed, 4 insertions(+), 12 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 0/2] A fix and cleanup around qio_channel_socket_connect_sync()

[Markus Armbruster]

Queued.  Thanks for the review!

Re: [PATCH 1/2] i386/kvm/vmsr_energy: Plug memory leak on failure to connect socket

[Michael Tokarev]

On 23.07.2025 16:32, Markus Armbruster wrote:
> vmsr_open_socket() leaks the Error set by
> qio_channel_socket_connect_sync().  Plug the leak by not creating the
> Error.
> 
> Fixes: 0418f90809ae (Add support for RAPL MSRs in KVM/Qemu)

I'm picking this up for qemu-stable (10.0 & 10.1).
Please let me know if I shouldn't.

Thanks,

/mjt

Re: [PATCH 2/2] vfio scsi ui: Error-check qio_channel_socket_connect_sync() the same way

[Michael Tokarev]

On 23.07.2025 16:32, Markus Armbruster wrote:
> qio_channel_socket_connect_sync() returns 0 on success, and -1 on
> failure, with errp set.  Some callers check the return value, and some
> check whether errp was set.
> 
> For consistency, always check the return value, and always check it's
> negative.

Ditto for this one - applying this for qemu-stable (10.0 & 10.1).
It doesn't hurt, and the code in longer-supported branches will
be less different from the master branch, making future changes,
if any, easier to pick up (there were multiple fixes in this area
recently).

Please let me know if I shouldn't :)

Thanks,

/mjt