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

[Ján Tomko <jtomko@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> Jano



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

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list