[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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

   virDomainObjPtr *vms = NULL;
   virDomainObjPtr vm;
   size_t nvms;
@@ -21160,6 +21161,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
           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.


           if (HAVE_JOB(domflags) && vm)
               qemuDomainObjEndJob(driver, vm);

@@ -21184,6 +21186,10 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,

Here. And we have a specific helper for that:

   virObjectListFreeCount(vms, nvms);
+    if (save_err) {
+        virSetError(save_err);
+        virFreeError(save_err);
+    }


With that addressed:
Reviewed-by: Ján Tomko <jtomko redhat com>


libvir-list mailing list
libvir-list redhat com

Attachment: signature.asc
Description: PGP signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]