Misc xen driver bug/crash fixes

diff --git a/ChangeLog b/ChangeLog
index c6b7696f21a4a6ab6942f7b7f5eebee83696c40d..d19c01e8c65adc8d8ae6ba037d15299098586648 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+Thu Jan 29 23:01:22 GMT 2009 Daniel P. Berrange <berrange@redhat.com>
+
+       Misc Xen driver crash/bug fixes
+       * src/remote_internal.c: Re-factor startup of secondary driver
+       activation to fix missing initialization & crash.  Fix memory
+       leak in error reporting
+       * src/xen_unified.c: Don't activate inotify driver if non-root
+       * src/xend_internal.c: Don't report errors when probing for
+       XenD TCP port if unprivileged, allow caller to do it. Fix bad
+       return values in open method
+       * src/xs_internal.c: Fix double free
+
 Thu Jan 29 17:22:53 GMT 2009 John Levon <john.levon@sun.com>
 
        * src/xend_internal.c: Fix xend XML generation when CPU pinning

diff --git a/src/remote_internal.c b/src/remote_internal.c
index 06f8a7f09f4fba9b80410e18c763522ff0db7810..f8740af6864141afe97f0afcebe9bcd5196353d6 100644 (file)
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -892,31 +892,70 @@ doRemoteOpen (virConnectPtr conn,
     goto cleanup;
 }
 
-static virDrvOpenStatus
-remoteOpen (virConnectPtr conn,
-            virConnectAuthPtr auth,
-            int flags)
+static struct private_data *
+remoteAllocPrivateData(virConnectPtr conn)
 {
     struct private_data *priv;
-    int ret, rflags = 0;
-
-    if (inside_daemon)
-        return VIR_DRV_OPEN_DECLINED;
-
     if (VIR_ALLOC(priv) < 0) {
-        virReportOOMError (conn);
-        return VIR_DRV_OPEN_ERROR;
+        virReportOOMError(conn);
+        return NULL;
     }
 
     if (virMutexInit(&priv->lock) < 0) {
         error(conn, VIR_ERR_INTERNAL_ERROR,
               _("cannot initialize mutex"));
         VIR_FREE(priv);
-        return VIR_DRV_OPEN_ERROR;
+        return NULL;
     }
     remoteDriverLock(priv);
     priv->localUses = 1;
     priv->watch = -1;
+    priv->sock = -1;
+
+    return priv;
+}
+
+static int
+remoteOpenSecondaryDriver(virConnectPtr conn,
+                          virConnectAuthPtr auth,
+                          int flags,
+                          struct private_data **priv)
+{
+    int ret;
+    int rflags = 0;
+
+    if (!((*priv) = remoteAllocPrivateData(conn)))
+        return VIR_DRV_OPEN_ERROR;
+
+    if (flags & VIR_CONNECT_RO)
+        rflags |= VIR_DRV_OPEN_REMOTE_RO;
+    rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
+
+    ret = doRemoteOpen(conn, *priv, auth, rflags);
+    if (ret != VIR_DRV_OPEN_SUCCESS) {
+        remoteDriverUnlock(*priv);
+        VIR_FREE(*priv);
+    } else {
+        (*priv)->localUses = 1;
+        remoteDriverUnlock(*priv);
+    }
+
+    return ret;
+}
+
+static virDrvOpenStatus
+remoteOpen (virConnectPtr conn,
+            virConnectAuthPtr auth,
+            int flags)
+{
+    struct private_data *priv;
+    int ret, rflags = 0;
+
+    if (inside_daemon)
+        return VIR_DRV_OPEN_DECLINED;
+
+    if (!(priv = remoteAllocPrivateData(conn)))
+        return VIR_DRV_OPEN_ERROR;
 
     if (flags & VIR_CONNECT_RO)
         rflags |= VIR_DRV_OPEN_REMOTE_RO;
@@ -971,7 +1010,6 @@ remoteOpen (virConnectPtr conn,
 #endif
     }
 
-    priv->sock = -1;
     ret = doRemoteOpen(conn, priv, auth, rflags);
     if (ret != VIR_DRV_OPEN_SUCCESS) {
         conn->privateData = NULL;
@@ -3085,30 +3123,13 @@ remoteNetworkOpen (virConnectPtr conn,
          * which doesn't have its own impl of the network APIs.
          */
         struct private_data *priv;
-        int ret, rflags = 0;
-        if (VIR_ALLOC(priv) < 0) {
-            virReportOOMError (conn);
-            return VIR_DRV_OPEN_ERROR;
-        }
-        if (virMutexInit(&priv->lock) < 0) {
-            error(conn, VIR_ERR_INTERNAL_ERROR,
-                  _("cannot initialize mutex"));
-            VIR_FREE(priv);
-            return VIR_DRV_OPEN_ERROR;
-        }
-        if (flags & VIR_CONNECT_RO)
-            rflags |= VIR_DRV_OPEN_REMOTE_RO;
-        rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
-
-        priv->sock = -1;
-        ret = doRemoteOpen(conn, priv, auth, rflags);
-        if (ret != VIR_DRV_OPEN_SUCCESS) {
-            conn->networkPrivateData = NULL;
-            VIR_FREE(priv);
-        } else {
-            priv->localUses = 1;
+        int ret;
+        ret = remoteOpenSecondaryDriver(conn,
+                                        auth,
+                                        flags,
+                                        &priv);
+        if (ret == VIR_DRV_OPEN_SUCCESS)
             conn->networkPrivateData = priv;
-        }
         return ret;
     }
 }
@@ -3598,30 +3619,13 @@ remoteStorageOpen (virConnectPtr conn,
          * which doesn't have its own impl of the network APIs.
          */
         struct private_data *priv;
-        int ret, rflags = 0;
-        if (VIR_ALLOC(priv) < 0) {
-            virReportOOMError (NULL);
-            return VIR_DRV_OPEN_ERROR;
-        }
-        if (virMutexInit(&priv->lock) < 0) {
-            error(conn, VIR_ERR_INTERNAL_ERROR,
-                  _("cannot initialize mutex"));
-            VIR_FREE(priv);
-            return VIR_DRV_OPEN_ERROR;
-        }
-        if (flags & VIR_CONNECT_RO)
-            rflags |= VIR_DRV_OPEN_REMOTE_RO;
-        rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
-
-        priv->sock = -1;
-        ret = doRemoteOpen(conn, priv, auth, rflags);
-        if (ret != VIR_DRV_OPEN_SUCCESS) {
-            conn->storagePrivateData = NULL;
-            VIR_FREE(priv);
-        } else {
-            priv->localUses = 1;
+        int ret;
+        ret = remoteOpenSecondaryDriver(conn,
+                                        auth,
+                                        flags,
+                                        &priv);
+        if (ret == VIR_DRV_OPEN_SUCCESS)
             conn->storagePrivateData = priv;
-        }
         return ret;
     }
 }
