[Libvir] [PATCH] Implement SetVcpus and DomainGetMaxVcpus for qemu
The attached patch fills in two of the vcpu functions for the qemu driver: virDomainSetVcpus : set the number of vcpus the domain can use virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain. Code change is only in qemu_driver, as the backend stuff was already in place. I also edited qemudGetMaxVcpus to ignore case when checking the passed OS type, since it wasn't matching the returned results of qemudDomainGetOSType. Thanks, Cole diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b65ae66..8bedf5a 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -113,6 +113,8 @@ static int qemudShutdownNetworkDaemon(virConnectPtr conn, struct qemud_driver *driver, struct qemud_network *network); +static int qemudDomainGetMaxVcpus(virDomainPtr dom); + static struct qemud_driver *qemu_driver = NULL; @@ -1524,21 +1526,23 @@ static const char *qemudGetType(virConnectPtr conn ATTRIBUTE_UNUSED) { return "QEMU"; } -static int qemudGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED, - const char *type) { +static int qemudGetMaxVCPUs(virConnectPtr conn, const char *type) { if (!type) return 16; - if (!strcmp(type, "qemu")) + if (!strcasecmp(type, "qemu")) return 16; /* XXX future KVM will support SMP. Need to probe kernel to figure out KVM module version i guess */ - if (!strcmp(type, "kvm")) + if (!strcasecmp(type, "kvm")) return 1; - if (!strcmp(type, "kqemu")) + if (!strcasecmp(type, "kqemu")) return 1; + + qemudReportError(conn, NULL, NULL, VIR_ERR_INVALID_ARG, + _("unknown type '%s'"), type); return -1; } @@ -2122,6 +2126,66 @@ static int qemudDomainSave(virDomainPtr dom, } +static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid); + int max; + + if (!vm) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + _("no domain with matching uuid '%s'"), dom->uuid); + return -1; + } + + if (qemudIsActiveVM(vm)) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change vcpu count of an active domain")); + return -1; + } + + if ((max = qemudDomainGetMaxVcpus(dom)) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + _("could not determine max vcpus for the domain")); + return -1; + } + + if (nvcpus > max) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG, + _("requested vcpus is greater than max allowable" + " vcpus for the domain: %d > %d"), nvcpus, max); + return -1; + } + + vm->def->vcpus = nvcpus; + return 0; +} + +static int qemudDomainGetMaxVcpus(virDomainPtr dom) { + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid); + char *type; + int ret; + + if (!vm) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + _("no domain with matching uuid '%s'"), dom->uuid); + return -1; + } + + if (!(type = qemudDomainGetOSType(dom))) { + return -1; + } + + if ((ret = qemudGetMaxVCPUs(dom->conn, vm->def->virtType)) < 0) { + free(type); + return -1; + } + + free(type); + return ret; +} + + static int qemudDomainRestore(virConnectPtr conn, const char *path) { struct qemud_driver *driver = (struct qemud_driver *)conn->privateData; @@ -3042,10 +3106,10 @@ static virDriver qemuDriver = { qemudDomainSave, /* domainSave */ qemudDomainRestore, /* domainRestore */ NULL, /* domainCoreDump */ - NULL, /* domainSetVcpus */ + qemudDomainSetVcpus, /* domainSetVcpus */ NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ - NULL, /* domainGetMaxVcpus */ + qemudDomainGetMaxVcpus, /* domainGetMaxVcpus */ qemudDomainDumpXML, /* domainDumpXML */ qemudListDefinedDomains, /* listDomains */ qemudNumDefinedDomains, /* numOfDomains */
Cole Robinson wrote:
The attached patch fills in two of the vcpu functions for the qemu driver:
virDomainSetVcpus : set the number of vcpus the domain can use virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain.
Code change is only in qemu_driver, as the backend stuff was already in place. I also edited qemudGetMaxVcpus to ignore case when checking the passed OS type, since it wasn't matching the returned results of qemudDomainGetOSType.
OK, but...
+ if (qemudIsActiveVM(vm)) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change vcpu count of an active domain")); + return -1; + }
I'm fairly certain KVM is going to support this in the near future (if not now, I can't remember). I'm not sure what libvirt's policy is about things that don't work now, but will potentially work in the future, is, but it seems wrong to disable this feature for KVM. Chris Lalancette
On Mon, Apr 28, 2008 at 02:38:52PM -0400, Chris Lalancette wrote:
Cole Robinson wrote:
The attached patch fills in two of the vcpu functions for the qemu driver:
virDomainSetVcpus : set the number of vcpus the domain can use virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain.
Code change is only in qemu_driver, as the backend stuff was already in place. I also edited qemudGetMaxVcpus to ignore case when checking the passed OS type, since it wasn't matching the returned results of qemudDomainGetOSType.
OK, but...
+ if (qemudIsActiveVM(vm)) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change vcpu count of an active domain")); + return -1; + }
I'm fairly certain KVM is going to support this in the near future (if not now, I can't remember). I'm not sure what libvirt's policy is about things that don't work now, but will potentially work in the future, is, but it seems wrong to disable this feature for KVM.
Once there is a monitor API to support this we'll implement support for it, and detect the lack of the monitor command for old versions. Until then, this is correct behaviour. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Apr 28, 2008 at 01:46:28PM -0400, Cole Robinson wrote:
The attached patch fills in two of the vcpu functions for the qemu driver:
virDomainSetVcpus : set the number of vcpus the domain can use virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain.
Code change is only in qemu_driver, as the backend stuff was already in place. I also edited qemudGetMaxVcpus to ignore case when checking the passed OS type, since it wasn't matching the returned results of qemudDomainGetOSType.
Thanks, Cole
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b65ae66..8bedf5a 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -113,6 +113,8 @@ static int qemudShutdownNetworkDaemon(virConnectPtr conn, struct qemud_driver *driver, struct qemud_network *network);
+static int qemudDomainGetMaxVcpus(virDomainPtr dom); + static struct qemud_driver *qemu_driver = NULL;
@@ -1524,21 +1526,23 @@ static const char *qemudGetType(virConnectPtr conn ATTRIBUTE_UNUSED) { return "QEMU"; }
-static int qemudGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED, - const char *type) { +static int qemudGetMaxVCPUs(virConnectPtr conn, const char *type) { if (!type) return 16;
- if (!strcmp(type, "qemu")) + if (!strcasecmp(type, "qemu")) return 16;
/* XXX future KVM will support SMP. Need to probe kernel to figure out KVM module version i guess */ - if (!strcmp(type, "kvm")) + if (!strcasecmp(type, "kvm")) return 1;
This comment is seriously out of date - KVM supports 16 (or was is 32?) vCPUs, so we should change this. Aside from that, this looks OK to me. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
/* XXX future KVM will support SMP. Need to probe kernel to figure out KVM module version i guess */ - if (!strcmp(type, "kvm")) + if (!strcasecmp(type, "kvm")) return 1;
This comment is seriously out of date - KVM supports 16 (or was is 32?) vCPUs, so we should change this.
According to the kvm changelog, 16 vcpus is supported since kvm-62. Prior to that (such as on f8) only 4 seem to be supported. - Cole
On Mon, Apr 28, 2008 at 01:46:28PM -0400, Cole Robinson wrote: [...] Fine except:
- if (!strcmp(type, "qemu")) + if (!strcasecmp(type, "qemu")) return 16;
/* XXX future KVM will support SMP. Need to probe kernel to figure out KVM module version i guess */ - if (!strcmp(type, "kvm")) + if (!strcasecmp(type, "kvm")) return 1;
- if (!strcmp(type, "kqemu")) + if (!strcasecmp(type, "kqemu")) return 1;
Can you change these equality tests to use the STR* macros from src/internal.h before committing. Thanks, Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
Cole Robinson <crobinso@redhat.com> wrote:
The attached patch fills in two of the vcpu functions for the qemu driver:
virDomainSetVcpus : set the number of vcpus the domain can use virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain.
Code change is only in qemu_driver, as the backend stuff was already in place. I also edited qemudGetMaxVcpus to ignore case when checking the passed OS type, since it wasn't matching the returned results of qemudDomainGetOSType.
Hi Cole, This looks fine, modulo a couple nits: ...
diff --git a/src/qemu_driver.c b/src/qemu_driver.c ... +static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
Since it doesn't affect an interface, it's less important, but I think "driver" can be a const pointer.
+ struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid); + int max; + + if (!vm) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + _("no domain with matching uuid '%s'"), dom->uuid); + return -1; + } + + if (qemudIsActiveVM(vm)) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change vcpu count of an active domain")); + return -1; + } + + if ((max = qemudDomainGetMaxVcpus(dom)) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
Please insert a "%s" argument here, like above, since the message contains no "%"-directive. This will avoid a compile-time warning.
+ _("could not determine max vcpus for the domain")); + return -1; + } + + if (nvcpus > max) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG, + _("requested vcpus is greater than max allowable" + " vcpus for the domain: %d > %d"), nvcpus, max); + return -1; + } + + vm->def->vcpus = nvcpus; + return 0; +}
Cole Robinson wrote:
The attached patch fills in two of the vcpu functions for the qemu driver:
virDomainSetVcpus : set the number of vcpus the domain can use virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain.
Updated patch. Fixes the issues pointed out by Jim and Rich. I also realized I was passing the incorrect value to qemudMaxVCPUS, so to help fix this I added a function to qemu_conf.c to translate a domain's virtType value to the corresponding string. Any feedback is appreciated. Thanks, Cole diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 1efd9e9..d845d18 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -554,6 +554,21 @@ int qemudExtractVersion(virConnectPtr conn, return 0; } +/* Converts def->virtType to applicable string type + * @param type integer virt type + * @return string type on success, NULL on fail + */ +const char * qemudVirtTypeToString(int type) { + switch (type) { + case QEMUD_VIRT_QEMU: + return "qemu"; + case QEMUD_VIRT_KQEMU: + return "kqemu"; + case QEMUD_VIRT_KVM: + return "kvm"; + } + return NULL; +} /* Parse the XML definition for a disk * @param disk pre-allocated & zero'd disk record @@ -1699,9 +1714,12 @@ static struct qemud_vm_def *qemudParseXML(virConnectPtr conn, obj = xmlXPathEval(BAD_CAST "string(/domain/devices/emulator[1])", ctxt); if ((obj == NULL) || (obj->type != XPATH_STRING) || (obj->stringval == NULL) || (obj->stringval[0] == 0)) { - const char *type = (def->virtType == QEMUD_VIRT_QEMU ? "qemu" : - def->virtType == QEMUD_VIRT_KQEMU ? "kqemu": - "kvm"); + const char *type = qemudVirtTypeToString(def->virtType); + if (!type) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("unknown virt type")); + goto error; + } const char *emulator = virCapabilitiesDefaultGuestEmulator(driver->caps, def->os.type, def->os.arch, @@ -3479,18 +3497,7 @@ char *qemudGenerateXML(virConnectPtr conn, const char *type = NULL; int n; - switch (def->virtType) { - case QEMUD_VIRT_QEMU: - type = "qemu"; - break; - case QEMUD_VIRT_KQEMU: - type = "kqemu"; - break; - case QEMUD_VIRT_KVM: - type = "kvm"; - break; - } - if (!type) { + if (!(type = qemudVirtTypeToString(def->virtType))) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unexpected domain type %d"), def->virtType); goto cleanup; diff --git a/src/qemu_conf.h b/src/qemu_conf.h index bd8d8b2..81e4aea 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -486,6 +486,7 @@ char * qemudGenerateNetworkXML (virConnectPtr conn, struct qemud_network *network, struct qemud_network_def *def); +const char *qemudVirtTypeToString (int type); #endif /* WITH_QEMU */ diff --git a/src/qemu_driver.c b/src/qemu_driver.c index e924633..cf52a07 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -113,6 +113,8 @@ static int qemudShutdownNetworkDaemon(virConnectPtr conn, struct qemud_driver *driver, struct qemud_network *network); +static int qemudDomainGetMaxVcpus(virDomainPtr dom); + static struct qemud_driver *qemu_driver = NULL; @@ -1563,21 +1565,23 @@ static const char *qemudGetType(virConnectPtr conn ATTRIBUTE_UNUSED) { return "QEMU"; } -static int qemudGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED, - const char *type) { +static int qemudGetMaxVCPUs(virConnectPtr conn, const char *type) { if (!type) return 16; - if (!strcmp(type, "qemu")) + if (STRCASEEQ(type, "qemu")) return 16; /* XXX future KVM will support SMP. Need to probe kernel to figure out KVM module version i guess */ - if (!strcmp(type, "kvm")) + if (STRCASEEQ(type, "kvm")) return 1; - if (!strcmp(type, "kqemu")) + if (STRCASEEQ(type, "kqemu")) return 1; + + qemudReportError(conn, NULL, NULL, VIR_ERR_INVALID_ARG, + _("unknown type '%s'"), type); return -1; } @@ -2161,6 +2165,67 @@ static int qemudDomainSave(virDomainPtr dom, } +static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { + const struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid); + int max; + + if (!vm) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + _("no domain with matching uuid '%s'"), dom->uuid); + return -1; + } + + if (qemudIsActiveVM(vm)) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change vcpu count of an active domain")); + return -1; + } + + if ((max = qemudDomainGetMaxVcpus(dom)) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, "%s", + _("could not determine max vcpus for the domain")); + return -1; + } + + if (nvcpus > max) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG, + _("requested vcpus is greater than max allowable" + " vcpus for the domain: %d > %d"), nvcpus, max); + return -1; + } + + vm->def->vcpus = nvcpus; + return 0; +} + +static int qemudDomainGetMaxVcpus(virDomainPtr dom) { + struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid); + const char *type; + int ret; + + if (!vm) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + _("no domain with matching uuid '%s'"), dom->uuid); + return -1; + } + + if (!(type = qemudVirtTypeToString(vm->def->virtType))) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + _("unknown virt type in domain definition '%d'"), + vm->def->virtType); + return -1; + } + + if ((ret = qemudGetMaxVCPUs(dom->conn, type)) < 0) { + return -1; + } + + return ret; +} + + static int qemudDomainRestore(virConnectPtr conn, const char *path) { struct qemud_driver *driver = (struct qemud_driver *)conn->privateData; @@ -3081,10 +3146,10 @@ static virDriver qemuDriver = { qemudDomainSave, /* domainSave */ qemudDomainRestore, /* domainRestore */ NULL, /* domainCoreDump */ - NULL, /* domainSetVcpus */ + qemudDomainSetVcpus, /* domainSetVcpus */ NULL, /* domainPinVcpu */ NULL, /* domainGetVcpus */ - NULL, /* domainGetMaxVcpus */ + qemudDomainGetMaxVcpus, /* domainGetMaxVcpus */ qemudDomainDumpXML, /* domainDumpXML */ qemudListDefinedDomains, /* listDomains */ qemudNumDefinedDomains, /* numOfDomains */
On Tue, May 06, 2008 at 12:35:12PM -0400, Cole Robinson wrote:
Cole Robinson wrote:
The attached patch fills in two of the vcpu functions for the qemu driver:
virDomainSetVcpus : set the number of vcpus the domain can use virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain.
Updated patch. Fixes the issues pointed out by Jim and Rich. I also realized I was passing the incorrect value to qemudMaxVCPUS, so to help fix this I added a function to qemu_conf.c to translate a domain's virtType value to the corresponding string.
Okidoc, looks fine, applied and commited, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (6)
-
Chris Lalancette -
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Richard W.M. Jones