API: Tweak virDomainOpenGraphics to return fd directly

Let's fix this before we bake in a painful API.  Since we know
that we have exactly one non-negative fd on success, we might
as well return the fd directly instead of forcing the user to
pass in a pointer.  Furthermore, I found some memory and fd
leaks while reviewing the code - the idea is that on success,
libvirtd will have handed two fds in two different directions:
one to qemu, and one to the RPC client.

* include/libvirt/libvirt.h.in (virDomainOpenGraphicsFD): Drop
unneeded parameter.
* src/driver.h (virDrvDomainOpenGraphicsFD): Likewise.
* src/libvirt.c (virDomainOpenGraphicsFD): Adjust interface to
return fd directly.
* daemon/remote.c (remoteDispatchDomainOpenGraphicsFd): Adjust
semantics.
* src/qemu/qemu_driver.c (qemuDomainOpenGraphicsFD): Likewise,
and plug fd leak.
* src/remote/remote_driver.c (remoteDomainOpenGraphicsFD):
Likewise, and plug memory and fd leak.

Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/daemon/remote.c b/daemon/remote.c
index 27d1aa8037c5f1b6c02e9fd2c549a5678a24b5f3..9251576b4a8cede7400db18acebcf202014b788e 100644 (file)
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -4398,6 +4398,7 @@ remoteDispatchDomainOpenGraphics(virNetServerPtr server ATTRIBUTE_UNUSED,
     return rv;
 }
 