@@ -4551,30 +4555,13 @@ remoteDevMonOpen(virConnectPtr conn,
          * which doesn't have its own impl of the network APIs.
          */
         struct private_data *priv;
-        int ret, rflags = 0;
-        if (VIR_ALLOC(priv) < 0) {
-            virReportOOMError (NULL);
-            return VIR_DRV_OPEN_ERROR;
-        }
-        if (virMutexInit(&priv->lock) < 0) {
-            error(conn, VIR_ERR_INTERNAL_ERROR,
-                  _("cannot initialize mutex"));
-            VIR_FREE(priv);
-            return VIR_DRV_OPEN_ERROR;
-        }
-        if (flags & VIR_CONNECT_RO)
-            rflags |= VIR_DRV_OPEN_REMOTE_RO;
-        rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
-
-        priv->sock = -1;
-        ret = doRemoteOpen(conn, priv, auth, rflags);
-        if (ret != VIR_DRV_OPEN_SUCCESS) {
-            conn->devMonPrivateData = NULL;
-            VIR_FREE(priv);
-        } else {
-            priv->localUses = 1;
+        int ret;
+        ret = remoteOpenSecondaryDriver(conn,
+                                        auth,
+                                        flags,
+                                        &priv);
+        if (ret == VIR_DRV_OPEN_SUCCESS)
             conn->devMonPrivateData = priv;
-        }
         return ret;
     }
 }
@@ -6429,6 +6416,7 @@ cleanup:
             thiscall->err.domain == VIR_FROM_REMOTE &&
             thiscall->err.code == VIR_ERR_RPC &&
             thiscall->err.level == VIR_ERR_ERROR &&
+            thiscall->err.message &&
             STRPREFIX(*thiscall->err.message, "unknown procedure")) {
             rv = -2;
         } else {
@@ -6436,6 +6424,7 @@ cleanup:
                           &thiscall->err);
             rv = -1;
         }
+        xdr_free((xdrproc_t)xdr_remote_error,  (char *)&thiscall->err);
     } else {
         rv = 0;
     }

diff --git a/src/xen_unified.c b/src/xen_unified.c
index e70c8acd78995af41ca32a5d51e62cad07b547e9..eefdb6c15c684f73e2a0358c248f37690f513d2b 100644 (file)
--- a/src/xen_unified.c
+++ b/src/xen_unified.c
@@ -355,11 +355,13 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, int flags)
     }
 
 #if WITH_XEN_INOTIFY
