[libvirt] [PATCH 0/2] libxl: a few minor fixes
A few minor fixes for patches 1 and 2 of Joao's series adding stats support to the libxl driver https://www.redhat.com/archives/libvir-list/2015-November/msg00489.html I only noticed the minor flaws while reviewing patches later in the series, after already committing patches 1 and 2. Jim Fehlig (2): libxl: unref libxlDriverConfig object libxl: don't unlock virDomainObj if refcnt is 0 src/libxl/libxl_driver.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) -- 2.1.4
Commits b6e19cf4 and 6472e54a missed unref'ing the libxlDriverConfig object. Add missing calls to virObjectUnref. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index fd4bae1..4609c00 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4648,18 +4648,21 @@ libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver, virTypedParameterPtr params, unsigned int nparams) { - libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDriverConfigPtr cfg; libxl_dominfo d_info; int ret = -1; if (nparams == 0) return LIBXL_NB_TOTAL_CPU_STAT_PARAM; + libxl_dominfo_init(&d_info); + cfg = libxlDriverConfigGet(driver); + if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("libxl_domain_info failed for domain '%d'"), vm->def->id); - return ret; + goto cleanup; } if (virTypedParameterAssign(¶ms[0], VIR_DOMAIN_CPU_STATS_CPUTIME, @@ -4670,6 +4673,7 @@ libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver, cleanup: libxl_dominfo_dispose(&d_info); + virObjectUnref(cfg); return ret; } @@ -4684,7 +4688,7 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver, libxl_vcpuinfo *vcpuinfo; int maxcpu, hostcpus; size_t i; - libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDriverConfigPtr cfg; int ret = -1; if (nparams == 0 && ncpus != 0) @@ -4692,12 +4696,13 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver, else if (nparams == 0) return vm->def->maxvcpus; + cfg = libxlDriverConfigGet(driver); if ((vcpuinfo = libxl_list_vcpu(cfg->ctx, vm->def->id, &maxcpu, &hostcpus)) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to list vcpus for domain '%d' with libxenlight"), vm->def->id); - return ret; + goto cleanup; } for (i = start_cpu; i < maxcpu && i < ncpus; ++i) { @@ -4710,7 +4715,9 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver, ret = nparams; cleanup: - libxl_vcpuinfo_list_free(vcpuinfo, maxcpu); + if (vcpuinfo) + libxl_vcpuinfo_list_free(vcpuinfo, maxcpu); + virObjectUnref(cfg); return ret; } @@ -4766,7 +4773,7 @@ libxlDomainMemoryStats(virDomainPtr dom, unsigned int flags) { libxlDriverPrivatePtr driver = dom->conn->privateData; - libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDriverConfigPtr cfg; virDomainObjPtr vm; libxl_dominfo d_info; unsigned mem, maxmem; @@ -4775,6 +4782,8 @@ libxlDomainMemoryStats(virDomainPtr dom, virCheckFlags(0, -1); + cfg = libxlDriverConfigGet(driver); + if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup; @@ -4815,6 +4824,7 @@ libxlDomainMemoryStats(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); + virObjectUnref(cfg); return ret; } -- 2.1.4
On Wed, Nov 18, 2015 at 03:47:24PM -0700, Jim Fehlig wrote:
Commits b6e19cf4 and 6472e54a missed unref'ing the libxlDriverConfig object. Add missing calls to virObjectUnref.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Wed, Nov 18, 2015 at 03:47:24PM -0700, Jim Fehlig wrote:
Commits b6e19cf4 and 6472e54a missed unref'ing the libxlDriverConfig object. Add missing calls to virObjectUnref.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
ACK
Thanks. I've pushed this and 2/2. Regards, Jim
On 11/18/2015 10:47 PM, Jim Fehlig wrote:
Commits b6e19cf4 and 6472e54a missed unref'ing the libxlDriverConfig object. Add missing calls to virObjectUnref.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index fd4bae1..4609c00 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4648,18 +4648,21 @@ libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver, virTypedParameterPtr params, unsigned int nparams) { - libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDriverConfigPtr cfg; libxl_dominfo d_info; int ret = -1;
if (nparams == 0) return LIBXL_NB_TOTAL_CPU_STAT_PARAM;
+ libxl_dominfo_init(&d_info); + cfg = libxlDriverConfigGet(driver); + if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("libxl_domain_info failed for domain '%d'"), vm->def->id); - return ret; + goto cleanup; }
if (virTypedParameterAssign(¶ms[0], VIR_DOMAIN_CPU_STATS_CPUTIME, @@ -4670,6 +4673,7 @@ libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver,
cleanup: libxl_dominfo_dispose(&d_info); + virObjectUnref(cfg); return ret; }
@@ -4684,7 +4688,7 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver, libxl_vcpuinfo *vcpuinfo; int maxcpu, hostcpus; size_t i; - libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDriverConfigPtr cfg; int ret = -1;
if (nparams == 0 && ncpus != 0) @@ -4692,12 +4696,13 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver, else if (nparams == 0) return vm->def->maxvcpus;
+ cfg = libxlDriverConfigGet(driver); if ((vcpuinfo = libxl_list_vcpu(cfg->ctx, vm->def->id, &maxcpu, &hostcpus)) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to list vcpus for domain '%d' with libxenlight"), vm->def->id); - return ret; + goto cleanup; }
for (i = start_cpu; i < maxcpu && i < ncpus; ++i) { @@ -4710,7 +4715,9 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver, ret = nparams;
cleanup: - libxl_vcpuinfo_list_free(vcpuinfo, maxcpu); + if (vcpuinfo) + libxl_vcpuinfo_list_free(vcpuinfo, maxcpu); + virObjectUnref(cfg); return ret; }
@@ -4766,7 +4773,7 @@ libxlDomainMemoryStats(virDomainPtr dom, unsigned int flags) { libxlDriverPrivatePtr driver = dom->conn->privateData; - libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxlDriverConfigPtr cfg; virDomainObjPtr vm; libxl_dominfo d_info; unsigned mem, maxmem; @@ -4775,6 +4782,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
virCheckFlags(0, -1);
+ cfg = libxlDriverConfigGet(driver); + if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup;
@@ -4815,6 +4824,7 @@ libxlDomainMemoryStats(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); + virObjectUnref(cfg); return ret; }
Looks great and also tested this series. I see you also moved cfg init further down which avoids unnecessary initialization for some cases. Regards, Joao
Commit 6472e54a unlocks the virDomainObj even if libxlDomainObjEndJob returns false, indicating that its refcnt has dropped to 0. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4609c00..d77a0e4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4816,10 +4816,8 @@ libxlDomainMemoryStats(virDomainPtr dom, libxl_dominfo_dispose(&d_info); endjob: - if (!libxlDomainObjEndJob(driver, vm)) { - virObjectUnlock(vm); + if (!libxlDomainObjEndJob(driver, vm)) vm = NULL; - } cleanup: if (vm) -- 2.1.4
On Wed, Nov 18, 2015 at 03:47:25PM -0700, Jim Fehlig wrote:
Commit 6472e54a unlocks the virDomainObj even if libxlDomainObjEndJob returns false, indicating that its refcnt has dropped to 0.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/18/2015 10:47 PM, Jim Fehlig wrote:
Commit 6472e54a unlocks the virDomainObj even if libxlDomainObjEndJob returns false, indicating that its refcnt has dropped to 0.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4609c00..d77a0e4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4816,10 +4816,8 @@ libxlDomainMemoryStats(virDomainPtr dom, libxl_dominfo_dispose(&d_info);
endjob: - if (!libxlDomainObjEndJob(driver, vm)) { - virObjectUnlock(vm); + if (!libxlDomainObjEndJob(driver, vm)) vm = NULL; - }
cleanup: if (vm)
And it looks this mistake is also on all patches that use libxlDomainObjEndJob so will fix that on the next version (interface, block, getalldomainstats. Regards, Joao
participants (3)
-
Daniel P. Berrange -
Jim Fehlig -
Joao Martins