+
 static int
 remoteDispatchDomainOpenGraphicsFd(virNetServerPtr server ATTRIBUTE_UNUSED,
                                    virNetServerClientPtr client ATTRIBUTE_UNUSED,
@@ -4419,10 +4420,9 @@ remoteDispatchDomainOpenGraphicsFd(virNetServerPtr server ATTRIBUTE_UNUSED,
     if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
         goto cleanup;
 
-    if (virDomainOpenGraphicsFD(dom,
-                                args->idx,
-                                &fd,
-                                args->flags) < 0)
+    if ((fd = virDomainOpenGraphicsFD(dom,
+                                      args->idx,
+                                      args->flags)) < 0)
         goto cleanup;
 
     if (virNetMessageAddFD(msg, fd) < 0)
@@ -4442,6 +4442,8 @@ remoteDispatchDomainOpenGraphicsFd(virNetServerPtr server ATTRIBUTE_UNUSED,
         virDomainFree(dom);
     return rv;
 }
+
+
 static int
 remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUSED,
                                            virNetServerClientPtr client ATTRIBUTE_UNUSED,

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 2a108b81aa14c41607a6d261fa65d281d718261a..e79c9ad9318c79d817765959d0761839bb23cb85 100644 (file)
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -5401,7 +5401,6 @@ int virDomainOpenGraphics(virDomainPtr dom,
 
 int virDomainOpenGraphicsFD(virDomainPtr dom,
                             unsigned int idx,
-                            int *fd,
                             unsigned int flags);
 
 int virDomainInjectNMI(virDomainPtr domain, unsigned int flags);

diff --git a/src/driver.h b/src/driver.h
index cd136ba3d704550445217bfd704a1e47e678f22f..a7366e44c33811c2283df564221728c6da9b07ca 100644 (file)
--- a/src/driver.h
+++ b/src/driver.h
@@ -890,7 +890,6 @@ typedef int
 typedef int
 (*virDrvDomainOpenGraphicsFD)(virDomainPtr dom,
                               unsigned int idx,
-                              int *fd,
                               unsigned int flags);
 
 typedef int

diff --git a/src/libvirt.c b/src/libvirt.c
index 772cc0d4bbb1f9dbee558919f0fc74f41dda78fd..17ec679d9b3219c3a3ba037cfc24d31187b003d2 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -20305,11 +20305,11 @@ virDomainOpenGraphics(virDomainPtr dom,
  * virDomainOpenGraphicsFD:
  * @dom: pointer to domain object
  * @idx: index of graphics config to open
- * @fd: returned file descriptor
  * @flags: bitwise-OR of virDomainOpenGraphicsFlags
  *
  * This will create a socket pair connected to the graphics backend of @dom.
- * One socket will be returned as @fd.
+ * One end of the socket will be returned on success, and the other end is
+ * handed to the hypervisor.
  * If @dom has multiple graphics backends configured, then @idx will determine
  * which one is opened, starting from @idx 0.
  *
@@ -20318,23 +20318,20 @@ virDomainOpenGraphics(virDomainPtr dom,
  *
  * This method can only be used when connected to a local
  * libvirt hypervisor, over a UNIX domain socket. Attempts
- * to use this method over a TCP connection will always fail
+ * to use this method over a TCP connection will always fail.
  *
- * Returns 0 on success, -1 on failure
+ * Returns an fd on success, -1 on failure
  */
 int
 virDomainOpenGraphicsFD(virDomainPtr dom,
                         unsigned int idx,
-                        int *fd,
                         unsigned int flags)
 {
-    VIR_DOMAIN_DEBUG(dom, "idx=%u, fd=%p, flags=%x",
-                     idx, fd, flags);
+    VIR_DOMAIN_DEBUG(dom, "idx=%u, flags=%x", idx, flags);
 
     virResetLastError();
 
     virCheckDomainReturn(dom, -1);
-    virCheckNonNullArgGoto(fd, error);
 
     virCheckReadOnlyGoto(dom->conn->flags, error);
 
@@ -20347,7 +20344,7 @@ virDomainOpenGraphicsFD(virDomainPtr dom,
 
     if (dom->conn->driver->domainOpenGraphicsFD) {
         int ret;
-        ret = dom->conn->driver->domainOpenGraphicsFD(dom, idx, fd, flags);
+        ret = dom->conn->driver->domainOpenGraphicsFD(dom, idx, flags);
         if (ret < 0)
             goto error;
         return ret;
@@ -20359,6 +20356,8 @@ virDomainOpenGraphicsFD(virDomainPtr dom,
     virDispatchError(dom->conn);
     return -1;
 }
+
+
 /**
  * virConnectSetKeepAlive:
  * @conn: pointer to a hypervisor connection

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5ff205908000d5ce9c82621945918514aac1c25d..f5b17ba80a0edd9a629ab78b6561aad0afcbe978 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15808,7 +15808,6 @@ qemuDomainOpenGraphics(virDomainPtr dom,
 static int
 qemuDomainOpenGraphicsFD(virDomainPtr dom,
                          unsigned int idx,
-                         int *fd,
                          unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
@@ -15866,18 +15865,19 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
         goto cleanup;
     qemuDomainObjEnterMonitor(driver, vm);
     ret = qemuMonitorOpenGraphics(priv->mon, protocol, pair[1], "graphicsfd",
-                                  (flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH) != 0);
+                                  (flags & VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH));
     qemuDomainObjExitMonitor(driver, vm);
     if (!qemuDomainObjEndJob(driver, vm))
         vm = NULL;
+    if (ret < 0)
+        goto cleanup;
 
-    *fd = pair[0];
+    ret = pair[0];
+    pair[0] = -1;
 
  cleanup:
-    if (ret < 0) {
-        VIR_FORCE_CLOSE(pair[0]);
-        VIR_FORCE_CLOSE(pair[1]);
-    }
+    VIR_FORCE_CLOSE(pair[0]);
+    VIR_FORCE_CLOSE(pair[1]);
     if (vm)
         virObjectUnlock(vm);
     return ret;

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index e01216a86132248fc96eae00e5fd4754f2007b27..e949223bc9764811e54a8f6802283bfca7c3a39a 100644 (file)
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -6448,7 +6448,6 @@ remoteDomainOpenGraphics(virDomainPtr dom,
 static int
 remoteDomainOpenGraphicsFD(virDomainPtr dom,
                            unsigned int idx,
-                           int *fd,
                            unsigned int flags)
 {
     int rv = -1;
@@ -6472,15 +6471,21 @@ remoteDomainOpenGraphicsFD(virDomainPtr dom,
         goto done;
 
     if (fdoutlen != 1) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("no file descriptor received"));
+        if (fdoutlen) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("too many file descriptors received"));
+            while (fdoutlen)
+                VIR_FORCE_CLOSE(fdout[--fdoutlen]);
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("no file descriptor received"));
+        }
         goto done;
     }
-    *fd = fdout[0];
-
-    rv = 0;
+    rv = fdout[0];
 
  done:
+    VIR_FREE(fdout);
     remoteDriverUnlock(priv);
 
     return rv;