[libvirt] [PATCH 0/4] Some small libxl bug fixes
While working on a patch to add dom0 support to the libxl driver, I noticed a few existing bugs which are fixed by this small series. IMO 1 and 2 are safe even during the freeze as they fix quite annoying bugs. 3 and 4 are probably safe bug fixes too, unless I'm confused about the use of domainObj->{def,newDef,persistentDef} Jim Fehlig (4): libxl: don't overwrite domain state from statedir config libxl: don't remove persistent domain on start failure libxl: honor domainGetXMLDesc() --inactive flag libxl: Set def->vcpus after successfully modifying live vcpu count src/libxl/libxl_driver.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) -- 2.3.7
When restarting libvirtd and reconnecting to running domains, libxlReconnectDomain() would unconditionally set the domain state to VIR_DOMAIN_RUNNING, overwriting the state maintained in $statedir/<domname>.xml. A domain in a paused state would have the state changed to running, even though it was actually in a paused state. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index a7be745..8a8d27d 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -372,8 +372,6 @@ libxlReconnectDomain(virDomainObjPtr vm, vm->def, VIR_HOSTDEV_SP_PCI) < 0) goto out; - virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); -- 2.3.7
libxlDomainCreateXML() would remove a persistent domain if libxlDomainStart() failed. Check if domain is persistent before removing. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 8a8d27d..fb9523a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -918,7 +918,10 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, if (libxlDomainStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1) < 0) { - virDomainObjListRemove(driver->domains, vm); + if (!vm->persistent) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } goto endjob; } -- 2.3.7
The libxl driver always uses virDomainObj->def when formatting the domain XML description. Use virDomainObj->newDef when --inactive flag is set. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index fb9523a..1a5b1a7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2429,6 +2429,7 @@ static char * libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { virDomainObjPtr vm; + virDomainDefPtr def; char *ret = NULL; /* Flags checked by virDomainDefFormat */ @@ -2439,8 +2440,13 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - ret = virDomainDefFormat(vm->def, - virDomainDefFormatConvertXMLFlags(flags)); + if ((flags & VIR_DOMAIN_XML_INACTIVE) && vm->newDef) + def = vm->newDef; + else + def = vm->def; + + ret = virDomainDefFormat(def, + virDomainDefFormatConvertXMLFlags(flags)); cleanup: if (vm) -- 2.3.7
def->vcpus was never updated after successfully changing the live vcpu count of a domain. Subsequent queries for vcpu info would return incorrect results. E.g.: virsh vcpucount test maximum config 4 maximum live 4 current config 4 current live 4 virsh setvcpus test 2 virsh vcpucount test maximum config 4 maximum live 4 current config 4 current live 4 After patch, live current config is reported correctly: virsh vcpucount test maximum config 4 maximum live 4 current config 4 current live 2 While fixing this, noticed that the live config was not saved to cfg->stateDir via virDomainSaveStatus. Save the live config and change error handling of virDomainSave{Config,Status} to log a message via VIR_WARN, instead of failing the entire DomainSetVcpusFlags operation. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 1a5b1a7..96c9d96 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2101,6 +2101,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, " with libxenlight"), vm->def->id); goto endjob; } + vm->def->vcpus = nvcpus; break; case VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG: @@ -2110,14 +2111,25 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, " with libxenlight"), vm->def->id); goto endjob; } + vm->def->vcpus = nvcpus; def->vcpus = nvcpus; break; } ret = 0; - if (flags & VIR_DOMAIN_VCPU_CONFIG) - ret = virDomainSaveConfig(cfg->configDir, def); + if (flags & VIR_DOMAIN_VCPU_LIVE) { + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { + VIR_WARN("Unable to save status on vm %s after changing vcpus", + vm->def->name); + } + } + if (flags & VIR_DOMAIN_VCPU_CONFIG) { + if (virDomainSaveConfig(cfg->configDir, def) < 0) { + VIR_WARN("Unable to save configuration of vm %s after changing vcpus", + vm->def->name); + } + } endjob: if (!libxlDomainObjEndJob(driver, vm)) -- 2.3.7
On Mon, Jun 29, 2015 at 11:30:11AM -0600, Jim Fehlig wrote:
While working on a patch to add dom0 support to the libxl driver, I noticed a few existing bugs which are fixed by this small series. IMO 1 and 2 are safe even during the freeze as they fix quite annoying bugs. 3 and 4 are probably safe bug fixes too, unless I'm confused about the use of domainObj->{def,newDef,persistentDef}
Jim Fehlig (4): libxl: don't overwrite domain state from statedir config libxl: don't remove persistent domain on start failure libxl: honor domainGetXMLDesc() --inactive flag libxl: Set def->vcpus after successfully modifying live vcpu count
src/libxl/libxl_driver.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
Those looks reasonable to me but 1/ I'm a bit rusty 2/ locking is hard so even if I ACK this take it with a grain of salt :-) Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veillard@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/
On 29.06.2015 19:30, Jim Fehlig wrote:
While working on a patch to add dom0 support to the libxl driver, I noticed a few existing bugs which are fixed by this small series. IMO 1 and 2 are safe even during the freeze as they fix quite annoying bugs. 3 and 4 are probably safe bug fixes too, unless I'm confused about the use of domainObj->{def,newDef,persistentDef}
Jim Fehlig (4): libxl: don't overwrite domain state from statedir config libxl: don't remove persistent domain on start failure libxl: honor domainGetXMLDesc() --inactive flag libxl: Set def->vcpus after successfully modifying live vcpu count
src/libxl/libxl_driver.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
ACK to all four and safe for the freeze. Michal
On 06/30/2015 04:08 AM, Michal Privoznik wrote:
On 29.06.2015 19:30, Jim Fehlig wrote:
While working on a patch to add dom0 support to the libxl driver, I noticed a few existing bugs which are fixed by this small series. IMO 1 and 2 are safe even during the freeze as they fix quite annoying bugs. 3 and 4 are probably safe bug fixes too, unless I'm confused about the use of domainObj->{def,newDef,persistentDef}
Jim Fehlig (4): libxl: don't overwrite domain state from statedir config libxl: don't remove persistent domain on start failure libxl: honor domainGetXMLDesc() --inactive flag libxl: Set def->vcpus after successfully modifying live vcpu count
src/libxl/libxl_driver.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
ACK to all four and safe for the freeze.
Thanks! I've pushed the series now. Regards, Jim
participants (3)
-
Daniel Veillard -
Jim Fehlig -
Michal Privoznik