Re: [libvirt] [PATCH 1/2] qemu: Save qemuDomainGetStats error

On Tue, Nov 27, 2018 at 11:23:22AM -0500, John Ferlan wrote: >During qemuConnectGetAllDomainStats if qemuDomainGetStats causes >a failure, then when collecting more than one domain's worth of >statistics the loop in virDomainStatsRecordListFree would call >virDomainFree which would call virResetLastError effectively wiping >out the reason we failed leaving the caller with no idea why the >collection failed. > >To fix this, let's save the error and restore it prior to return >so that a caller such as 'virsh domstats' doesn't get the generic >"error: An error occurred, but the cause is unknown". > >Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >--- >src/qemu/qemu_driver.c | 6 ++++++ >1 file changed, 6 insertions(+) > >diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >index 7d9e17e72c..2fb8eef609 100644 >--- a/src/qemu/qemu_driver.c >+++ b/src/qemu/qemu_driver.c >@@ -21092,6 +21092,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, > unsigned int flags) >{ > virQEMUDriverPtr driver = conn->privateData; >+ virErrorPtr save_err = NULL; git grep virError src/qemu shows most files use orig_err as the variable name > virDomainObjPtr *vms = NULL; > virDomainObjPtr vm; > size_t nvms; >@@ -21160,6 +21161,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, > if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING) > domflags |= QEMU_DOMAIN_STATS_BACKING; > if (qemuDomainGetStats(conn, vm, stats, &tmp, domflags) < 0) { >+ save_err = virSaveLastError(); Since virDomainStatsRecordListFree is the one resetting the error, I think we should only save it right before that call.

This would cause a memory leak if qemuDomainGetStats would fail for more than one domain.

> if (HAVE_JOB(domflags) && vm) > qemuDomainObjEndJob(driver, vm); > >@@ -21184,6 +21186,10 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, > cleanup: Here. And we have a specific helper for that: virErrorPreserveLast(&orig_err); > virDomainStatsRecordListFree(tmpstats); > virObjectListFreeCount(vms, nvms); >+ if (save_err) { >+ virSetError(save_err); >+ virFreeError(save_err); >+ } virErrorRestore(&orig_err); With that addressed: Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list