-    DEBUG0("Trying Xen inotify sub-driver");
-    if (drivers[XEN_UNIFIED_INOTIFY_OFFSET]->open(conn, auth, flags) ==
-        VIR_DRV_OPEN_SUCCESS) {
-        DEBUG0("Activated Xen inotify sub-driver");
-        priv->opened[XEN_UNIFIED_INOTIFY_OFFSET] = 1;
+    if (xenHavePrivilege()) {
+        DEBUG0("Trying Xen inotify sub-driver");
+        if (drivers[XEN_UNIFIED_INOTIFY_OFFSET]->open(conn, auth, flags) ==
+            VIR_DRV_OPEN_SUCCESS) {
+            DEBUG0("Activated Xen inotify sub-driver");
+            priv->opened[XEN_UNIFIED_INOTIFY_OFFSET] = 1;
+        }
     }
 #endif
 

diff --git a/src/xend_internal.c b/src/xend_internal.c
index a47214663a521498aa3a0d2979d33d437742e778..aca22d25a641921edc03eee29cf133b31ae93057 100644 (file)
--- a/src/xend_internal.c
+++ b/src/xend_internal.c
@@ -840,9 +840,11 @@ xenDaemonOpen_tcp(virConnectPtr conn, const char *host, const char *port)
     freeaddrinfo (res);
 
     if (!priv->addrlen) {
-        virReportSystemError(conn, saved_errno,
-                             _("unable to connect to '%s:%s'"),
-                             host, port);
+        /* Don't raise error when unprivileged, since proxy takes over */
+        if (xenHavePrivilege())
+            virReportSystemError(conn, saved_errno,
+                                 _("unable to connect to '%s:%s'"),
+                                 host, port);
         return -1;
     }
 
@@ -2733,7 +2735,6 @@ error:
  * @flags: combination of virDrvOpenFlag(s)
  *
  * Creates a localhost Xen Daemon connection
- * Note: this doesn't try to check if the connection actually works
  *
  * Returns 0 in case of success, -1 in case of error.
  */
@@ -2742,7 +2743,8 @@ xenDaemonOpen(virConnectPtr conn,
               virConnectAuthPtr auth ATTRIBUTE_UNUSED,
               int flags ATTRIBUTE_UNUSED)
 {
-    int ret;
+    char *port = NULL;
+    int ret = VIR_DRV_OPEN_ERROR;
 
     /* Switch on the scheme, which we expect to be NULL (file),
      * "http" or "xen".
@@ -2753,45 +2755,30 @@ xenDaemonOpen(virConnectPtr conn,
             virXendError(NULL, VIR_ERR_NO_CONNECT, __FUNCTION__);
             goto failed;
         }
-        ret = xenDaemonOpen_unix(conn, conn->uri->path);
-        if (ret < 0)
-            goto failed;
-
-        ret = xend_detect_config_version(conn);
-        if (ret == -1)
+        if (xenDaemonOpen_unix(conn, conn->uri->path) < 0 ||
+            xend_detect_config_version(conn) == -1)
             goto failed;
     }
     else if (STRCASEEQ (conn->uri->scheme, "xen")) {
         /*
          * try first to open the unix socket
          */
-        ret = xenDaemonOpen_unix(conn, "/var/lib/xend/xend-socket");
-        if (ret < 0)
-            goto try_http;
-        ret = xend_detect_config_version(conn);
-        if (ret != -1)
+        if (xenDaemonOpen_unix(conn, "/var/lib/xend/xend-socket") == 0 &&
+            xend_detect_config_version(conn) != -1)
             goto done;
 
-    try_http:
         /*
          * try though http on port 8000
          */
-        ret = xenDaemonOpen_tcp(conn, "localhost", "8000");
-        if (ret < 0)
-            goto failed;
-        ret = xend_detect_config_version(conn);
-        if (ret == -1)
+        if (xenDaemonOpen_tcp(conn, "localhost", "8000") < 0 ||
+            xend_detect_config_version(conn) == -1)
             goto failed;
     } else if (STRCASEEQ (conn->uri->scheme, "http")) {
-        char *port;
         if (virAsprintf(&port, "%d", conn->uri->port) == -1)
             goto failed;
-        ret = xenDaemonOpen_tcp(conn, conn->uri->server, port);
-        VIR_FREE(port);
-        if (ret < 0)
-            goto failed;
-        ret = xend_detect_config_version(conn);
-        if (ret == -1)
+
+        if (xenDaemonOpen_tcp(conn, conn->uri->server, port) < 0 ||
+            xend_detect_config_version(conn) == -1)
             goto failed;
     } else {
         virXendError(NULL, VIR_ERR_NO_CONNECT, __FUNCTION__);
@@ -2799,10 +2786,11 @@ xenDaemonOpen(virConnectPtr conn,
     }
 
  done:
-    return(ret);
+    ret = VIR_DRV_OPEN_SUCCESS;
 
 failed:
-    return(-1);
+    VIR_FREE(port);
+    return ret;
 }
 
 

diff --git a/src/xs_internal.c b/src/xs_internal.c
index c7087ed57423f0ef8fe151c6e3e25825e69948df..9e6b80f08abb0f1fc9bfc097e1d2de1dae815a3e 100644 (file)
--- a/src/xs_internal.c
+++ b/src/xs_internal.c
@@ -599,8 +599,6 @@ xenStoreDoListDomains(xenUnifiedPrivatePtr priv, int *ids, int maxids)
         ids[ret++] = (int) id;
     }
 
-    free(idlist);
-
 out:
     VIR_FREE (idlist);
     return ret;