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

2008-03-17 Thread Daniel Veillard 
On Mon, Mar 17, 2008 at 04:02:06PM +, Daniel P. Berrange wrote:
> On Tue, Mar 11, 2008 at 06:47:47AM -0400, Daniel Veillard wrote:
> > On Mon, Mar 10, 2008 at 07:09:36PM +, 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

  Okidoc, +1 :-)

thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard  | virtualization library  http://libvirt.org/
[EMAIL PROTECTED]  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

--
Libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list

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

2008-03-17 Thread Daniel P. Berrange 
On Tue, Mar 11, 2008 at 06:47:47AM -0400, Daniel Veillard wrote:
> On Mon, Mar 10, 2008 at 07:09:36PM +, 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.in11 Mar 2008 14:49:04 -  1.134
+++ configure.in17 Mar 2008 15:52:58 -
@@ -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 -  1.62
+++ src/remote_internal.c   17 Mar 2008 15:52:58 -
@@ -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 -  1.38
+++ src/xen_unified.c   17 Mar 2008 15:52:58 -
@@ -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",

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

2008-03-11 Thread Daniel Veillard 
On Mon, Mar 10, 2008 at 07:09:36PM +, 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...

>  - 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,

>  non-root: (XenD)
>  non-root: (XenD + XS)
>  non-root: (proxy)
> 
> If non-root, and the proxy is not compiled in, we'll hand off to the remote
> driver. Any other scenario will result in an explicit fail.

  okay except for the exception I sugegst to add back,

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard  | virtualization library  http://libvirt.org/
[EMAIL PROTECTED]  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

--
Libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list

[Libvir] PATCH: Fix xen unified driver open logic

2008-03-10 Thread Daniel P. Berrange 
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

 - 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)
 non-root: (XenD)
 non-root: (XenD + XS)
 non-root: (proxy)

If non-root, and the proxy is not compiled in, we'll hand off to the remote
driver. Any other scenario will result in an explicit fail.

Dan.

? docs/apibuild.pyc
Index: configure.in
===
RCS file: /data/cvs/libvirt/configure.in,v
retrieving revision 1.133
diff -u -p -r1.133 configure.in
--- configure.in3 Mar 2008 14:42:37 -   1.133
+++ configure.in10 Mar 2008 18:59:23 -
@@ -873,6 +873,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.61
diff -u -p -r1.61 remote_internal.c
--- src/remote_internal.c   26 Feb 2008 07:05:18 -  1.61
+++ src/remote_internal.c   10 Mar 2008 18:59:24 -
@@ -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 -  1.38
+++ src/xen_unified.c   10 Mar 2008 18:59:24 -
@@ -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 +242,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 +279,69 @@ 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)
+goto fail;