[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH: Fix xen unified driver open logic



On Tue, Mar 11, 2008 at 06:47:47AM -0400, Daniel Veillard wrote:
> On Mon, Mar 10, 2008 at 07:09:36PM +0000, Daniel P. Berrange wrote:
> > When adding PolicyKit support we disabled the proxy driver, but did not
> > correctly fix up the Xen unified driver. The result is that it is still
> > trying to run the proxy setuid helper which doesn't exist and thus it fails
> > the open operation before the remote driver gets the opportunity to process
> > the URI. I attempted to fix this by just disabling the proxy driver in the
> > unified driver, but came to the conclusion the logic of the current code is
> > just not flexible enough for what we need to be able todo  these days.
> > 
> > THe core problem is the 'for(;;)' loop iterating over the drivers - it
> > already has several special cases in the loop body to skip drivers, or
> > ignore errors and adding more special cases is making my mind hurt trying
> > to trace the logic.
> > 
> > So I have removed the loop, and encode the desired logic explicitly. The
> > diff a little unpleasant to read, so to summarize the logic is thus:
> > 
> >  - If root only, try open the hypervisor driver
> >          -> Failure to open is fatal, do not try other drivers
> 
>   hum, I'm not 100% sure of that, an old libvirt version might still be
> able to work though xend in face of an hypervisor change it can't handle,
> we had the problem for example with 0.4.0 on xen-3.2, there was side effects
> but it was basically working without hypervisor access...

This attached patch adds that use case too. Failure of the HV driver is
now non-fatal. The other rules below are unchagned

> 
> >  - Try to open the XenD driver
> >       - If XenD suceeds
> >           -> If XenD < 3.0.4, then open the XM driver for inactive domains
> >           -> Try to open the XS driver
> >                    => Failure to open is fatal if root
> >       - Else XenD fails
> >           ->.If proxy is compiled in, try to open proxy
> >                 => Failure to open is fatal
> > 
> > 
> > This should result in one of the following combinations of drivers being
> > activated:
> > 
> >  root: (HV + XenD + XS)
> >  root: (HV + XenD + XS + XM)
> 
>   root: (XenD + XS [+XM]) should still be allowed IMHO,

Dan.

Index: configure.in
===================================================================
RCS file: /data/cvs/libvirt/configure.in,v
retrieving revision 1.134
diff -u -p -r1.134 configure.in
--- configure.in	11 Mar 2008 14:49:04 -0000	1.134
+++ configure.in	17 Mar 2008 15:52:58 -0000
@@ -869,6 +869,9 @@ fi
 AC_MSG_RESULT([$with_xen_proxy])
 
 AM_CONDITIONAL(WITH_PROXY,[test "$with_xen_proxy" = "yes"])
+if test "$with_xen_proxy" = "yes"; then
+  AC_DEFINE(WITH_PROXY, 1, [Whether Xen proxy is enabled])
+fi
 
 dnl Enable building libvirtd?
 AM_CONDITIONAL(WITH_LIBVIRTD,[test "x$with_libvirtd" = "xyes"])
Index: src/remote_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/remote_internal.c,v
retrieving revision 1.62
diff -u -p -r1.62 remote_internal.c
--- src/remote_internal.c	17 Mar 2008 10:27:32 -0000	1.62
+++ src/remote_internal.c	17 Mar 2008 15:52:58 -0000
@@ -835,6 +835,14 @@ remoteOpen (virConnectPtr conn,
         }
     }
 #endif
+#if WITH_XEN
+    if (uri &&
+        uri->scheme && STREQ (uri->scheme, "xen") &&
+        (!uri->server || STREQ (uri->server, "")) &&
+        (!uri->path || STREQ(uri->path, "/"))) {
+        rflags |= VIR_DRV_OPEN_REMOTE_UNIX;
+    }
+#endif
 
     priv->magic = DEAD;
     priv->sock = -1;
Index: src/xen_unified.c
===================================================================
RCS file: /data/cvs/libvirt/src/xen_unified.c,v
retrieving revision 1.38
diff -u -p -r1.38 xen_unified.c
--- src/xen_unified.c	27 Feb 2008 10:37:19 -0000	1.38
+++ src/xen_unified.c	17 Mar 2008 15:52:58 -0000
@@ -42,6 +42,7 @@
 #include "util.h"
 
 #define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__)
+#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
 
 static int
 xenUnifiedNodeGetInfo (virConnectPtr conn, virNodeInfoPtr info);
