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;
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();
if (HAVE_JOB(domflags) && vm)
qemuDomainObjEndJob(driver, vm);
@@ -21184,6 +21186,10 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
cleanup:
virDomainStatsRecordListFree(tmpstats);
virObjectListFreeCount(vms, nvms);
+ if (save_err) {
+ virSetError(save_err);
+ virFreeError(save_err);
+ }
return ret;
}
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 A (probably) silly question, but why do we have to call virResetLastError as within virDomainFree in the first place? Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 28, 2018 at 12:49:14PM +0100, Erik Skultety 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 > > A (probably) silly question, but why do we have to call virResetLastError as > within virDomainFree in the first place? Well virDomainFree can return an error just like any other API can :-) See virCheckDomainReturn(...) Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 28, 2018 at 12:08:51PM +0000, Daniel P. Berrangé wrote: > On Wed, Nov 28, 2018 at 12:49:14PM +0100, Erik Skultety 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 > > > > A (probably) silly question, but why do we have to call virResetLastError as > > within virDomainFree in the first place? > > Well virDomainFree can return an error just like any other API can :-) > See virCheckDomainReturn(...) In which case we'd get into virRaiseErrorFull which already resets the error object inside, that's why I'm asking whether there's another reason for it. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 28, 2018 at 01:26:17PM +0100, Erik Skultety wrote: > On Wed, Nov 28, 2018 at 12:08:51PM +0000, Daniel P. Berrangé wrote: > > On Wed, Nov 28, 2018 at 12:49:14PM +0100, Erik Skultety 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 > > > > > > A (probably) silly question, but why do we have to call virResetLastError as > > > within virDomainFree in the first place? > > > > Well virDomainFree can return an error just like any other API can :-) > > See virCheckDomainReturn(...) > > In which case we'd get into virRaiseErrorFull which already resets the error > object inside, that's why I'm asking whether there's another reason for it. That is insuficient as APIs need to guarantee that when they *don't* take the error path, the last error is clear. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 28, 2018 at 12:28:21PM +0000, Daniel P. Berrangé wrote: > On Wed, Nov 28, 2018 at 01:26:17PM +0100, Erik Skultety wrote: > > On Wed, Nov 28, 2018 at 12:08:51PM +0000, Daniel P. Berrangé wrote: > > > On Wed, Nov 28, 2018 at 12:49:14PM +0100, Erik Skultety 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 > > > > > > > > A (probably) silly question, but why do we have to call virResetLastError as > > > > within virDomainFree in the first place? > > > > > > Well virDomainFree can return an error just like any other API can :-) > > > See virCheckDomainReturn(...) > > > > In which case we'd get into virRaiseErrorFull which already resets the error > > object inside, that's why I'm asking whether there's another reason for it. > > That is insuficient as APIs need to guarantee that when they *don't* take > the error path, the last error is clear. Right, makes sense, thanks. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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@redhat.com>
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
>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@redhat.com>
>
>Jano
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/6/18 10:15 AM, Ján Tomko wrote:
> 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
>> name
>>
OK - I'll modify... Of course the example I cut-n-paste'd from was
save_err - chances were slim, but possible.
>>> 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.
>
It was a 50/50 coin flip for placement. I'm fine with and will move this
to just before the virDomainStatsRecordListFree. Perhaps the more common
model used throughout the code. I hedged my "bets" by putting it here
because I thought it was clearer as to why an error would be saved.
>>
>>> 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);
>>
We should be "more consistent" in the code regarding usage to avoid
future copypasta's, but going with Preserve and Restore is fine by me.
And by more consistent the underlying question/tone is - should the
virSaveLastError w/ paired virSetError/virFreeError all be replaced with
the Preserve/Restore... I see that could cause a lot of short term
churn, but perhaps the long term gain is worth it. The second question
then becomes all at one time or module my module...
Thanks for the review - the changes are made here at least.
John
>>> 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@redhat.com>
>>
>> Jano
>
>
>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.