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

[Ján Tomko]

On Thu, Dec 06, 2018 at 02:49:59PM +0100, Ján Tomko wrote:
>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.

Ehm, disregard this sentence. (Thanks Erik for pointing that out)
But I still consider the cleanup section a beter place for this.

Jano

>
>>            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



>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

-------------- 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/1a7e35c2/attachment-0001.sig>