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(a)aa.jp.fujitsu.com&gt; 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(a)redhat.com&gt; "Re: [Libvir] [PATCH] check the maximum of virtual CPU" ""Daniel P. Berrange" <berrange(a)redhat.com&gt;&quot; 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 -=| > -- Libvir-list mailing list Libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list