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

Re: [Libvir] [PATCH] check the maximum of virtual CPU



Hi Dan

> This looks like a bug in XenD that should be reported upstrem. If the hypercall
> is given an invalid value it should reject it and not screw up the whole host.
> 
I agree.
I will consider it as a back log.

> If we add this against the virConnectPtr object, we should name it 
> 
>     virConnectGetVcpuMax()
> 
> For consistency with other VCPU method naming.  I wonder though, if we should
> 
I contribute the patch that corrects the following again.
 ・ Correction of name of method and argument
    --> Isn't the name bad?
 ・ Correction of position of method


This patch is over 200lines.
If you request, I am ready to repost it with split this patch.

Signed-off-by: Masayuki Sunou <fj1826dm aa jp fujitsu com>

Thanks,
Masayuki Sunou.


libvirt-0.2.0
----------------------------------------------------------------------
diff -uprN a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h
--- a/include/libvirt/libvirt.h	2007-02-23 17:51:30.000000000 +0900
+++ b/include/libvirt/libvirt.h	2007-03-03 00:07:15.000000000 +0900
@@ -310,6 +310,8 @@ int			virDomainSetMaxMemory	(virDomainPt
 						 unsigned long memory);
 int			virDomainSetMemory	(virDomainPtr domain,
 						 unsigned long memory);
+int			virDomainGetMaxVcpus	(virDomainPtr domain);
+
 /*
  * XML domain description
  */
diff -uprN a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
--- a/include/libvirt/libvirt.h.in	2007-02-23 17:51:30.000000000 +0900
+++ b/include/libvirt/libvirt.h.in	2007-03-02 20:37:55.000000000 +0900
@@ -310,6 +310,8 @@ int			virDomainSetMaxMemory	(virDomainPt
 						 unsigned long memory);
 int			virDomainSetMemory	(virDomainPtr domain,
 						 unsigned long memory);
+int			virDomainGetMaxVcpus	(virDomainPtr domain);
+
 /*
  * XML domain description
  */
diff -uprN a/src/driver.h b/src/driver.h
--- a/src/driver.h	2007-02-23 17:51:30.000000000 +0900
+++ b/src/driver.h	2007-03-02 23:33:20.000000000 +0900
@@ -129,6 +129,8 @@ typedef int
 					 unsigned char *cpumaps,
 					 int maplen);
 typedef int
+	(*virDrvDomainGetMaxVcpus)	(virDomainPtr domain);
+typedef int
 	(*virDrvDomainAttachDevice)	(virDomainPtr domain,
 					 char *xml);
 typedef int
