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

[Ján Tomko]

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 at redhat.com>
>---
> 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 at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181206/a7d39ab5/attachment-0001.sig>