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

Re: [Libvir] State of driver backend infrastructure



On Mon, Jun 12, 2006 at 06:52:57PM -0400, Daniel Veillard wrote:
> On Mon, Jun 12, 2006 at 11:19:00PM +0100, Daniel P. Berrange wrote:
> >  * Driver method hookup. The libvirt.c file has alot of methods which are
> >    either not hooked up to the driver backend, or hooked up, but still have
> >    duplicated (redundant?) Xen specific calls. Here is the complete status:
> 
>   Okay, I need to clean them up, please bugzilla this I will fix this soonish.
> 
> >     virDomainDestroy
> >     
> >      - Does not call out to driver backends, hardcodes XenD & Xen HV
> >     
> 
>   bad need fixing to call the driver entry point.
> 
> >     virDomainSuspend
> >     
> >      - Does not call out to driver backends, hardcodes XenD & Xen HV
> >     
> > 
>   same as previous
> 
> >     virDomainResume
> >     
> >      - Does not call out to driver backends, hardcodes XenD & Xen HV
> > 
>   same as previous
> 
> >     virDomainShutdown
> >     
> >      - Does not call out to driver backends, hardcodes XenD
> > 
>   same as previous
> 
> >     
> >     virDomainReboot
> >     
> >      - Does not call out to driver backends, hardcodes XenD
> > 
>   same as previous

I'm attaching an initial patch which implements the driver backend calls
for these five methods.	

I've tested that suspend & resume work, but my hardware died before I fully
tested the other methods. I've got a couple questions before proceeding in
any case:

 1. In all of these methods I followed example from virDomainSetMemory and
    put in

    if (domain->conn->flags & VIR_CONNECT_RO)
         return (-1);

    This prevents these methods working with a 'read only' connection, however,
    this is a deviation from previous semantics. Even with a read only connection,
    XenD will let arbitrary unprivileged user shutdown/suspend/resume/etc a
    domain, so by putting this VIR_CONNECT_RO check in we'd be preventing an 
    operation which used to work. 

    However, IMHO this is a good thing, because it can be considered a *HUGE*
    security hole / denial of service attack, that unprivileged users can shutdown
    reboot, suspend, etc  domains via XenD with no authentication. When this hole
    is eventually plugged, these methods would cease to work with a read only 
    connection, so long term we'd end up in the same situation. Thus this patch
    could be considered a 'pre-emptive' solution.

 2. The current implementation of shutdown & reboot methods calls the same code
    twice in a succession:
 
    /*
     * try first with the xend daemon
     */
    ret = xenDaemonDomainShutdown(domain);
    if (ret == 0) {
        domain->flags |= DOMAIN_IS_SHUTDOWN;
        return (0);
    }

    /*
     * this is very hackish, the domU kernel probes for a special 
     * node in the xenstore and launch the shutdown command if found.
     */
    ret = xenDaemonDomainShutdown(domain);
    if (ret == 0) {
        domain->flags |= DOMAIN_IS_SHUTDOWN;
    }


    What was the reason to call xenDaemonDomainShutdown  twice ? With my
    change to use the driver backends, it will only be called once. So I want
    to make sure I'm not missing an edge case here.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
Index: src/libvirt.c
===================================================================
RCS file: /data/cvs/libvirt/src/libvirt.c,v
retrieving revision 1.29
diff -u -r1.29 libvirt.c
--- src/libvirt.c	14 Jun 2006 13:03:04 -0000	1.29
+++ src/libvirt.c	14 Jun 2006 13:27:24 -0000
@@ -880,28 +880,33 @@
 int
 virDomainDestroy(virDomainPtr domain)
 {
-    int ret;
+    int ret = -1, i;
+    virConnectPtr conn;
 
     if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
         virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
         return (-1);
     }
 
-    /*
-     * try first with the xend method
-     */
-    ret = xenDaemonDomainDestroy(domain);
-    if (ret == 0) {
-        virDomainFree(domain);
-        return (0);
-    }
-
-    ret = xenHypervisorDestroyDomain(domain);
-    if (ret < 0)
+    conn = domain->conn;
+    if (domain->conn->flags & VIR_CONNECT_RO)
         return (-1);
 