@@ -181,6 +183,7 @@ struct _virDriver {
 	virDrvDomainSetVcpus		domainSetVcpus;
 	virDrvDomainPinVcpu		domainPinVcpu;
 	virDrvDomainGetVcpus		domainGetVcpus;
+	virDrvDomainGetMaxVcpus		domainGetMaxVcpus;
 	virDrvDomainDumpXML		domainDumpXML;
 	virDrvListDefinedDomains	listDefinedDomains;
 	virDrvNumOfDefinedDomains	numOfDefinedDomains;
diff -uprN a/src/libvirt.c b/src/libvirt.c
--- a/src/libvirt.c	2007-02-23 17:51:30.000000000 +0900
+++ b/src/libvirt.c	2007-03-03 00:10:50.000000000 +0900
@@ -2110,6 +2110,40 @@ virDomainGetVcpus(virDomainPtr domain, v
 }
 
 /**
+ * virDomainGetMaxVcpus:
+ * @domain: pointer to domain object
+ * 
+ *  Returns the maximum of virtual CPU of Domain.
+ *
+ * Returns the maximum of virtual CPU or 0 in case of error.
+ */
+int
+virDomainGetMaxVcpus(virDomainPtr domain) {
+    int i;
+    int ret = 0;
+    virConnectPtr conn;
+
+    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
+        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+        return (0);
+    }
+
+    conn = domain->conn;
+
+    for (i = 0;i < conn->nb_drivers;i++) {
+	if ((conn->drivers[i] != NULL) &&
+	    (conn->drivers[i]->domainGetMaxVcpus != NULL)) {
+	    ret = conn->drivers[i]->domainGetMaxVcpus(domain);
+	    if (ret != 0)
+	        return(ret);
+	}
+    }
+    virLibConnError(conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
+    return (-1);
+}
+
+
+/**
  * virDomainAttachDevice:
  * @domain: pointer to domain object
  * @xml: pointer to XML description of one device
diff -uprN a/src/libvirt_sym.version b/src/libvirt_sym.version
--- a/src/libvirt_sym.version	2007-02-23 17:51:30.000000000 +0900
+++ b/src/libvirt_sym.version	2007-03-02 20:38:53.000000000 +0900
@@ -56,6 +56,7 @@
 	virDomainSetVcpus;
 	virDomainPinVcpu;
 	virDomainGetVcpus;
+	virDomainGetMaxVcpus;
 
     virDomainAttachDevice;
     virDomainDetachDevice;
diff -uprN a/src/proxy_internal.c b/src/proxy_internal.c
--- a/src/proxy_internal.c	2007-02-23 17:51:30.000000000 +0900
+++ b/src/proxy_internal.c	2007-03-02 18:42:42.000000000 +0900
@@ -73,6 +73,7 @@ static virDriver xenProxyDriver = {
     NULL, /* domainSetVcpus */
     NULL, /* domainPinVcpu */
     NULL, /* domainGetVcpus */
+    NULL, /* domainGetMaxVcpus */
     xenProxyDomainDumpXML, /* domainDumpXML */
     NULL, /* listDefinedDomains */
     NULL, /* numOfDefinedDomains */
diff -uprN a/src/qemu_internal.c b/src/qemu_internal.c
--- a/src/qemu_internal.c	2007-02-23 21:46:35.000000000 +0900
+++ b/src/qemu_internal.c	2007-03-02 23:34:08.000000000 +0900
@@ -1190,6 +1190,7 @@ static virDriver qemuDriver = {
     NULL, /* domainSetVcpus */
     NULL, /* domainPinVcpu */
     NULL, /* domainGetVcpus */
+    NULL, /* domainGetMaxVcpus */
     qemuDomainDumpXML, /* domainDumpXML */
     qemuListDefinedDomains, /* listDomains */
     qemuNumOfDefinedDomains, /* numOfDomains */
diff -uprN a/src/test.c b/src/test.c
--- a/src/test.c	2007-02-23 17:51:30.000000000 +0900
+++ b/src/test.c	2007-03-02 23:36:07.000000000 +0900
@@ -117,6 +117,7 @@ static virDriver testDriver = {
     testSetVcpus, /* domainSetVcpus */
     NULL, /* domainPinVcpu */
     NULL, /* domainGetVcpus */
+    NULL, /* domainGetMaxVcpus */
     testDomainDumpXML, /* domainDumpXML */
     testListDefinedDomains, /* listDefinedDomains */
     testNumOfDefinedDomains, /* numOfDefinedDomains */
diff -uprN a/src/virsh.c b/src/virsh.c
--- a/src/virsh.c	2007-02-28 00:35:50.000000000 +0900
+++ b/src/virsh.c	2007-03-02 20:35:28.000000000 +0900
@@ -1383,6 +1383,7 @@ cmdSetvcpus(vshControl * ctl, vshCmd * c
 {
     virDomainPtr dom;
     int count;
+    int maxcpu;
     int ret = TRUE;
 
     if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
@@ -1397,6 +1398,18 @@ cmdSetvcpus(vshControl * ctl, vshCmd * c
         return FALSE;
     }
 
+    maxcpu = virDomainGetMaxVcpus(dom);
+    if (!maxcpu) {
+        virDomainFree(dom);
+        return FALSE;
+    }
+
+    if (count > maxcpu) {
+        vshError(ctl, FALSE, _("Too many virtual CPU's."));
+        virDomainFree(dom);
+        return FALSE;
+    }
+
     if (virDomainSetVcpus(dom, count) != 0) {
         ret = FALSE;
     }
diff -uprN a/src/xen_internal.c b/src/xen_internal.c
--- a/src/xen_internal.c	2007-02-23 17:51:30.000000000 +0900
+++ b/src/xen_internal.c	2004-07-03 05:43:41.000000000 +0900
@@ -446,6 +446,7 @@ static virDriver xenHypervisorDriver = {
     xenHypervisorSetVcpus, /* domainSetVcpus */
     xenHypervisorPinVcpu, /* domainPinVcpu */
     xenHypervisorGetVcpus, /* domainGetVcpus */
+    xenHypervisorGetVcpuMax, /* domainGetMaxVcpus */
     NULL, /* domainDumpXML */
     NULL, /* listDefinedDomains */
     NULL, /* numOfDefinedDomains */
@@ -1824,6 +1825,17 @@ xenHypervisorGetVcpus(virDomainPtr domai
 }
 #endif
 
+/**
+ * xend_get_cpu_max:
+ *
+ * Returns the maximum of CPU defined by Xen.
+ */
+int
+xenHypervisorGetVcpuMax(virDomainPtr domain)
+{
+    return MAX_VIRT_CPUS;
+}
+
 /*
  * Local variables:
  *  indent-tabs-mode: nil
diff -uprN a/src/xen_internal.h b/src/xen_internal.h
--- a/src/xen_internal.h	2006-08-04 19:41:05.000000000 +0900
+++ b/src/xen_internal.h	2004-07-03 05:44:03.000000000 +0900
@@ -55,6 +55,7 @@ int	xenHypervisorGetVcpus		(virDomainPtr
 					 int maxinfo,
 					 unsigned char *cpumaps,
 					 int maplen);
+int	xenHypervisorGetVcpuMax		(virDomainPtr domain);
 
 #ifdef __cplusplus
 }
diff -uprN a/src/xend_internal.c b/src/xend_internal.c
--- a/src/xend_internal.c	2007-03-02 08:24:09.000000000 +0900
+++ b/src/xend_internal.c	2007-03-02 23:34:44.000000000 +0900
@@ -89,6 +89,7 @@ static virDriver xenDaemonDriver = {
     xenDaemonDomainSetVcpus, /* domainSetVcpus */
     xenDaemonDomainPinVcpu, /* domainPinVcpu */
     xenDaemonDomainGetVcpus, /* domainGetVcpus */
+    NULL, /* domainGetMaxVcpus */
     xenDaemonDomainDumpXML, /* domainDumpXML */
     xenDaemonListDefinedDomains, /* listDefinedDomains */
     xenDaemonNumOfDefinedDomains, /* numOfDefinedDomains */
@@ -511,10 +512,10 @@ xend_post(virConnectPtr xend, const char
     } else if ((ret == 202) && (strstr(content, "failed") != NULL)) {
         virXendError(xend, VIR_ERR_POST_FAILED, content);
         ret = -1;
-    } else if (((ret >= 200) && (ret <= 202)) && (strstr(content, "xend.err") != NULL)) {
-        /* This is to catch case of things like 'virsh dump Domain-0 foo'
-         * which returns a success code, but the word 'xend.err'
-         * in body to indicate error :-(
+    } else if ((ret == 202) && (strstr(content, "Cannot") != NULL)) {
+        /* This is to catch case of 'virsh dump Domain-0 foo'
+         * which returns a success code, but the word 'Cannot'
+         * in body to indicate error
          */
         virXendError(xend, VIR_ERR_POST_FAILED, content);
         ret = -1;
diff -uprN a/src/xm_internal.c b/src/xm_internal.c
--- a/src/xm_internal.c	2007-02-23 17:51:30.000000000 +0900
+++ b/src/xm_internal.c	2007-03-02 23:35:29.000000000 +0900
@@ -98,6 +98,7 @@ static virDriver xenXMDriver = {
     xenXMDomainSetVcpus, /* domainSetVcpus */
     NULL, /* domainPinVcpu */
     NULL, /* domainGetVcpus */
+    NULL, /* domainGetMaxVcpus */
     xenXMDomainDumpXML, /* domainDumpXML */
     xenXMListDefinedDomains, /* listDefinedDomains */
     xenXMNumOfDefinedDomains, /* numOfDefinedDomains */
diff -uprN a/src/xs_internal.c b/src/xs_internal.c
--- a/src/xs_internal.c	2007-02-23 17:51:30.000000000 +0900
+++ b/src/xs_internal.c	2007-03-02 23:36:22.000000000 +0900
@@ -67,6 +67,7 @@ static virDriver xenStoreDriver = {
     NULL, /* domainSetVcpus */
     NULL, /* domainPinVcpu */
     NULL, /* domainGetVcpus */
+    NULL, /* domainGetMaxVcpus */
     NULL, /* domainDumpXML */
     NULL, /* listDefinedDomains */
     NULL, /* numOfDefinedDomains */
----------------------------------------------------------------------


In message <20070301200929 GK6079 redhat com>
   "Re: [Libvir] [PATCH] check the maximum of virtual CPU"
   ""Daniel P. Berrange" <berrange redhat com>" wrote:

> On Thu, Mar 01, 2007 at 10:02:03AM +0900, Masayuki Sunou wrote:
> > Hi
> > 
> > The maximum of virtual CPU is not guarded in virsh setvcpus now.
> > Then, when 32767 was set to virtual CPU of virsh setvcpus, the problem
> > that Xend became abnormal was detected.
> > 
> > example:
> > ----------------------------------------------------------------------
> >   # virsh setvcpus 0 32767
> >   libvir: Xen Daemon error : POST operation failed: (xend.err "(9, 'Bad file descriptor')")
> >   libvir: Xen error : failed Xen syscall  ioctl  3166208
> >   libvir: error : library call virDomainSetVcpus failed, possibly not supported
> >   
> >   # xm list -l
> >   Error: (9, 'Bad file descriptor')
> >   Usage: xm list [options] [Domain, ...]
> 
> This looks like a bug in XenD that should be reported upstrem. If the hypercall
> is given an invalid value it should reject it and not screw up the whole host.
> 
> > Therefore, I propose the correction that adjusts the maximum of virtual
> > CPU to the same value as Xen. 
> 
> Ordinarily I'd say just fix Xen, but given that there are many broken versions
> of Xen out there, I agree it is worth putting a max-vcpus check into libvirt to
> protect users.
> 
> > libvirt-0.2.0
> > ----------------------------------------------------------------------
> > diff -rup a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h
> > --- a/include/libvirt/libvirt.h	2007-02-23 17:51:30.000000000 +0900
> > +++ b/include/libvirt/libvirt.h	2007-03-01 03:02:20.000000000 +0900
> > @@ -233,6 +233,7 @@ int			virConnectGetVersion	(virConnectPt
> >  						 unsigned long *hvVer);
> >  int			virNodeGetInfo		(virConnectPtr conn,
> >  						 virNodeInfoPtr info);
> > +int			virGetCpuMax		(virConnectPtr conn);
> 
> If we add this against the virConnectPtr object, we should name it 
> 
>     virConnectGetVcpuMax()
> 
> For consistency with other VCPU method naming.  I wonder though, if we should
> instead make it a method which takes a virDomainPtr instead - semantically we
> are asking for the VCPU limit which can be applied to a domain. What do people
> think ?
> 
> 
> > diff -rup a/src/qemu_internal.c b/src/qemu_internal.c
> > --- a/src/qemu_internal.c	2007-02-23 21:46:35.000000000 +0900
> > +++ b/src/qemu_internal.c	2007-03-01 02:18:02.000000000 +0900
> > @@ -1200,6 +1200,7 @@ static virDriver qemuDriver = {
> >      NULL, /* domainDetachDevice */
> >      qemuDomainGetAutostart, /* domainGetAutostart */
> >      qemuDomainSetAutostart, /* domainSetAutostart */
> > +    NULL, /* cpumax */
> >  };
> 
> W e should an impl for QEMU, and in fact this makes me think that we should
> definitely make it virDomainGetMaxVcpus(virDomainPtr dom) because in the QEMU
> case the limit is different depending on what type of domain we've created.
> ie, a QEMU or KQEMU domain can have many VCPUs, but a KVM domain can currently
> only have 1 VCPU. So we need the virDomainptr object to implement this logic.
> 
> 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  -=| 
> 


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