[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

Masayuki changes the function 
from virGetCpuMax to virDomainGetMaxVcpus.
but not changes  xenHypervisorGetCpuMax.
This is based on your suggestion.

Is this code satify your criteria?

If this code satisfied, he will make final code for the commiting.

Thanks
Atsushi SAKAI


 

Masayuki Sunou <fj1826dm aa jp fujitsu com> wrote:

> 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  -=| 
> > 
> 
> --
> Libvir-list mailing list
> Libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list



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