@@ -239,7 +240,7 @@ xenUnifiedProbe (void)
 static int
 xenUnifiedOpen (virConnectPtr conn, xmlURIPtr uri, virConnectAuthPtr auth, int flags)
 {
-    int i, j;
+    int i;
     xenUnifiedPrivatePtr priv;
 
     /* Refuse any scheme which isn't "xen://" or "http://";. */
@@ -276,41 +277,73 @@ xenUnifiedOpen (virConnectPtr conn, xmlU
     priv->xshandle = NULL;
     priv->proxy = -1;
 
-    for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) {
-        priv->opened[i] = 0;
 
-        /* Only use XM driver for Xen <= 3.0.3 (ie xendConfigVersion <= 2) */
-        if (drivers[i] == &xenXMDriver &&
-            priv->xendConfigVersion > 2)
-            continue;
-
-        /* Ignore proxy for root */
-        if (i == XEN_UNIFIED_PROXY_OFFSET && getuid() == 0)
-            continue;
-
-        if (drivers[i]->open) {
-            DEBUG("trying Xen sub-driver %d", i);
-            if (drivers[i]->open (conn, uri, auth, flags) == VIR_DRV_OPEN_SUCCESS)
-                priv->opened[i] = 1;
-            DEBUG("Xen sub-driver %d open %s\n",
-                  i, priv->opened[i] ? "ok" : "failed");
+    /* Hypervisor is only run as root & required to succeed */
+    if (getuid() == 0) {
+        DEBUG0("Trying hypervisor sub-driver");
+        if (drivers[XEN_UNIFIED_HYPERVISOR_OFFSET]->open(conn, uri, auth, flags) ==
+            VIR_DRV_OPEN_SUCCESS) {
+            DEBUG0("Activated hypervisor sub-driver");
+            priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET] = 1;
         }
+    }
 
-        /* If as root, then all drivers must succeed.
-           If non-root, then only proxy must succeed */
-        if (!priv->opened[i] &&
-            (getuid() == 0 || i == XEN_UNIFIED_PROXY_OFFSET)) {
-            for (j = 0; j < i; ++j)
-                if (priv->opened[j]) drivers[j]->close (conn);
-            free (priv);
-            /* The assumption is that one of the underlying drivers
-             * has set virterror already.
-             */
-            return VIR_DRV_OPEN_ERROR;
+    /* XenD is required to suceed if root.
+     * If it fails as non-root, then the proxy driver may take over 
+     */
+    DEBUG0("Trying XenD sub-driver");
+    if (drivers[XEN_UNIFIED_XEND_OFFSET]->open(conn, uri, auth, flags) ==
+        VIR_DRV_OPEN_SUCCESS) {
+        DEBUG0("Activated XenD sub-driver");
+        priv->opened[XEN_UNIFIED_XEND_OFFSET] = 1;
+
+        /* XenD is active, so try the xm & xs drivers too, both requird to
+         * succeed if root, optional otherwise */
+        if (priv->xendConfigVersion <= 2) {
+            DEBUG0("Trying XM sub-driver");
+            if (drivers[XEN_UNIFIED_XM_OFFSET]->open(conn, uri, auth, flags) ==
+                VIR_DRV_OPEN_SUCCESS) {
+                DEBUG0("Activated XM sub-driver");
+                priv->opened[XEN_UNIFIED_XM_OFFSET] = 1;
+            }
+        }
+        DEBUG0("Trying XS sub-driver");
+        if (drivers[XEN_UNIFIED_XS_OFFSET]->open(conn, uri, auth, flags) ==
+            VIR_DRV_OPEN_SUCCESS) {
+            DEBUG0("Activated XS sub-driver");
+            priv->opened[XEN_UNIFIED_XS_OFFSET] = 1;
+        } else {
+            if (getuid() == 0)
+                goto fail; /* XS is mandatory as root */
+        }
+    } else {
+        if (getuid() == 0) {
+            goto fail; /* XenD is mandatory as root */
+        } else {
+#if WITH_PROXY
+            DEBUG0("Trying proxy sub-driver");
+            if (drivers[XEN_UNIFIED_PROXY_OFFSET]->open(conn, uri, auth, flags) ==
+                VIR_DRV_OPEN_SUCCESS) {
+                DEBUG0("Activated proxy sub-driver");
+                priv->opened[XEN_UNIFIED_PROXY_OFFSET] = 1;
+            } else {
+                goto fail; /* Proxy is mandatory if XenD failed */
+            }
+#else
+            DEBUG0("Handing off for remote driver");
+            return VIR_DRV_OPEN_DECLINED; /* Let remote_driver try instead */
+#endif
         }
     }
 
     return VIR_DRV_OPEN_SUCCESS;
+
+ fail:
+    DEBUG0("Failed to activate a mandatory sub-driver");
+    for (i = 0 ; i < XEN_UNIFIED_NR_DRIVERS ; i++)
+        if (priv->opened[i]) drivers[i]->close(conn);
+    free(priv);
+    return VIR_DRV_OPEN_ERROR;
 }
 
 #define GET_PRIVATE(conn) \
Index: src/xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.174
diff -u -p -r1.174 xend_internal.c
--- src/xend_internal.c	17 Mar 2008 10:27:32 -0000	1.174
+++ src/xend_internal.c	17 Mar 2008 15:52:59 -0000
@@ -234,14 +234,13 @@ do_connect(virConnectPtr xend)
         close(s);
         errno = serrno;
         s = -1;
-	/*
-	 * not being able to connect via the socket as a normal user
-	 * is rather normal, this should fallback to the proxy (or
-	 * remote) mechanism.
-	 */
-	if ((getuid() == 0) || (xend->flags & VIR_CONNECT_RO)) {
-	    virXendError(xend, VIR_ERR_INTERNAL_ERROR,
-                     _("failed to connect to xend"));
+
+        /*
+         * Connecting to XenD as root is mandatory, so log this error
+         */
+        if (getuid() == 0) {
+            virXendError(xend, VIR_ERR_INTERNAL_ERROR,
+                         _("failed to connect to xend"));
         }
     }
 


-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]