-    virDomainFree(domain);
-    return (0);
+    /* Go though the driver registered entry points */
+    for (i = 0;i < conn->nb_drivers;i++) {
+	if ((conn->drivers[i] != NULL) &&
+	    (conn->drivers[i]->domainDestroy != NULL)) {
+	    if (conn->drivers[i]->domainDestroy(domain) == 0) 
+	        ret = 0;
+	}
+    }
+    
+    if (ret != 0) {
+        virLibConnError(conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
+        return (ret);
+    }
+
+    return (ret);
 }
 
 /**
@@ -940,20 +945,33 @@
 int
 virDomainSuspend(virDomainPtr domain)
 {
-    int ret;
+    int ret = -1, i;
+    virConnectPtr conn;
 
     if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
         virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
         return (-1);
     }
 
-    /* first try though the Xen daemon */
-    ret = xenDaemonDomainSuspend(domain);
-    if (ret == 0)
-        return (0);
+    conn = domain->conn;
+    if (domain->conn->flags & VIR_CONNECT_RO)
+        return (-1);
+ 
+    /* Go though the driver registered entry points */
+    for (i = 0;i < conn->nb_drivers;i++) {
+	if ((conn->drivers[i] != NULL) &&
+	    (conn->drivers[i]->domainSuspend != NULL)) {
+	    if (conn->drivers[i]->domainSuspend(domain) == 0) 
+	        ret = 0;
+	}
+    }
+    
+    if (ret != 0) {
+        virLibConnError(conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
+        return (ret);
+    }
 
-    /* then try a direct hypervisor access */
-    return (xenHypervisorPauseDomain(domain));
+    return (ret);
 }
 
 /**
@@ -969,20 +987,33 @@
 int
 virDomainResume(virDomainPtr domain)
 {
-    int ret;
+    int ret = -1, i;
+    virConnectPtr conn;
 
     if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
         virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
         return (-1);
     }
 
-    /* first try though the Xen daemon */
-    ret = xenDaemonDomainResume(domain);
-    if (ret == 0)
-        return (0);
+    conn = domain->conn;
+    if (domain->conn->flags & VIR_CONNECT_RO)
+        return (-1);
 
-    /* then try a direct hypervisor access */
-    return (xenHypervisorResumeDomain(domain));
+    /* Go though the driver registered entry points */
+    for (i = 0;i < conn->nb_drivers;i++) {
+	if ((conn->drivers[i] != NULL) &&
+	    (conn->drivers[i]->domainResume != NULL)) {
+	    if (conn->drivers[i]->domainResume(domain) == 0) 
+	        ret = 0;
+	}
+    }
+    
+    if (ret != 0) {
+        virLibConnError(conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
+        return (ret);
+    }
+
+    return (ret);
 }
 
 /**
@@ -1099,30 +1130,32 @@
 int
 virDomainShutdown(virDomainPtr domain)
 {
-    int ret;
+    int ret = -1, i;
+    virConnectPtr conn;
 
     if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
         virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
         return (-1);
     }
 
-    /*
-     * try first with the xend daemon
-     */
-    ret = xenDaemonDomainShutdown(domain);
-    if (ret == 0) {
-        domain->flags |= DOMAIN_IS_SHUTDOWN;
-        return (0);
-    }
+    conn = domain->conn;
+    if (domain->conn->flags & VIR_CONNECT_RO)
+        return (-1);
 
-    /*
-     * this is very hackish, the domU kernel probes for a special 
-     * node in the xenstore and launch the shutdown command if found.
-     */
-    ret = xenDaemonDomainShutdown(domain);
-    if (ret == 0) {
-        domain->flags |= DOMAIN_IS_SHUTDOWN;
+    /* Go though the driver registered entry points */
+    for (i = 0;i < conn->nb_drivers;i++) {
+	if ((conn->drivers[i] != NULL) &&
+	    (conn->drivers[i]->domainShutdown != NULL)) {
+	    if (conn->drivers[i]->domainShutdown(domain) == 0) 
+	        ret = 0;
+	}
     }
+    
+    if (ret != 0) {
+        virLibConnError(conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
+        return (ret);
+    }
+
     return (ret);
 }
 
@@ -1140,30 +1173,32 @@
 int
 virDomainReboot(virDomainPtr domain, unsigned int flags)
 {
-    int ret;
+    int ret = -1, i;
+    virConnectPtr conn;
 
     if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
         virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
         return (-1);
     }
 
-    /*
-     * try first with the xend daemon
-     */
-    ret = xenDaemonDomainReboot(domain, flags);
-    if (ret == 0) {
-        domain->flags |= DOMAIN_IS_SHUTDOWN;
-        return (0);
-    }
+    conn = domain->conn;
+    if (domain->conn->flags & VIR_CONNECT_RO)
+        return (-1);
 
-    /*
-     * this is very hackish, the domU kernel probes for a special 
-     * node in the xenstore and launch the shutdown command if found.
-     */
-    ret = xenDaemonDomainReboot(domain, flags);
-    if (ret == 0) {
-        domain->flags |= DOMAIN_IS_SHUTDOWN;
+    /* Go though the driver registered entry points */
+    for (i = 0;i < conn->nb_drivers;i++) {
+	if ((conn->drivers[i] != NULL) &&
+	    (conn->drivers[i]->domainReboot != NULL)) {
+	    if (conn->drivers[i]->domainReboot(domain, flags) == 0) 
+	        ret = 0;
+	}
     }
+    
+    if (ret != 0) {
+        virLibConnError(conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
+        return (ret);
+    }
+
     return (ret);
 }
 

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